test(frontend): point the acceptance suite at what the UI renders now - #6514
test(frontend): point the acceptance suite at what the UI renders now#6514ardaerzin wants to merge 4 commits into
Conversation
The antd->Radix migration removed the classes these tests select on, so the locators matched nothing and the tests timed out rather than failing with a reason. Six locators moved to role/testid/AVT hooks that survive the swap: - saveVariant waited on button.ant-btn-primary for Commit; that button is an EnhancedButton, now a facade over the Radix @agenta/ui Button. It also asserts visibility first, so a missing button reports itself instead of hanging isDisabled() for the full timeout. Five of the eleven failures run through this one helper. - the observability projection tabs are the shared Segmented (role=radiogroup of role=radio), not an antd Radio.Group with a -checked class. - the span glyph lost its antd Avatar; it gets a data-testid instead. - the testset and registry tables are the virtual table, whose stable hook is .avt-body (see AVT in tableDom.ts) and whose selection control is the Radix Checkbox.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughAcceptance tests now use role-based selectors, stable test IDs, and virtual-table hooks for the updated UI. The observability avatar component exposes the new test ID. ChangesUI locator migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to This change updates acceptance-test locators for the migrated UI and preserves compatibility with both table implementations. The functional selector issue is addressed; only minor repository comment-style cleanup remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/oss/tests/playwright/acceptance/testsset/testset-management.ts (1)
213-213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMigrate the remaining active table selectors.
Line 213 still queries
.ant-table-cell, and Line 348 still queries.ant-table-thead .anticon-plus. The same virtual table usesavt-cellandavt-thead. These selectors can fail before the new.avt-bodyassertion completes. Replace them with current AVT hooks or stable roles/test IDs.Also applies to: 348-348
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: d977c360-c809-47fd-9f79-6cf30417105c
📒 Files selected for processing (6)
web/oss/tests/playwright/acceptance/observability/index.tsweb/oss/tests/playwright/acceptance/playground/index.tsweb/oss/tests/playwright/acceptance/playground/tests.tsweb/oss/tests/playwright/acceptance/testsset/testset-management.tsweb/oss/tests/playwright/acceptance/use-api/index.tsweb/packages/agenta-observability-ui/src/cells/AvatarTreeContent.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
| // Each span in the tree renders a span-type glyph (AvatarTreeContent, a plain div | ||
| // since the antd Avatar was dropped). At least one confirms the tree has nodes. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep each new code comment to one short line.
The same hard-rule violation appears at every listed site. Compress each block to one short line. Keep the trace-control wording accurate and remove the stale Ant Design description.
web/oss/tests/playwright/acceptance/observability/index.ts#L318-L319: shorten the span-avatar comment.web/oss/tests/playwright/acceptance/observability/index.ts#L340-L342: shorten and correct the Segmented-control comment.web/oss/tests/playwright/acceptance/playground/tests.ts#L352-L354: shorten the Commit-locator comment.web/oss/tests/playwright/acceptance/playground/tests.ts#L356-L358: shorten the visibility comment.web/oss/tests/playwright/acceptance/testsset/testset-management.ts#L311-L312: shorten the virtual-table comment.web/oss/tests/playwright/acceptance/use-api/index.ts#L127-L129: shorten the checkbox-locator comment.
As per coding guidelines: At most ONE short line per comment.
📍 Affects 4 files
web/oss/tests/playwright/acceptance/observability/index.ts#L318-L319(this comment)web/oss/tests/playwright/acceptance/observability/index.ts#L340-L342web/oss/tests/playwright/acceptance/playground/tests.ts#L352-L354web/oss/tests/playwright/acceptance/playground/tests.ts#L356-L358web/oss/tests/playwright/acceptance/testsset/testset-management.ts#L311-L312web/oss/tests/playwright/acceptance/use-api/index.ts#L127-L129
Source: Coding guidelines
Railway Preview Environment
|
Verified locally — the fix is confirmed, not just arguedRan the playground spec against a local EE stack, on
The baseline reproduced CI's signature exactly: both Outcome per test:
No regressions, and the run is faster purely from not burning 60s hangs. Reproducing locally (three traps, worth adding to #6154)
|
CI on this branch cut failures 11 -> 8, and named the two that remained: - testset-management still waited on .ant-table-cell, a second stale selector in the same file the first fix did not cover; the virtual table stamps .avt-cell. - the use-api page-rendered check was my own bad replacement: the Variants control is not a role=radio. It now waits for the registry table the test actually goes on to use, so the check cannot drift with the chrome again.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: f9706e98-3b05-4598-881b-537da359d008
📒 Files selected for processing (2)
web/oss/tests/playwright/acceptance/testsset/testset-management.tsweb/oss/tests/playwright/acceptance/use-api/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/oss/tests/playwright/acceptance/testsset/testset-management.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| // Wait for the registry table itself — the thing this test goes on to use — | ||
| // rather than a tab control, so the check cannot drift with the chrome again. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the new comment to one short line. Condense the two-line note into one short // comment. As per coding guidelines: “At most ONE short line per comment.”
Source: Coding guidelines
The evaluation and annotation modals pick rows through a table whose selection control is now a Radix <button role="checkbox">, not an <input>. isSelected() looked for input[type=checkbox] and the antd classes, matched nothing, and fell through to a selected-tag lookup that also no longer applied — so it returned false however many rows were selected. The CLICK already worked (controlSelector carries [role="checkbox"]); only the verification was blind, which is why these failed on a 30s poll rather than on the click. Same one-line widening in both helpers.
Third pass — CI's own results drove theseCI on the previous commit cut the suite from 11 failed → 8, and named exactly what was still wrong. Two more commits: 1. The two locators CI proved wrong (
2. The selection check was blind to Radix (
'input[type="checkbox"], input[type="radio"], .ant-checkbox-input, .ant-radio-input'The control is now a Radix The click already worked ( Deliberately left alone: the other The two remaining failures are not selector driftWorth separating so they aren't chased as part of this:
Local verification noteThe testsets/use-api corrections could not be confirmed locally: against a Next dev server those specs fail earlier with |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 1be8b5e6-0f02-494c-a116-535f0e295a85
📒 Files selected for processing (2)
web/oss/tests/playwright/acceptance/auto-evaluation/tests.tsweb/oss/tests/playwright/acceptance/human-annotation/tests.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| // `[role]` included: the Radix checkbox/radio is a <button role="checkbox">, not an | ||
| // <input>, so the native+antd list alone matched nothing and selection read as false. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one short line for both new comments.
Rewrite each two-line explanation as one short line.
web/oss/tests/playwright/acceptance/auto-evaluation/tests.ts#L176-L177: collapse the explanation into one line.web/oss/tests/playwright/acceptance/human-annotation/tests.ts#L551-L552: collapse the explanation into one line.
As per coding guidelines, “Hard rule. At most ONE short line per comment.”
📍 Affects 2 files
web/oss/tests/playwright/acceptance/auto-evaluation/tests.ts#L176-L177(this comment)web/oss/tests/playwright/acceptance/human-annotation/tests.ts#L551-L552
Source: Coding guidelines
CI got the suite to 4 failures and named another stale one: the add-column click used .ant-table-thead, which no longer matches (the icon inside it is still antd, only the header moved). Rather than keep finding these one CI run at a time, every table selector now accepts both halves — .avt-*, .ant-table-* — which is the pattern DOM_SELECTOR in the package already uses, and for its stated reason: a host may still mount an antd table through the legacy adapter.
1 participant
Relates to #6154, #5616.
What's broken
tests / run-web-tests (acceptance)is red on every branch — 11 failed / 5 flaky / 33 passed on therelease/v0.114.7tip, and v0.114.7 shipped with it waived. The suite has been giving no signal, so a real frontend regression would land unseen.Why
The antd → Radix/shadcn migration removed the CSS classes these tests select on. The UI is fine; the locators match nothing — and a locator that matches nothing doesn't fail with a reason, it hangs until the timeout. That's the
12 × locator.isDisabled: Test timeout of 60000msand32 × element(s) not foundin the run.Note this is not the failure #6154 describes. That issue's signature (
navigateToModelswaiting on anLLMsheading, 28×/run) no longer occurs at all — zero occurrences in today's run. The suite moved on to a different failure mode.The fixes
Six locators, each pointed at a hook that survives the swap:
playground/tests.tssaveVariantbutton.ant-btn-primary+ "Commit"getByRole("button", {name: "Commit"})playground/index.tsobservabilityprojection tabs.ant-radio-button-wrapper+-checkedclassrole=radiogroup/role=radio+toBeChecked()observabilityspan glyph.ant-avatar-squaredata-testid="span-type-avatar"testset-management.ts.ant-table-tbody.avt-bodyuse-apirow selection.ant-table-tbody .ant-checkbox-input.avt-body+role=checkboxuse-apivariants tab.ant-radio-button-wrapperrole=radiosaveVariantis the high-leverage one: the two playground failures and all three observability failures run through it, so five of the eleven are one bug.It also asserts the Commit button is visible before calling
isDisabled(). Without that, a missing button burns the full 60s and reports a timeout instead of naming what it couldn't find.The
.avt-bodyhook isn't invented here —@agenta/ui'stableDom.tsalready definesAVTfor exactly this, documenting that the.ant-table-*half "matches nothing here".One product change:
AvatarTreeContentgains adata-testid. It's a plaindivsince the antd Avatar was dropped, so it offered nothing stable to select;getByTestIdis already the convention in this suite.Testing
Not verified by a run — I can't reproduce the suite locally, so CI on this PR is the check. Each locator was derived from what the component actually renders today (
Segmented→role=radiogroup, RadixCheckbox→role=checkbox,AVT.bodystamped atVirtualTable.tsx:562) rather than guessed.tscand lint are clean.Expect this to reduce the failures, not zero them. Untouched: the
agent-chat,auto-evaluationandhuman-annotationfailures, and the 5 flaky tests — which are the fixture flows (app and prompt creation), a separate problem tracked in #5977 and #5695. The run also showed Playwright's browser install retrying twice, which is environmental.Real product bugs may be hiding behind these stale selectors; you can't see past a locator that never resolves.