[OPIK-8125] [QA] Proposed e2e spec from the #8031 exploration: untruncated experiment-comparison export - #8036
Conversation
…ison export Covers experiments.export-comparison, which was uncovered: exporting the selected rows of the experiment-comparison grid must download the whole value, not the truncated copy the grid renders (opik#8031). The defect this locks is silent — the grid looks identical either way, because it is supposed to show truncated text, and the only place a truncating export shows up is inside a downloaded file, on the path people take precisely when they want the data whole. Two limits are exercised, because they are separate mechanisms and the fix has to hold for both: the experiment output is slimmed at 1 000 characters with a "..." suffix, and the dataset item's data map is cut at 10 001 with no suffix at all. - fixtures/long-value-experiment.fixture.ts: one experiment over three dataset items whose values straddle both limits, seeded through the SDK bridge and the REST client, torn down after use(). - core/backend: listCompareItems(), the compare-items read with `truncate` under the caller's control, so the spec can prove the truncated read really does cut these values before it opens a browser. - pom/compare-experiments.page.ts: row selection, cell text by row+column id, and Export → CSV/JSON with the download captured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. ⏭️ 44 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| * The trigger is an icon-only button with no accessible name and no | ||
| * `data-testid`, so it is addressed by the icon it renders — its identity, not | ||
| * its position. A `data-testid` on `ExportToButton` would be the better handle | ||
| * and is worth adding; it is not added here because this spec has to run | ||
| * against already-built deployments, where a fresh front-end attribute does | ||
| * not exist yet. `toHaveCount(1)` guards against the locator widening. |
There was a problem hiding this comment.
Brittle export selector violates E2E contract
ExportToButton is located via structural CSS and svg.lucide-download, so icon, class, or menu-structure changes can break the export test while behavior still works — should we add a descriptive kebab-case data-testid to its trigger and use page.getByTestId(...), as .agents/skills/writing-e2e-tests/conventions.md and .agents/skills/playwright-pom-discovery/SKILL.md require?
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 240-245, update the
exportAs method to stop locating the Export menu trigger with structural CSS and
svg.lucide-download. Add a descriptive kebab-case data-testid to the ExportToButton
trigger in its shared component, then locate it with page.getByTestId(...) while
retaining the uniqueness and enabled-state assertions. Update the surrounding comment to
document the stable test ID instead of explaining why one is unavailable.
| export const SLIM_STRING_MAX_LENGTH = 1_000; | ||
| export const SLIM_TRUNCATION_SUFFIX = '...'; | ||
| export const DATASET_TRUNCATION_SIZE = 10_001; |
There was a problem hiding this comment.
Non-default deployments fail truncation spec
DATASET_TRUNCATION_SIZE is assumed to be fixed, but config.yml defaults responseFormatting.truncationSize from OPIK_RESPONSE_TRUNCATION_CHAR_LIMIT, and DatasetItemVersionDAO.getItemsWithExperimentItems uses that runtime value for truncate=true while the e2e loader neither sets nor discovers it, so mismatched deployments seed artifacts before compare-export-truncation.spec.ts fails — should we resolve the active limit or make it explicit and validate it before seeding?
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/long-value-experiment.fixture.ts around lines 19-21,
update the `DATASET_TRUNCATION_SIZE` and seed-validation logic so it does not assume the
backend limit is always 10,001. Make the fixture receive or discover the active
`responseFormatting.truncationSize`/`OPIK_RESPONSE_TRUNCATION_CHAR_LIMIT`, or require an
explicit test configuration, and validate that value before creating any artifacts.
Ensure the generated seed values and downstream assertions use the validated runtime
threshold and fail with a clear configuration-mismatch error.
| const dataset = await sdkClient.python.createDataset({ | ||
| project_name: project.name, | ||
| name: datasetName, | ||
| description: 'Export of experiment comparison rows must not truncate', | ||
| items: SEED.map((row) => ({ | ||
| label: row.label, | ||
| input: row.input, | ||
| expected_output: row.label, | ||
| })) as unknown as Array<Record<string, unknown>>, | ||
| }); | ||
|
|
||
| // Read the ids back rather than generating them, and key them by the label |
There was a problem hiding this comment.
Fixture setup leaks large test artifacts
A rejection from getDatasetItems or the creation helpers (createTraceWithSource, createExperiment, or createExperimentItems) occurs before await use(ref), so teardown is skipped and successfully created entities remain behind — should we register rollback actions after each creation or use an independent cleanup fixture while preserving the experiment → dataset → traces 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/long-value-experiment.fixture.ts around lines 164-175,
update the `longValueExperiment` fixture setup so failures in dataset reads, trace
creation, experiment creation, or experiment-item linking cannot leak already-created
backend entities before `await use(ref)`. Register rollback actions immediately after
each successful creation, or move cleanup into an independently running teardown
fixture, and ensure partial trace creation is handled. Preserve the existing ordered
cleanup of experiment, then dataset, then traces.
| 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.
Fixture setup leaks partial test artifacts
A failure in getDatasetItems, createTraceWithSource, createExperiment, createExperimentItems, or a later attachment exits before cleanup, so earlier entities remain in the workspace. Could we register each successful entity immediately and perform best-effort reverse-order deletion from finally/fixture teardown while retaining shouldLeaveArtifacts(testInfo), as .agents/skills/writing-e2e-tests/conventions.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/fixtures/long-value-experiment.fixture.ts` around lines 258-263,
refactor the `longValueExperiment` fixture lifecycle so cleanup cannot be skipped when
`getDatasetItems`, trace/experiment creation, experiment-item creation, or attachment
setup throws. Register each successfully created entity immediately and execute
best-effort deletion in reverse dependency order from a `finally`-based teardown path,
while retaining the `shouldLeaveArtifacts(testInfo)` guard so preserved-artifact runs
remain unchanged.
1 participant
Generated by the release QA side flow (test-proposal step). It is a draft and needs human review before merge — nothing here has been reviewed by a person.
Where this came from
Exploratory testing of opik#8031 (
[OPIK-8125] [FE] fix: export experiment items without truncation). A human-driven exploration verified the flow end to end on that PR's own deployed environment (pr-8031.dev.comet.com,2.2.42-8031-merge-3115, OSS install, workspacedefault) before any spec was written. This PR turns the one strong candidate from that exploration into a permanent spec.Base note. The task I was given said #8031 was still open with its head branch alive, and told me to target
jacques/OPIK-8125-export-truncationin that case. That snapshot was stale: #8031 merged at 2026-08-27T09:38:47Z asbfa4ced0ef7217c5f98af301c369700b8efd22d1, and the head branch has since been deleted (git ls-remote --heads originno longer lists it). So this targetsmain, where the fix already lives. The spec was written against the PR's head commit96dec07ec2335de21a0516aa32f9553cdd6e776dand then rebased onto currentmain(472c30818) before being run again — see "Verification" for exactly what ran where.What the spec covers
tests_end_to_end/e2e/tests/experiments/compare-export-truncation.spec.ts—@t2-cuj,@area:experiments,@cap:experiments.export-comparison(previouslycovered: false).One test: exporting the selected rows writes the whole value the grid shows truncated.
The change under test is a single React Query param — the export refetch now passes
truncate: falseinstead of inheriting the table's flag — so the only way to see it is to press Export and read the file. What makes it worth a permanent test is that it is silent: the grid renders identically in the broken and the fixed build (it is supposed to show truncated text), nothing errors, nothing is missing, and a 1 000-character answer is long enough to look complete. The defect only exists inside a downloaded file, on the path people take precisely when they want the data whole.What it asserts, in order:
truncate=falsecome back byte-identical to the seed, and read withtruncate=truecome back cut. Without this a green export assertion would prove nothing — a seed that fell under both limits round-trips whole through a broken build too.dataset.inputandoutput.outputare string-equal to the seeded values, with no...suffix anywhere and the unselected row absent.truncate=false, rather than reusing the grid's truncated one — the regression asserted at its cause as well as its symptom, following the patternexperiment-logs-date-window.spec.tsalready uses in this area.Two truncation limits, not one. They are separate mechanisms and the fix has to hold for both, so the fixture seeds both:
output.output)TruncationUtils.DEFAULT_SLIM_STRING_MAX_LENGTH= 1 000...(so a cut value is 1 003 chars)datamap (dataset.input)responseFormatting.truncationSize= 10 001A 2 000-character dataset input is not truncated, so a spec that only seeded that would have passed against the pre-fix build on the dataset axis. The
longrow is deliberately kept under the dataset limit to act as that control.experiments-compare.spec.tsdrives the same grid but seeds single-character outputs, so it would pass unchanged against a truncating export.Supporting changes
fixtures/long-value-experiment.fixture.ts(new) — one experiment over three dataset items straddling both limits. Seeded through the SDK bridge (dataset) and the REST client (traces + experiment linkage), the same shapejsonOutputExperimentuses and for the same reason. It asserts its own seed lengths against both limits before writing, so a filler that came up short fails loudly instead of leaving a spec that cannot fail. Teardown lives in the fixture (experiment → dataset → traces) and honoursshouldLeaveArtifacts. Chained betweenjsonOutputExperimentandgroupedDataset.core/backend—listCompareItems(), the compare-items read withtruncateunder the caller's control and the payloads kept. The existinglistCompareItemIds()hard-codestruncate: truebecause it only asserts order; here the payload is the answer.pom/compare-experiments.page.ts—selectItemRow(),readCellText()(by row id + column id, never by position — column order is user-configurable and persisted), andexportAs('CSV' | 'JSON'), which drives the toolbar menu and returns the captured download's contents.coverage/taxonomy.yaml— spec added to theexperimentsspecs:list;export-comparisonflipped tocovered: true, tier: t2-cuj.Verification
Run from
tests_end_to_end/e2e/against the exploration's environment,OPIK_DEPLOYMENT=oss,OPIK_BASE_URL=https://pr-8031.dev.comet.com, workspacedefault.npx playwright test tests/experiments/compare-export-truncation.spec.tsnpx playwright test tests/experiments/python3 tests_end_to_end/coverage/tag_lint.py --taxonomy … --estate tests_end_to_end52 specs checked, 1 exempt, 0 problem(s)npx tsc --noEmitBoth Playwright runs above were executed after rebasing onto current
main, so the estate code under test ismain's.Two honest caveats about "against main":
472c30818([OPIK-8109] [BE] fix: bound dataset items page read by page size) — the one commit to land onmainsince. That change rewritesSELECT_DATASET_ITEM_VERSIONS, a different query path from the compare-items endpoint this spec reads, so it is not expected to interact; but I could not deploymainitself, so that is reasoning, not a run. The first CI run on this PR is the real check.npx tsc --noEmitas documented in.agents/skills/writing-e2e-tests/SKILL.mdcurrently fails onmainbefore reaching any source file:tsconfig.json(13,5): error TS5102: Option 'baseUrl' has been removed, from the pinnedtypescript@7.0.2. I got the 0-error result above by extending the repo tsconfig withpathsrewritten andtypes: ["node"], which typechecks the whole estate cleanly, including these files. This is pre-existing and not caused by this PR — it reproduces on unmodifiedmain— but it means the skill's own typecheck step is currently a no-op for everyone, and it is worth fixing separately.What I deliberately did not write
The exploration produced two candidates. One was written; one was dropped:
weakby the exploration). It is the tightest regression-lock available — the pre-fix export changed with the workspace preference, and the exploration confirmed the CSV and JSON download byte-identical (matching md5) with truncation on and off. It is dropped because driving it needs a workspace-level preference flip, which is shared state that this suite's per-test fixtures do not isolate: it would corrupt any spec running in parallel against the same workspace, and leave the workspace dirty if the test died mid-flip. It also largely duplicates the assertion the shipped spec already makes. Worth revisiting if the estate grows a safe way to scope a workspace preference to one test.Follow-up worth doing (not done here)
ExportToButtonrenders an icon-only trigger with no accessible name and nodata-testid, so the POM addresses it by the icon it renders, guarded withtoHaveCount(1). Adata-testidon that shared component is the better handle and conventions ask for one in the same change — I did not add it because this spec had to be run against an already-built deployment, where a brand-new front-end attribute does not exist, and an unrunnable spec is worse than a CSS selector. A reviewer adding that attribute and switching the POM togetByTestIdin a follow-up would be a straight improvement.Source PR: comet-ml/opik#8031 · OPIK-8125