[OPIK-7791] [QA] Proposed e2e specs from the 2.2.48 → 2.2.49 release exploration - #8127
[OPIK-7791] [QA] Proposed e2e specs from the 2.2.48 → 2.2.49 release exploration#8127CometActions wants to merge 2 commits into
Conversation
Three flows a human verified on staging during the 2.2.48 -> 2.2.49 release exploration, turned into permanent specs: - online-evaluation: saving an LLM-judge rule with no edits must preserve code.model.custom_parameters. Asserts the outbound PATCH body and the persisted blob, since nothing about the regression is visible on screen. Flips online-evaluation.edit-rule to covered (scoped to the unedited save). - datasets: each row of the datasets list carries its own computed summary. Four datasets with pairwise-distinct shapes (one empty), checked API-side against their own detail/items/versions endpoints and across two pages, and on screen for item count and the two recency columns. - test-suites: a suite reached through get_test_suites() deduplicates an insert of an item it already holds, while a new item still lands. Supporting: LLM-judge rule + dataset-summary readers on the backend client, an edit-dialog opener/submitter on the online-evaluation POM, cell and column helpers on the datasets POM, a four-dataset seed fixture, and a resolve_via option on the sdk-driver's test-suite insert route. Generated by the release QA side flow against the 2.2.49 tag; needs review.
github-actions
Bot
added
python
🔍 Generated-test reviewPR #8127 — OPIK-7791 [QA] Proposed e2e specs from the 2.2.48 → 2.2.49 release explorationCometActions · draft: yes · 13 files, 3 new specs VERDICT: ready with notes No blockers. No changes applied — see "What I fixed" below. The two open items Blockers (0)None. All three specs parse, collect, are placed in their area's Should fix (1)1. Why: the spec opens the edit dialog and presses "Update rule" with nothing This is not the opik#7943 failure mode — the spec does drive the real edit Fix (yours to choose — I will not invent a capability key):
I lean (a), because those precedents all describe how a capability was Notes (3)1. The test-suites dedup assertion has no capability of its own. 2. The 3. Latent, unreachable today: What I verified, and what I could notVerified:
Could not verify:
Bot comments
What I fixedNothing — FIX MODE was on and I made no edits, so there is no commit and
Estate gates
I did not re-run AssessmentThese are good specs — better than the estate's median. Each one names the This PR is ready to be marked ready for review by a human, once you decide Offered, not done — say the word:
review_generated_tests.yml · |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const patched = page.waitForRequest( | ||
| (request) => | ||
| request.method() === 'PATCH' && isRuleUpdate(request.url(), ruleId), | ||
| ); | ||
| await onlineEval.submitRuleDialog(); | ||
| return patched; |
There was a problem hiding this comment.
Reread can pass before persistence
waitForRequest() only confirms the PATCH was dispatched, so getLlmJudgeModel() can read the pre-save blob before the transaction commits and pass even when custom_parameters is dropped — should we await the matching page.waitForResponse() with response.ok() (or poll an equivalent authoritative condition) first?
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/tests/online-evaluation/online-evaluation-edit-rule-preserves-model-parameters.spec.ts`
around lines 116-121, update the edit-dialog submission logic to wait for the matching
PATCH response as well as the request. Arm both listeners before calling
`submitRuleDialog`, await the response, and require `response.ok()` before proceeding so
`getLlmJudgeModel()` runs only after the update has completed.
There was a problem hiding this comment.
Valid, and the barrier is weaker than it looks: AddEditRuleDialog calls setOpen(false) outside the mutation's callbacks (AddEditRuleDialog.tsx:485-493), so the dialog hides on dispatch and submitRuleDialog's hidden-dialog wait was no barrier at all — the persisted re-read could have observed the pre-save blob and passed on a backend that dropped custom_parameters, which is the exact regression this spec exists to catch. Fixed in d71f328: both listeners armed before the click, response.ok() asserted before the re-read. Good catch.
There was a problem hiding this comment.
Commit d71f328 addressed this comment by waiting for the matching PATCH response and asserting response.ok() before reading the persisted model.
| await use(ref); | ||
|
|
||
| if (!shouldLeaveArtifacts(testInfo)) { |
There was a problem hiding this comment.
Partial fixture setup leaks test resources
The fixture registers cleanup only after all seeding and await use(ref), while datasets adds each dataset only after its children finish, so setup failures leave traces and dataset items behind — should we register resources immediately and run child-before-parent cleanup in a failure-safe try/finally that preserves the original setup error?
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/fixtures/summarised-datasets.fixture.ts` around lines 188-190,
refactor the `summarisedDatasets` fixture so setup failures cannot bypass cleanup.
Register each dataset, trace, experiment, optimization, and related resource immediately
after creation, including partially seeded datasets, then wrap seeding and `await
use(ref)` in a `try/finally` that runs the existing child-before-parent cleanup.
Preserve cleanup error handling so teardown failures are logged without masking the
original setup exception.
There was a problem hiding this comment.
Accurate about the shape, but this is the estate's convention rather than something new here: none of the six multi-entity sibling fixtures (dataset, grouped-dataset, experiment, comparison-experiment, project-metric-spans, optimization-run) wraps use() in try/finally — all of them seed, await use(ref), then clean up with a try per delete, exactly as this one does. Fixing it here alone would make this the only fixture with different failure semantics. Deferring to an estate-wide fixture-teardown change instead.
There was a problem hiding this comment.
Thanks for the context—keeping fixture teardown semantics consistent and addressing this estate-wide makes sense. I’ll remember this convention after the PR is merged.
| datasetItemsCount: requireCount(d.dataset_items_count, 'dataset_items_count'), | ||
| experimentCount: requireCount(d.experiment_count, 'experiment_count'), | ||
| optimizationCount: requireCount(d.optimization_count, 'optimization_count'), | ||
| latestVersionHash: d.latest_version?.version_hash ?? null, |
There was a problem hiding this comment.
Malformed version data passes as empty
latest_version?.version_hash ?? null converts a missing version_hash into null, so an incomplete latest_version is indistinguishable from no latest version and can bypass summary assertions — should we validate version_hash when latest_version is present and reserve null for an absent or explicitly null API value?
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/core/backend/client.ts around lines 364-364, update the
`toDatasetSummary` mapper so `latest_version?.version_hash ?? null` does not treat a
missing hash on a present `latest_version` as valid. Return `null` only when
`latest_version` is absent/null or its hash is explicitly null; otherwise validate that
`version_hash` is a string and throw a descriptive error for missing or invalid values,
consistent with the mapper’s strict count validation.
There was a problem hiding this comment.
Not reachable from the consumer. The spec asserts latest[0].versionHash is not.toBeNull() and then compares the listed latestVersionHash against the dataset's actual latest version, so a latest_version present with a missing hash fails either way. The mapper's strict count validation exists because the empty-dataset case asserts zeros — a missing count would otherwise default into a passing assertion; the hash has no equivalent default to hide behind, so ?? null is not doing the same work there.
There was a problem hiding this comment.
Thanks for clarifying—the hash is already covered by the explicit assertions, unlike the count defaulting case. I’ll save this to memory once the PR is merged.
| const { status, message, json } = await rawFetch('GET', '/v1/private/datasets', { | ||
| query, |
There was a problem hiding this comment.
Raw helpers ignore explicit workspace
rawFetch hard-codes Comet-Workspace to env.workspace instead of the selected workspaceName ?? env.workspace, so listDatasetSummaries and the other new helpers target the environment workspace and fail for clients configured with another workspace — should we capture the normalized workspace in makeBackendClient and reuse it for raw headers?
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/core/backend/client.ts around lines 894-895, update
`listDatasetSummaries` and the other new rawFetch-based dataset and automation helpers
so requests use the explicitly selected `workspaceName` when provided, falling back to
`env.workspace` otherwise. Capture this normalized workspace once inside
`makeBackendClient` and pass it through to the raw request header construction instead
of relying on the hard-coded `env.workspace`, ensuring all reads and creates target the
same workspace as the typed client.
There was a problem hiding this comment.
Real, but pre-existing and not reachable from this PR. rawFetch and workspaceHeaders are untouched here — the same hardcoded env.workspace is already on main in three places — and the new helpers are reached through base.fixture.ts's makeBackendClient(envConfig.apiKey), which passes no workspace, so the fallback and the explicit value coincide. The callers that do pass an explicit workspace are the workspace-role paths, which never touch these helpers. Worth fixing across the client rather than in these helpers alone; tracking it with the fixture item.
There was a problem hiding this comment.
Thanks for clarifying the reachability and broader tracking plan. I’ll save this context to memory once the PR is merged.
| # get_or_create - get_test_suite(), falling back to create (the default, | ||
| # and what every other route uses) | ||
| # list - get_test_suites(), selecting the suite by name | ||
| resolve_via: Literal["get_or_create", "list"] = "get_or_create" |
There was a problem hiding this comment.
Missing 404 contract for suite resolution
The resolve_via="list" field omits that, according to the PR description, a missing suite returns 404 without falling back to creation, so callers may assume both modes create it — should we document this behavior in the field or generated schema description?
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 283-286, update the `TestSuiteInsertItemsRequest.resolve_via` documentation to
state that `list` selects by name, returns 404 when the suite is missing, and
deliberately does not fall back to creating it. Expose this behavior through the
Pydantic field/schema description, not only a source comment, so generated API
documentation accurately distinguishes it from `get_or_create`.
There was a problem hiding this comment.
Fair that a source comment misses generated docs, but I am leaving it as a comment: Field( appears zero times in the whole opik-sdk-driver package, so adding one here would make this the only field in the driver carrying schema metadata — the same "change it everywhere or nowhere" reasoning that applies to the validation suggestions on this PR. If the driver's schemas grow descriptions generally, this field should get one in that pass.
There was a problem hiding this comment.
Thanks, that consistency rationale makes sense. I’ll save this to memory once the PR is merged.
| if body.resolve_via == "list": | ||
| suites = client.get_test_suites(project_name=body.project_name) |
There was a problem hiding this comment.
List path overreports inserted items
The resolve_via == "list" path derives inserted from len(body.items) even though suite.insert() deduplicates, so duplicate-only requests report inserted: 1 after writing zero items and mixed requests overcount — should we use the insertion funnel’s accepted count or calculate the deduplicated count, then assert the response count?
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/routes/test_suites.py
around lines 78-79, update `insert_test_suite_items` so the `inserted` response reflects
the items actually accepted by `suite.insert`, not the raw `len(body.items)`. Use the
insertion funnel’s return value if available, or calculate the
content-hash-deduplicated count before building the response, and add assertions
covering duplicate-only and mixed list-path requests.
There was a problem hiding this comment.
inserted=len(body.items) is verbatim on main and unchanged by this PR, so it is not a regression introduced here. More to the point, the spec deliberately never reads inserted — it asserts the exact set of items the suite ends up holding, which catches both "wrote a duplicate" and "wrote nothing at all"; a truthful count would be a weaker assertion than the one in place. Making the driver report the accepted count is still worth doing driver-wide, and nothing in this spec depends on it either way.
There was a problem hiding this comment.
Thanks for the clarification—you're right that this is pre-existing and the exact-item assertion is stronger. I'll save this context to memory once the PR is merged.
AddEditRuleDialog calls setOpen(false) outside the mutation's callbacks, so the dialog hides as soon as the PATCH is dispatched. waitForRequest plus submitRuleDialog's hidden-dialog wait was therefore no barrier at all: the persisted re-read could observe the pre-save blob and pass on a backend that had dropped custom_parameters — the exact regression the spec exists to catch. Arm both listeners before the click, then assert response.ok() before reading the rule back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔎 Follow-up review — bot comments triagedThe earlier automated review reported
Finding 1 was worse than reported. Correction to the earlier review's
|
2 participants
Where these came from
The QA release side flow explored the 2.2.48 → 2.2.49 range by hand on
staging (
staging.dev.comet.com/opik, workspaceopik-testing, backendconfirmed at
{"version":"2.2.49"}). Three of the flows a human verified thereare worth a permanent test; this PR is those three.
These specs were written and run against the
2.2.49tag, which is the refthe exploration verified.
mainis only where they have to merge, and it willhave moved on — please re-run them on
mainbefore promoting this out of draft.This PR was generated by the release QA side flow and needs human review
before merge. It is deliberately a draft.
What's here
1.
online-evaluation/online-evaluation-edit-rule-preserves-model-parameters.spec.ts@cap:online-evaluation.edit-rule(wascovered: false; thearea's only rule specs create fresh rules, none ever re-saved one).
code.model.custom_parameters = {thinking: {...}, unrelated_marker: "keep-me"}is opened in the edit dialog and saved with nothing changed. Both the
outbound
PATCH /v1/private/automations/evaluators/{id}body and there-read persisted blob must still carry both keys.
values it hydrated, so reading it separates "the frontend dropped it" from
"the backend dropped it". The persisted read alone cannot.
thinkingsurviving alone would also be satisfied bya serializer that special-cases it while discarding the rest of a map that is
documented as free-form.
unconditional strip that silently disabled extended thinking on a save where
the user changed nothing. Nothing about it is visible on screen, which is why
the assertions are on the payload rather than the page.
2.
datasets/dataset-list-summary-columns.spec.ts(two tests)@cap:datasets.list-datasets(alreadycovered: trueat t1;the t1 spec asserts only that a row is visible, and
DatasetsPagehad nocolumn readers at all — this adds the t2 substance).
(items, experiments, optimizations, versions) tuples are pairwise distinct
— (3,2,0,1), (5,0,1,1), (0,0,0,0), (5,1,2,3) — so a summary attached to the
wrong row disagrees with something:
membership, not a
find()of our own); each row'sdataset_items_count,experiment_count,optimization_countand recency timestamps match thatdataset's own shape and its own detail read;
latest_version.version_hashmatches the dataset's actual latest version; the empty dataset reports
zeros and a
nulllatest_versionrather than erroring; and the same readat
size=2across two pages returns identical rows.menu, each row's Item count matches, and the two recency columns are
populated on exactly the datasets that have an experiment / an optimization
and empty (
-) on the ones that do not.Mono.zipand swappedcomputeIfAbsentforgetOrDefault. A summary attached to the wrong row issilent wrongness on a page people read daily, not an API error.
experiment_count,optimization_countandlatest_versionhave no column on the list (
DEFAULT_COLUMNSinDatasetListPage.tsxoffers only the item count and the two recency stamps), so they are asserted
API-side only. Also worth knowing:
experiment_countcounts experiments thathave experiment items, not experiment rows — the fixture links items for
that reason.
3.
test-suites/test-suite-insert-dedup-listed-suite.spec.ts@cap:test-suites.list-suites.get_test_suites()— the listingfactory, which nothing else in this estate touches; every other spec and
fixture goes through
get_or_create_test_suite— deduplicates an insert ofan item it already holds, while a genuinely new item still lands. The
assertion is on the exact set of items the suite ends up holding, so both
"wrote a duplicate" and "wrote nothing at all" fail.
every later run then evaluates.
suite_dataset.__internal_api__hashes_synced__ = Falseinsdks/python/.../dataset/rest_operations.pymade this spec fail with"an item the suite already holds must not be written a second time"; the
revert was undone before committing. So the spec is known to discriminate,
not merely to be green.
Supporting changes
core/backend/client.ts—createLlmJudgeAutomationRule/getLlmJudgeModel(the existing
createAutomationRuleonly builds the python-metric shape, andnothing read the
code.modelblock);listDatasetSummaries/getDatasetSummary(the pinned SDK's dataset shape does not surfacelatest_version);versionHashonDatasetVersionRef.pom/online-evaluation.page.ts—openEditRuleDialogByName+submitRuleDialog, extracted out of the existingsetRuleEnabledByName,which now calls them.
pom/datasets.page.ts—datasetCell/datasetCellText, addressed bydata-cell-id="<rowId>_<columnId>"rather than by position (column order isuser-configurable and persisted), and
setColumnEnabledfor the Columns menu.fixtures/summarised-datasets.fixture.ts— the four-dataset seed, withteardown for the experiments, optimizations, traces and datasets it creates
(none of which cascade with the project, and traces are not in the run-prefix
sweep at all).
services/opik-sdk-driver—resolve_via: "get_or_create" | "list"onPOST /test-suites/insert-items, so a spec can name which factory built thesuite it inserts into. Defaults to the existing behaviour; the
listbranch404s rather than falling back, so a caller asking for the listing path cannot
silently be given another one.
coverage/taxonomy.yaml— the three specs added to their areas'specs:lists, and
online-evaluation.edit-ruleflipped tocovered: true, tier: t2-cujwith a note scoping it to the unedited-save path.What I deliberately did not write
datasets.sdk-round-trip— datasetinsert()argument validation(
num_threadsof0 / -1 / 1.5 / True / "4" / None,deduplication="yes").Verified on staging, but graded weak and dropped: a regression here fails
loudly with a
ValueErrorrather than silently, anddataset-version-counters.spec.tsalready drives the happy path atnum_threads=8. It buys a narrow error-shape assertion, not new behaviouralcoverage. Three stronger candidates existed, so the rule "skip
weakunlessit is all you have" applied.
fact: the
opik-testingstaging workspace has no Gemini/Vertex provider key,so no Gemini model is selectable in any picker and the thinking control never
renders. Spec 1 covers the Anthropic half — the actual regression — and
deliberately does not claim the Gemini level-picker round-trip.
one minute on shared staging, and if the bucket turns out to be
workspace-scoped it would throttle other users of
opik-testing.re-inserting a held item is a no-op; the taxonomy has no key naming that, so
it is tagged
test-suites.list-suites(the path it drives, alreadycovered: true) with a note, rather than a stretched claim or an inventedkey. Worth adding a dedicated capability if anyone extends this area.
How they were run
From
tests_end_to_end/e2e/, againstOPIK_BASE_URL=https://staging.dev.comet.com/opik,workspace
opik-testing, on the2.2.49tree:Plus, because two shared POMs were touched:
One thing a reviewer should know about
tsc:npx tsc --noEmitdoes notrun at this ref.
tests_end_to_end/e2e/package.jsonpins"typescript": "^7.0.2", and TypeScript 7 removedbaseUrl, whichtsconfig.jsonstill sets— so the check fails with
TS5102before compiling anything. This ispre-existing and unrelated to this PR. I typechecked with an override config
that replaces
baseUrlwith an equivalentpathsmapping; the only errors area pre-existing duplicate
deleteDashboardincore/backend/client.ts(lines 552 and 1044 on
2.2.49). Both are worth a separate fix.