[NA] [QA] Proposed e2e specs from the #8113 exploration — dataset/test-suite insert deduplication flag - #8114
Draft
CometActions wants to merge 1 commit into
Draft
Conversation
…lag (#8113) Turns the verified exploration candidates for #8113 into permanent specs. - Bridge: thread `deduplication` through the dataset and test-suite insert-items routes, and add `/datasets/insert-items-session` so a spec can run several inserts against ONE `Dataset` object — without it, the SDK's hash-cache invalidation is unobservable and the spec asserting it could never fail. - POM: `versionChangeSummary` on the dataset items page (the Version history "Changes" cell), `itemRowIds` on the test-suite items page, `itemRows` on both. - Specs: `deduplication=False` stores every duplicate and counts it in the version counters while the default collapses it and cuts no second version; a non-deduplicated insert invalidates the local hash cache so the next deduplicated insert still drops its duplicate; and the same flag on `TestSuite.insert`, rendered on the Test cases tab. - Taxonomy: both specs listed, capability notes scoped to what is now asserted. Generated by the release QA side flow. Draft — needs human review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
python
Contributor
📋 PR Linter Failed❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the |
Contributor
⏱️ pre-commit per-hook timingNo linted files changed — nothing to run. ⏭️ 44 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+148
to
+151
| # Mirrors Dataset.insert's own default. False bypasses the content-hash | ||
| # dedup path entirely: every item is sent as-is, so identical content | ||
| # inserted twice is stored twice. | ||
| deduplication: bool = True |
Contributor
There was a problem hiding this comment.
Initial-item deduplication remains unavailable
Only DatasetInsertItemsRequest exposes deduplication, so createDataset({items, deduplication:false}) is rejected by DatasetCreate's extra="forbid", while createTestSuite and the live experiment seed routes always use the default — should we forward the option through all insertion paths or explicitly document and enforce default-only create/seed operations?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/services/opik-sdk-driver/src/opik_sdk_driver/schemas.py` around
lines 148-151, the `deduplication` option is exposed only by
`DatasetInsertItemsRequest`, so `createDataset`, `createTestSuite`, and the live
experiment seed routes cannot represent non-default insertion behavior and may be
rejected by `extra="forbid"`. Add `deduplication: bool = True` to the corresponding
create/seed request models and update their handlers/bridge calls to forward it to
`Dataset.insert` or `TestSuite.insert`; add or update coverage for `False` in each path.
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| items: list[dict[str, Any]] | ||
| num_threads: int = 1 |
Contributor
There was a problem hiding this comment.
Invalid thread booleans reach dataset writes
DatasetInsertCall.num_threads accepts booleans as integers, so FastAPI coerces true into 1 before Dataset.insert can reject it, letting invalid requests write data — should we enforce strict positive-integer validation on this field while keeping the default of 1?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/services/opik-sdk-driver/src/opik_sdk_driver/schemas.py` around
lines 160-167, update `DatasetInsertCall.num_threads`, which currently coerces `true`
into `1` and permits invalid non-positive values before `Dataset.insert` runs. Use
strict integer validation with a positive constraint (while preserving the default of
1), so FastAPI rejects booleans and values below 1 during request validation before
dataset lookup or insertion.
Comment on lines
+160
to
+171
| class DatasetInsertCall(BaseModel): | ||
| """One `Dataset.insert(...)` inside an insert-items-session request.""" | ||
|
|
||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| items: list[dict[str, Any]] | ||
| num_threads: int = 1 | ||
| deduplication: bool = True | ||
|
|
||
|
|
||
| class DatasetInsertItemsSessionRequest(BaseModel): | ||
| """Several inserts against ONE `Dataset` object, in one client session. |
Contributor
There was a problem hiding this comment.
Missing unit coverage for request validation
The new DatasetInsertCall/DatasetInsertItemsSessionRequest boundary lacks tests for unknown fields at either nesting level and for inserts: [], so validation/status regressions can go unnoticed — should we add focused Pydantic/FastAPI tests asserting the intended behavior, as AGENTS.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/services/opik-sdk-driver/src/opik_sdk_driver/schemas.py` around
lines 160-171, the new `DatasetInsertCall` and `DatasetInsertItemsSessionRequest`
contract is not covered at its validation boundary. Add focused Pydantic/FastAPI tests
in the corresponding test module that submit unknown fields at both the nested-call and
top-level request levels, and exercise an empty `inserts` list, asserting the intended
validation or HTTP status behavior defined by the route. Keep the existing
`extra="forbid"` contract and update tests according to the conventions in `AGENTS.md`
and nearby endpoint tests.
Comment on lines
+61
to
+62
| }) => { | ||
| const offName = `${testNamespace}-dedup-off`; |
Contributor
There was a problem hiding this comment.
Add required test description annotations
The three new test callbacks begin without the required description annotation; should we add test.info().annotations.push({ type: 'description', description: '...' }) as the first statement in each body, per .agents/skills/playwright-e2e/test-conventions.md?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/tests/datasets/dataset-insert-deduplication.spec.ts` around lines
61-62, update the new dataset test callback so its first statement is the required
`test.info().annotations` description annotation, before declaring dataset names. Apply
the same convention to the other new test callback around lines 193-194 (and any
affected suite/test callback in this spec), using concise descriptions that explain each
test’s purpose and following `.agents/skills/playwright-e2e/test-conventions.md`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Generated by the release QA side flow (release-test-proposal). It is a draft on
purpose and needs a human review before merge — nothing here has been reviewed by a person.
Where these came from
#8113 —
[SDK] feat: allow dataset item insertion without deduplication. A human-driven exploration worked the change on the PR'sown deployed environment (
pr-8113.dev.comet.com, backend2.2.47-8113-merge-3176) andverified each flow below by hand before any spec was written.
Which base this targets, and why
The run that produced this was told #8113 was still open with its head branch alive, and to
base on
aliaksandrk/NA-dataset-insert-without-deduplication. That is no longer true:#8113 is merged (merge commit
976d1ca) and the head branch returns 404, so this PR isbased on
maininstead, which already carries the change.The specs were written against the PR's head commit
c684b655c30932b0f0fe54ba8155826a388f480aand then re-run onmain(this branch'sbase) before this PR was opened — see the run below. The app they ran against is the PR
environment, which is a merge build of #8113 into main, so both the SDK under test and the
e2e estate the specs live in are the post-merge code.
What is in here
Three things, all in
tests_end_to_end/:1. The bridge can express the flag (it could not before — the exploration flagged this as
a blocker for writing any spec at all).
deduplication: bool = TrueonDatasetInsertItemsRequestandTestSuiteInsertItemsRequest(both areextra="forbid"), threaded intoDataset.insert(...)/TestSuite.insert(...), and surfaced oninsertDatasetItems/insertTestSuiteItemsincore/sdk/python-sdk-client.ts.POST /datasets/insert-items-sessionroute +insertDatasetItemsSessionclientmethod.
/datasets/insert-itemsbuilds a fresh client — and therefore a freshDataset—per request, and a backend-fetched
Datasetalways starts with its local hash cachemarked unsynced. That makes the second spec below impossible to fail across separate
calls: the third insert would re-sync regardless of what the second one did. The new route
keeps one
Datasetalive across a sequence of inserts, which is the only shape where"the cache was invalidated and re-synced" is distinguishable from "a new object synced
because it always does."
2. Two POM additions, no new selectors invented:
DatasetItemsPage.versionChangeSummary(versionName)— the Version history "Changes" cell(
+ N/~ N/− N), addressed bydata-cell-idexactly like the existingversionItemCount. This is the only place a user sees the per-version added/modifiedsplit; "Item count" alone cannot tell a version that added one item from one that added
two and deleted one.
TestSuiteItemsPage.itemRowIds()(mirroring the dataset page) anditemRows()on both,so row counts use a retrying
toHaveCount(...)rather than a one-shotcountItems().3. Two specs. Each pairs the flag against its own default on a second entity seeded
identically, so the flag is the only variable and the test cannot pass by coincidence — not
against a backend that stored everything, nor one that collapsed everything.
tests/datasets/dataset-insert-deduplication.spec.tsdeduplication=false stores every duplicate and counts it; the default collapses duplicates into one item and one version@t2-cuj@cap:datasets.sdk-round-tripitems_total1→2,items_added1 each,items_modified0); the control holds 1 item and one version — the second call deduplicated to zero, so no batch was sent and none was cut. UI: 2 rows on the Records tab matched bydata-row-id(the rows are content-identical, so only identity shows both stored items are rendering rather than one twice), Item count1/2and Changes+ 1/+ 1on Version history; 1 row for the control.A deduplication=false insert invalidates the local hash cache, so the next deduplicated insert still drops the duplicate@t2-cuj@cap:datasets.version-history-view[ITEM]deduplicated,[ITEM, OTHER]not deduplicated,[OTHER, THIRD]deduplicated. Result is 4 items (2×ITEM, 1×OTHER, 1×THIRD) and versions+1 / +2 / +1over totals1 / 3 / 4, asserted in the API and rendered on Version history.One deliberate deviation from the verified flow, and it matters. The candidate's flow was
two
deduplication=Falseinserts followed by a deduplicated[duplicate, new]. Run throughthe bridge that sequence passes whether or not the SDK invalidates its cache, because the
session's
Datasetstarts unsynced and insert 3 would sync anyway. The order above startswith a deduplicated insert, which forces the one-shot sync and leaves the cache marked
in-sync holding only ITEM's hash — so insert 3 can only be right if the non-deduplicated
insert 2 invalidated it. Without that invalidation, OTHER is sent again under a fresh id and
v3 reads
+ 2over 6 items instead of+ 1over 4. Same behaviour the exploration verified,sharpened so the test can actually fail.
tests/test-suites/test-suite-insert-deduplication.spec.tsdeduplication=false stores both identical test cases and the Test cases tab renders both; the default stores one@t2-cuj@cap:test-suites.view-suite-itemsTestSuite.insertis a separate public entry point routing into the same internal insert. Dedup-off suite holds 2 items under distinct ids and renders 2 rows; the control holds 1 and renders 1. Rows matched bydata-row-id— the Test cases table renders only ID / Tags / Created, so the item content is not visible there and identity is the only discriminator. The content assertion is API-side for that reason.Taxonomy
Both specs added to their areas'
specs:lists. All three capabilities were alreadycovered: true, so no flip was needed; eachnote:was extended to scope what is nowasserted (
tag_lint.pydoes not check either of those, so they are worth reading).No new capability keys were invented.
Verification — passed
Run against the PR environment (
OPIK_BASE_URL=https://pr-8113.dev.comet.com, OSS install,workspace
default, no auth), fromtests_end_to_end/e2e/on this branch:python3 tests_end_to_end/coverage/tag_lint.py --taxonomy tests_end_to_end/coverage/taxonomy.yaml --estate tests_end_to_end→58 specs checked, 1 exempt, 0 problem(s).npx tsc --noEmitcould not be used as a gate here, and not because of this change:tsconfig.jsonstill setsbaseUrl, which TypeScript 7 (the pinned^7.0.2) has removed,so it fails identically on an untouched checkout. Type-checking with
baseUrldroppedreports the same 94 pre-existing errors before and after this change and none in the files
added or edited here. Worth fixing separately.
What was deliberately not written
deduplication=Falseinsert on the newnum_threads=4default.dataset-version-counters.spec.tsalready covers the parallel-upload-collapses-to-one-version axis at
num_threads=8andasserts on the result rather than the transport, so a new spec would add cost without
coverage.
DatasetInsertItemsRequestdefault from
num_threads=1to4so existing specs exercise the SDK's new default.Not done here — it silently changes how every existing dataset spec uploads, which is a
call for the estate's owners, not a side effect of a test proposal.
Reviewer notes
rather not have it, the second dataset test cannot be written honestly — say so and it can
be dropped rather than weakened into a test that always passes.
registerDatasetCleanup(datasets and test suites sharestorage and do not cascade with their project); no cleanup lives in a test body.