[OPIK-8023] [QA] Proposed e2e spec from the opik#7935 exploration: compare-grid sort by evaluation-task JSON key - #7938
Conversation
…N key Adds tests_end_to_end/e2e/tests/experiments/experiments-compare-json-sort.spec.ts, covering the branch #7935 rewrote: sorting the experiments-comparison grid by a key inside the evaluation task's output JSON, including a key containing a quote or a backslash. The estate only ever sorted this grid by feedback_scores_<metric>, so nothing today reads an output.* sort. Both orders are asserted on the rendered rows and on the server read behind them. Supporting changes: - fixtures/json-sortable-comparison.fixture.ts seeds four shared items and two experiments whose task outputs carry three JSON keys, each inducing a different ordering. - backendClient.listComparedDatasetItems reads the compare grid's endpoint with an explicit sorting clause. - CompareExperimentsPage gains a header-click sort, a sort reset, and output cell/column lookups keyed by JSON key. Generated by the release QA side flow; needs review before merge.
github-actions
Bot
added
tests
📋 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 |
⏱️ pre-commit per-hook timingNo linted files changed — nothing to run. ⏭️ 43 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const cssAttributeValue = (value: string): string => | ||
| value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); |
There was a problem hiding this comment.
Control-character keys break output lookups
cssAttributeValue leaves CSS control characters such as \n and \r unescaped, so a backend-returned ExperimentOutputColumn.name containing one produces an invalid selector in outputColumnHeader()/splitBand() and causes sortByOutputKey and readItemOutput to throw — should we use a complete CSS string-escaping routine or a trusted locator API?
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/pom/compare-experiments.page.ts` around lines 15-16, update
`cssAttributeValue`, which is used by `outputColumnHeader` and `splitBand`, so
unconstrained JSON keys cannot produce invalid CSS selectors. Replace the partial
quote/backslash escaping with a complete CSS string escaping routine that handles
newlines, carriage returns, all C0 control characters, quotes, and backslashes, or
refactor to use a trusted locator API that avoids manual CSS interpolation.
| * query param instead. Nothing in the header carries a `data-testid`; the | ||
| * shared `DataTable` stamps only `data-header-id`, and the environments these | ||
| * specs run against serve a prebuilt frontend image, so adding one has to be | ||
| * a separate frontend change. | ||
| */ | ||
| async sortByOutputKey(outputKey: string): Promise<void> { | ||
| await test.step(`click the "${outputKey}" output column header to sort`, async () => { | ||
| const header = this.outputColumnHeader(outputKey); | ||
| await expect(header, `output column header for "${outputKey}"`).toHaveCount(1); | ||
| await header.locator('span.truncate').click(); | ||
| await this.itemRows.first().waitFor({ state: 'visible' }); | ||
| }); |
There was a problem hiding this comment.
Brittle output-header selector violates E2E rules
The POM relies on th[data-header-id=...] and span.truncate for the shared header, so harmless DataTable/TypeHeader markup changes can break the spec; since these sources are in this repository, should we add a descriptive kebab-case data-testid at the shared/page header boundary and select it with getByTestId? .agents/skills/writing-e2e-tests/SKILL.md and .agents/skills/playwright-pom-discovery/SKILL.md require this pairing, so if the frontend cannot be changed, could we document and enforce a real source/deployment blocker instead of deferring it because of the prebuilt test image?
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/pom/compare-experiments.page.ts around lines 235-246, refactor
`sortByOutputKey` and its `outputColumnHeader` locator to use a descriptive kebab-case
`data-testid` with `getByTestId` instead of `th[data-header-id]` and `span.truncate`. In
the shared `DataTable`/`TypeHeader` frontend source that owns these headers, add the
matching output-header test ID in the same change; only retain the CSS fallback if you
document and enforce a genuine source or deployment blocker.
| await test.step('Both awkward keys render as their own grid columns', async () => { | ||
| await compare.gotoResults(); | ||
| await compare.waitForResultsReady(); | ||
| await compare.clearSort(); | ||
| for (const key of [QUOTED_OUTPUT_KEY, BACKSLASHED_OUTPUT_KEY]) { | ||
| await expect(compare.outputColumnHeader(key), `output column for key "${key}"`).toHaveCount(1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Backslash UI sorting remains untested
c\d is covered only through direct sortedItemIds() calls, so a broken sortByOutputKey() selector, click/sort binding, or rendered value can pass while the test claims the backslashed key sorts — should we click BACKSLASHED_OUTPUT_KEY in both directions and assert row order and rendered cells for each compared experiment?
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/experiments/experiments-compare-json-sort.spec.ts around
lines 178-214, update the awkward-key sorting test so `BACKSLASHED_OUTPUT_KEY` is
exercised through the browser, not only `sortedItemIds()`. Click its header twice and
assert descending and ascending row orders, then verify its rendered output cells for
every compared experiment, matching the existing plain-key coverage; retain the backend
assertions as additional validation.
| @@ -1,4 +1,4 @@ | |||
| export { test, expect } from './evaluated-thread.fixture'; | |||
| export { test, expect } from './json-sortable-comparison.fixture'; | |||
There was a problem hiding this comment.
Shared test entrypoint gains feature coupling
fixtures/index.ts now re-exports test from json-sortable-comparison.fixture, so every generic test import transitively depends on comparison-specific fixture setup — should we source the shared export from the generic fixture base and expose the specialized test through a dedicated module?
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/index.ts` around lines 1-1, change the shared `test` and
`expect` export so it comes from the generic `evaluated-thread.fixture` rather than the
JSON-sortable comparison fixture. Keep the JSON-sortable types and constants exported as
needed, but expose that fixture’s specialized `test` through its own narrowly consumed
module or import path so generic consumers do not depend on comparison-specific setup.
| export interface ComparedDatasetItemRef { | ||
| id: string; | ||
| /** | ||
| * Evaluation-task output JSON keyed by experiment id, or null where the | ||
| * experiment recorded no output for this item — which is a real answer, not | ||
| * an empty one. | ||
| */ | ||
| outputByExperimentId: Record<string, Record<string, unknown> | null>; |
There was a problem hiding this comment.
Compared-row output contract drops valid JSON shapes
DatasetItemResultMapper.getJsonNodeOrNull() leaves ExperimentItem.output as a broad JsonNode, and listComparedDatasetItems() only asserts it to the narrower outputByExperimentId type, so valid scalar and array responses pass through with an unrepresentable runtime shape — should we type this field as JsonListStringCompare | null (or an equivalent local union) and preserve undefined as null?
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 47-54 and 445-451, update
`ComparedDatasetItemRef.outputByExperimentId` and the `listComparedDatasetItems` mapping
so they accurately represent the backend’s `JsonListString` values. Replace the
object-only type assertion with `JsonListStringCompare | null` or an equivalent local
union that includes objects, object arrays, and strings, while continuing to convert
`undefined` outputs to `null`.
| const dataset = await sdkClient.python.createDataset({ | ||
| project_name: project.name, | ||
| name: datasetName, | ||
| description: 'evaluation-task outputs with sortable JSON keys', | ||
| items: SEED_ITEMS.map(({ input, expected_output }) => ({ input, expected_output })), | ||
| }); | ||
|
|
||
| const stored = await backendClient.getDatasetItems(dataset.id); |
There was a problem hiding this comment.
Setup failure leaves comparison artifacts
A rejection from createDataset, getDatasetItems, trace creation, createExperiment, or createExperimentItems before await use(ref) skips fixture cleanup, so partial datasets, traces, or experiments survive until best-effort global teardown and may remain if the runner exits early — should we wrap setup and teardown in try/finally, record the experiment ID on creation, and delete resources in dependency order?
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/json-sortable-comparison.fixture.ts` around lines 119-126
and the fixture setup/teardown through lines 202-217, refactor `jsonSortableComparison`
to wrap dataset creation, experiment setup, and `await use(ref)` in a `try/finally`.
Track each experiment ID immediately when it is generated, including the currently
incomplete experiment, and make the `finally` cleanup delete all partially created
experiments before deleting the dataset, while preserving `shouldLeaveArtifacts`
behavior and safe best-effort deletion.
| await use(ref); | ||
|
|
||
| if (!shouldLeaveArtifacts(testInfo)) { | ||
| const safe = async (what: string, fn: () => Promise<unknown>): Promise<void> => { | ||
| try { | ||
| await fn(); |
There was a problem hiding this comment.
Failure artifacts are deleted anyway
global-teardown.ts unconditionally deletes current-run cuj-${runId}- experiments and datasets even when OPIK_LEAVE_FAILURES=true, so the fixture's shouldLeaveArtifacts(testInfo) retention path cannot preserve them for post-failure inspection — should we make the run-end sweep honor the same retention flag?
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/json-sortable-comparison.fixture.ts` around lines
200-205, preserve the fixture’s failure-retention behavior and update
`global-teardown.ts` so its current-run experiment and dataset cleanup honors the same
`OPIK_LEAVE_FAILURES`/`shouldLeaveArtifacts` flag. When retention is enabled, skip
deleting `cuj-${runId}-` entities—including the `*-jsonsort-*` resources—while
retaining the existing cleanup behavior otherwise; do not limit the flag to scratch
files only.
1 participant
Where these came from
Exploratory testing of opik#7935 — OPIK-8023 Bind dataset-item JSON sort keys as query parameters — on that PR's own deployed environment (
https://pr-7935.dev.comet.com,2.2.35-7935-merge-3028, OSS install, workspacedefault). A human worked the flows by hand there first; this PR turns the two strongest verified candidates into one permanent spec.#7935 is merged (squash commit
7627ddc, 2026-08-20) and its head branch has been deleted, so this targetsmain. The specs were written against the PR's head commit1272150dbe2df35293f13f9ae5f7fd239da1b701and then re-verified onmain— see "Verification" below.tests_end_to_end/andapps/opik-frontend/are byte-identical between that commit andmain, andmaincarries exactly one further commit (a version bump), so the re-run exercised the same estate and the same frontend.What it covers
Sorting the experiments-comparison grid by an "Evaluation task (last trial)" column — a key inside the evaluation task's output JSON. That is the branch #7935 rewrote, and the estate does not drive it today:
experiments-compare.spec.ts @cap:experiments.compare-sort-searchonly ever sortsfeedback_scores_<metric>, so nothing would have noticed if the new:sorting_parambinding foroutput.*ordered rows wrongly.Both the old and the new failure modes are silent in the way that makes a permanent test worth having: a grid ordered by the wrong expression still renders a full, healthy-looking table.
Per spec
tests_end_to_end/e2e/tests/experiments/experiments-compare-json-sort.spec.ts—@t2-cuj,@area:experiments, both tests@cap:experiments.compare-sort-search.outputcolumn header sorts rows DESC and a second click flips to ASC; the rendered output cell of every row, for both compared experiments, carries the seeded value; and the server read behind the grid returns the same order foroutput.outputASC and DESC, with the full row count.a'bandc\deach render as their own grid column; clicking thea'bheader sorts DESC then ASC; its cells carry the seeded values; and the server sorts both awkward keys in both directions, returning the full collection rather than a 500. Pre-#7935 the key was interpolated intoJSONExtractRaw(%s, '%s'), soa'bproduced malformed SQL.Design notes, briefly:
argMax(..., created_at)to sort on, and identical values make that pick irrelevant to the expected order instead of a race.Supporting changes
fixtures/json-sortable-comparison.fixture.ts— new fixture (dataset + traces via the SDK bridge, experiment linking via the backend client, the same splitaged-experiment.fixture.tsuses). The bridge'scompare-seedroute writes one fixedoutputkey per item and this needs three keys per item, so it could not be reused. Teardown lives in the fixture, honoursshouldLeaveArtifacts, and deletes experiments before the dataset.core/backend/client.ts—listComparedDatasetItems(), the compare grid's server read with an explicitsortingclause, via the typed public SDK (opik.api.datasets.findDatasetItemsWithExperimentItems). Returnstotal: number | nullrather than defaulting to0so a caller can tell "the server said none" from "the server said nothing".pom/compare-experiments.page.ts— header-click sort, sort reset, and output cell/column lookups keyed by JSON key. Cells and headers are addressed bydata-cell-id/data-header-id, with a CSS-attribute escaper because output keys are user data and routinely contain quotes and backslashes.coverage/taxonomy.yaml— spec added to theexperimentsarea'sspecs:list.compare-sort-searchwas alreadycovered: true, tier: t2-cuj; anote:now records which spec covers which branch of it.Verification
Run against the exploration's environment (
OPIK_BASE_URL=https://pr-7935.dev.comet.com,OPIK_DEPLOYMENT=oss, workspacedefault), fromtests_end_to_end/e2e/:main.tests/experiments/directory was run because this change touches a shared POM, the fixtures index and the backend client: all 8 tests pass, so no sibling breakage.Two caveats a reviewer should have:
pr-7935deployment on both runs — it is the only environment this job has. What the second run re-verified is the spec againstmain's estate code; the frontend and test estate are identical between the two refs, and the backend fix is inmain, but this is not a run against amaindeployment.*.dev.comet.comhostname, which makes the TypeScript SDK demand an API key it does not need (new URL(apiUrl).hostname.endsWith("comet.com")). Runs there need a dummyOPIK_API_KEYset ormakeBackendClientthrows in global setup. Not caused by this change, and it does not affect a localhost or a real cloud target — flagging it because the next person pointing the suite at a PR preview will hit it.What I deliberately did not write
One candidate was dropped:
dataset_version_id, which routes the same endpoint throughDatasetItemVersionDAO'stop_sortingCTE). The exploration marked it weak and verified it worked, and it is arguably the branch [OPIK-8023] Bind dataset-item JSON sort keys as query parameters #7935 most plausibly fixes — by code inspection the oldhasDynamicKeys(sorting, fieldMapping)filter left an unbound:sorting_param_nullon that path. It was dropped anyway because it duplicates this spec's API assertions for a second code path, this PR's ownDatasetVersionResourceTest.sortByJsonKeyThroughPushTopLimitalready covers it at the resource level, andbackendClient.createExperimenthas nodataset_version_idtoday — covering it would have meant widening the shared client for an assertion a backend test already makes. If a reviewer disagrees, that is the one to add; the seed shape here would carry it with one more experiment pair.The candidate list was filtered, not exhausted: 2 candidates in, 1 spec file with 2 tests out, 1 dropped.
Placement note
The exploration suggested adding these as extra tests inside
experiments-compare.spec.ts. They are in their own file instead, for the reasonexperiment-logs-date-window.spec.tsgives for the same situation — a distinct failure mode of an already-covered capability, kept separable — and because a new file makes thespecs:entry in the taxonomy a real edit rather than a no-op. The capability tag is unchanged and no new capability was invented, which was the substance of the exploration's advice.🤖 Generated by the release QA side flow (
release-test-proposal). This is a draft and needs human review before merge — an unreviewed generated spec that asserts the wrong thing is worse than no spec, because it will be trusted.