Enhance E2E testing with new UI journey specs and mock backend#1015
Open
Devin-Yue wants to merge 18 commits into
Open
Enhance E2E testing with new UI journey specs and mock backend#1015Devin-Yue wants to merge 18 commits into
Devin-Yue wants to merge 18 commits into
Conversation
…pstream deletion) Co-authored-by: Cursor <cursoragent@cursor.com>
Start a minimal localhost:9999 HTTP stub during Playwright global setup so server-side manifest fetches are deterministic when Node MSW is disabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Use an http://127.0.0.1:9999 manifest URL so CI does not depend on NEXT_PUBLIC_MOCK_API for server-side manifest fetches. Co-authored-by: Cursor <cursoragent@cursor.com>
Set NEXT_PUBLIC_MOCK_API in Playwright webServer env so fetchManifest uses localhost:9999. Harden global-setup teardown on warmup failure; clarify dataset-detail + mock stub docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Add journey tests for dashboard, workflows, datasets, logs, profile, and related UI flows. Ignore coverage-e2e/ alongside Vitest coverage output in src/ui/.gitignore.
- Wait for recent-workflow anchor by href; avoid flaky networkidle; race-free waitForURL+click - Target DAG group via role=treeitem (matches GroupNode a11y); scroll into view; relax heading matchers - Wait for .react-flow before interacting on workflow detail
Clicks on the outer treeitem often miss the inner header div that owns onClick under React Flow. The treeitem already handles Enter/Space to call onSelectGroup for multi-task nodes. Also relax subtitle text match (regex), widen heading match for training, and raise describe timeout.
Cover stat cards, dataset panel/tables, occupancy, pool panel, profile save flows, sidebar state, submit/cancel/resubmit mutations, DAG a11y/expand, group detail/tasks/timeline, task switcher and tabs, sequence nav, and status hover card. Excludes scripts/start-dev.mjs (local dev only).
Add optional commented sections for src/ui changes and e2e validation commands; clarify title when no issue.
This reverts commit 0f074e0.
Contributor
📝 WalkthroughWalkthroughAdds extensive Playwright E2E tests across major UI areas (Dashboard, Pools, Resources, Occupancy, Datasets, Log Viewer, Profile, Workflows) and updates src/ui/.gitignore to ignore coverage-e2e. ChangesUI E2E Coverage Expansion
Sequence Diagram(s)(skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/e2e/journeys/pool-platform-selector.spec.ts (1)
1-204:⚠️ Potential issue | 🔴 CriticalCorrect the command path: use
cd src/uiinstead ofcd external/src/uiThe path in the original guideline refers to
external/src/ui, but the correct path is simplycd src/ui. Additionally, when running the command, TypeScript type-checking currently fails with errors in the e2e test files (including this spec file), indicating either missing dependencies in the environment or pre-existing configuration issues in the test suite. The specific errors are:
Cannot find module '@playwright/test'(line 17)Binding element 'page' implicitly has an 'any' type(lines 38, 43, 76, 81, 107, 137, 179)Please run the corrected command sequence:
cd src/ui && pnpm type-check && pnpm lint && pnpm test --run && pnpm formatVerify all checks pass with ZERO errors and ZERO warnings before marking the change complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/pool-platform-selector.spec.ts` around lines 1 - 204, Update the CLI path to use "cd src/ui" (not external/src/ui) and fix the TypeScript/test environment so the e2e spec compiles: ensure `@playwright/test` is installed as a devDependency (so import { test, expect } from "@playwright/test" resolves) and enable Playwright types so the "page" fixture is properly typed (either add "types": ["@playwright/test"] to your tsconfig.json/tsconfig.test.json or add a triple-slash reference /// <reference types="@playwright/test" /> at the top of these spec files like pool-platform-selector.spec.ts); then run the suggested command sequence (cd src/ui && pnpm type-check && pnpm lint && pnpm test --run && pnpm format) and verify zero errors/warnings.
🧹 Nitpick comments (12)
src/ui/e2e/journeys/pool-platform-selector.spec.ts (1)
65-67: ⚡ Quick winReplace
waitForLoadState("networkidle")with UI-ready locator waits.
networkidleis explicitly discouraged in Playwright documentation for test readiness. Use locator-based alternatives instead to prevent flakiness in SPA-style apps.Proposed pattern
- await page.waitForLoadState("networkidle"); + const panel = page.getByRole("complementary", { name: "Pool details: single-plat-pool" }); + await expect(panel).toBeVisible();Apply the same pattern to lines 100, 126, 167, and 197 with each pool's corresponding panel name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/pool-platform-selector.spec.ts` around lines 65 - 67, Replace the brittle await page.waitForLoadState("networkidle") calls used after page.goto("/pools?all=true&view=single-plat-pool") in pool-platform-selector.spec.ts with explicit UI-ready locator waits: find a stable locator that signals the pool panel is rendered (e.g., the panel header or a unique element inside the panel) and use locator.waitFor({ state: "visible" }) or Playwright's expect(locator).toBeVisible() instead; apply the same pattern for the other occurrences mentioned (the waits at the locations corresponding to the other pool panels on lines 100, 126, 167, and 197) by replacing their waitForLoadState("networkidle") with waits against each pool's specific panel locator.src/ui/e2e/journeys/submit-workflow-mutation.spec.ts (1)
205-214: ⚡ Quick winConsider a more targeted approach than unrouteAll.
Using
page.unrouteAll({ behavior: "ignoreErrors" })followed by re-setup can be fragile and may cause race conditions if routes are accessed during the re-setup window. Consider instead:
- Not setting up the profile mock in this specific test's beforeEach, or
- Using a more targeted unroute for just the profile endpoint, or
- Structuring the test to avoid needing unroute by using a separate describe block without the profile mock
♻️ Alternative approach
Option 1: Create a separate describe block without profile mock:
+test.describe("Submit Workflow Mutation — Missing Pool", () => { + test.beforeEach(async ({ page }) => { + await setupDefaultMocks(page); + // Intentionally skip setupProfileWithDefaultPool + await setupPools( + page, + createPoolResponse([{ name: "test-pool", status: PoolStatus.ONLINE }]), + ); + }); + + test("submit button disabled when pool is not yet loaded (no profile mock)", async ({ page }) => { - await page.unrouteAll({ behavior: "ignoreErrors" }); - await setupDefaultMocks(page); - await setupPools( - page, - createPoolResponse([{ name: "test-pool", status: PoolStatus.ONLINE }]), - ); - const overlay = await openFormView(page); // ... rest of test }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/submit-workflow-mutation.spec.ts` around lines 205 - 214, The test uses page.unrouteAll({ behavior: "ignoreErrors" }) which is brittle; instead avoid removing all routes and either skip registering the profile mock for this test or unroute only the profile endpoint: remove the call to page.unrouteAll and ensure setupDefaultMocks(page) is invoked without the profile route (or call page.unroute with the specific profile route pattern before reconfiguring mocks), and keep the rest of the arrangement using setupPools and not calling setupProfileWithDefaultPool so the pool remains empty; update the test to reference page.unroute (targeting the profile endpoint) or modify test setup so setupDefaultMocks does not register the profile mock for this case.src/ui/e2e/journeys/pool-gpu-summary.spec.ts (1)
154-196: ⚡ Quick winThe aggregation test does not assert an aggregate value.
This case currently verifies labels and row visibility only. Add at least one summary-card value/percentage assertion derived from both pools so the test actually fails on aggregation bugs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/pool-gpu-summary.spec.ts` around lines 154 - 196, The test "aggregates GPU values across multiple pools" currently only checks labels and rows; add an assertion that verifies an actual aggregated GPU value (e.g., total quota_used 20+30 = 50) so the test fails on aggregation regressions. Locate the test and after navigation/wait (in the same test), compute the expected aggregate (50) and assert it appears in the GPU summary card — referencing helpers like setupPools, createPoolResponse and PoolStatus used to seed data and the page queries (e.g., page.getByText or a more specific selector targeting the "GPU Quota" summary card) to check the aggregated value or percentage. Ensure the assertion targets the first visible GPU summary element so it is robust across render order.src/ui/e2e/journeys/workflow-spec-viewer.spec.ts (1)
107-145: 💤 Low valueConsider simplifying route registration order.
The current pattern registers specific routes first, then a catch-all with
fallback()logic. While correct, it's more complex than needed. A simpler approach: register the catch-all route first (so LIFO tries it last), then register more-specific routes afterward (tried first).♻️ Simpler registration order
async function setupWorkflowAndSpec( page: Parameters<typeof setupDefaultMocks>[0], name: string, ) { const data = createWorkflowDetailForSpec(name); + // Register catch-all FIRST (tried LAST in LIFO) + await page.route(`**/api/workflow/${name}`, (route) => + route.fulfill({ + status: 200, + contentType: CT_JSON, + body: JSON.stringify(data), + }), + ); + - // Mock the spec endpoint — registered BEFORE the catch-all so LIFO gives these priority + // Register specific routes LAST (tried FIRST in LIFO) await page.route(`**/api/workflow/${name}/spec`, (route) => route.fulfill({ status: 200, contentType: "text/plain", body: "version: 1\ntasks:\n - name: train\n image: nvcr.io/nvidia/pytorch:24.01-py3\n command: python train.py", }), ); - // Mock the template_spec endpoint await page.route(`**/api/workflow/${name}/template_spec`, (route) => route.fulfill({ status: 200, contentType: "text/plain", body: "version: 1\ntasks:\n - name: train\n image: {{image}}\n command: {{command}}", }), ); - - // Mock the workflow detail endpoint — registered LAST so LIFO tries it first for the base URL - // but the more specific spec/template_spec routes above will match first for those sub-paths - await page.route(`**/api/workflow/${name}*`, (route) => { - const url = route.request().url(); - // Let spec/template_spec requests fall through to their specific handlers - if (url.includes(`/workflow/${name}/spec`) || url.includes(`/workflow/${name}/template_spec`)) { - return route.fallback(); - } - return route.fulfill({ - status: 200, - contentType: CT_JSON, - body: JSON.stringify(data), - }); - }); }This eliminates the conditional
fallback()logic and relies on Playwright's LIFO matching: more-specific routes (registered last) are tried before the catch-all (registered first).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-spec-viewer.spec.ts` around lines 107 - 145, The route registration is more complex than necessary in setupWorkflowAndSpec; instead register the catch-all page.route(`**/api/workflow/${name}*`) first (so LIFO runs it last) and then register the two specific routes for `/spec` and `/template_spec` afterward, remove the URL.includes(...) conditional and the route.fallback() usage, and have the catch-all handler simply fulfill the workflow detail (using CT_JSON and JSON.stringify(data)) while the later-registered specific handlers fulfill their plain-text bodies; this simplifies page.route usage and eliminates the special-case branching.src/ui/e2e/journeys/workflow-group-timeline-phases.spec.ts (1)
144-150: ⚡ Quick winExtract
selectDagGroupto a shared E2E helper module.This helper is duplicated from
workflow-group-tasks-interaction.spec.ts(lines 156-162). Per coding guidelines, avoid code duplication by extracting it to a shared location such assrc/ui/e2e/utils/workflow-helpers.ts.♻️ Suggested approach
Create
src/ui/e2e/utils/workflow-helpers.ts:import { expect, type Page, type Locator } from "@playwright/test"; /** Select a DAG group: focus + Enter to handle React Flow transforms */ export async function selectDagGroup(page: Page, treeitem: Locator) { await expect(treeitem).toBeVisible({ timeout: 20_000 }); await treeitem.scrollIntoViewIfNeeded(); await treeitem.focus(); await page.keyboard.press("Enter"); }Then import:
import { selectDagGroup } from "@/e2e/utils/workflow-helpers";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-group-timeline-phases.spec.ts` around lines 144 - 150, Extract the duplicated selectDagGroup function into a shared E2E helper module and import it from both specs; specifically, move the function (signature: async function selectDagGroup(page: Page, treeitem: Locator)) into a new helper module, export it, remove the local copies in workflow-group-timeline-phases.spec.ts and workflow-group-tasks-interaction.spec.ts, and replace them with an import of the exported selectDagGroup; ensure the helper imports expect, Page and Locator from Playwright and preserves the visibility/scroll/focus/Enter behavior and the 20_000ms timeout.src/ui/e2e/journeys/workflow-group-tasks-interaction.spec.ts (1)
156-162: ⚡ Quick winExtract
selectDagGroupto a shared E2E helper module.This helper is duplicated in
workflow-group-timeline-phases.spec.ts(lines 144-150). Per coding guidelines, avoid code duplication by extracting it to a shared location such assrc/ui/e2e/utils/orsrc/ui/e2e/helpers/.♻️ Suggested location for shared helper
Create
src/ui/e2e/utils/workflow-helpers.ts:import { expect, type Page, type Locator } from "@playwright/test"; /** Select a DAG group: focus + Enter to handle React Flow transforms */ export async function selectDagGroup(page: Page, treeitem: Locator) { await expect(treeitem).toBeVisible({ timeout: 20_000 }); await treeitem.scrollIntoViewIfNeeded(); await treeitem.focus(); await page.keyboard.press("Enter"); }Then import and use:
import { selectDagGroup } from "@/e2e/utils/workflow-helpers";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-group-tasks-interaction.spec.ts` around lines 156 - 162, The selectDagGroup helper is duplicated across two E2E spec files; extract this function into a single shared module (e.g., an e2e utils/helpers module), export it, add the necessary imports (expect, Page, Locator), then replace the local copies in both spec files with an import of the shared selectDagGroup and remove the duplicated definitions so both tests call the same exported function and retain identical behavior (focus, scrollIntoViewIfNeeded, visibility assert, Enter key press).src/ui/e2e/journeys/workflow-cancel-dialog-ui.spec.ts (2)
38-39: 💤 Low valueExtract time calculation constants for clarity.
The magic numbers
60 * 60 * 1000appear repeatedly across test files. Consider defining named constants (e.g.,const ONE_HOUR_MS = 60 * 60 * 1000) at the module or shared-utility level for improved readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-cancel-dialog-ui.spec.ts` around lines 38 - 39, The test uses a magic time literal when computing oneHourAgo; replace the repeated expression in this file by introducing a named constant (e.g., ONE_HOUR_MS) and use it when computing oneHourAgo (and any other time calculations in this spec), so update the declaration that computes oneHourAgo to use ONE_HOUR_MS and extract any other repeated time multipliers into the same constant for clarity and reuse.
36-102: ⚖️ Poor tradeoffConsider extracting shared workflow fixture logic to a test utility module.
The
createRunningWorkflowhelper is similar to helpers in other specs (e.g.,createWorkflowForStatusCard,createWorkflowDetailResponse). Consolidating these into a shared test utility (e.g.,src/ui/e2e/utils/workflow-fixtures.ts) would reduce duplication and improve maintainability across the journey test suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-cancel-dialog-ui.spec.ts` around lines 36 - 102, Extract the duplicated fixture logic into a shared test utility: create a new module (e.g., src/ui/e2e/utils/workflow-fixtures.ts) that exports reusable helpers such as createRunningWorkflow, createWorkflowForStatusCard, and createWorkflowDetailResponse; update the spec file (workflow-cancel-dialog-ui.spec.ts) to import createRunningWorkflow from that utility and remove the local helper, ensuring the exported functions accept the same parameters/shape (name, timestamps, status, groups, etc.) so existing tests continue to work without behavior changes.src/ui/e2e/journeys/workflow-detail-tabs.spec.ts (1)
206-244: 💤 Low valueDocument route registration ordering dependency.
The test correctly registers specific routes (
/spec,/template_spec) after the catch-all**/api/workflow/${name}*frombeforeEach. Playwright matches routes in reverse registration order, so this works. However, this relies on implicit ordering behavior. Consider adding a comment explaining the ordering requirement or refactoring to make route setup more explicit (e.g., parameterized setup helper).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-detail-tabs.spec.ts` around lines 206 - 244, The test "Spec tab shows spec viewer toolbar with YAML and Template buttons" relies on Playwright's route matching order because specific page.route calls for `/api/workflow/${wfName}/spec` and `/template_spec` must be registered after the catch-all route set up in beforeEach (or setupWorkflowDetail) so they win; update the test by adding a concise comment above the two page.route registrations stating this ordering dependency and, if you prefer a more robust change, refactor the test setup to accept parameterized route handlers from setupWorkflowDetail (or extract a helper like registerWorkflowSpecRoutes) so the specific routes are registered explicitly after the catch-all without relying on implicit reverse-registration behavior.src/ui/e2e/journeys/resource-panel-content.spec.ts (1)
74-75: ⚡ Quick winReplace
networkidlewaits with explicit UI visibility assertions.Each occurrence waits for network idle, then immediately checks that the panel is visible. Use the existing panel selector to wait for visibility instead of network idle:
// Replace this: await page.goto("/resources?view=dgx-001"); await page.waitForLoadState("networkidle"); const panel = page.getByRole("complementary", { name: "Resource details: dgx-001" }); await expect(panel).toBeVisible(); // With this: await page.goto("/resources?view=dgx-001"); await page.getByRole("complementary", { name: "Resource details: dgx-001" }).waitFor({ state: "visible" }); await expect(panel).toBeVisible(); // Or simply: await page.goto("/resources?view=dgx-001"); const panel = page.getByRole("complementary", { name: "Resource details: dgx-001" }); await expect(panel).toBeVisible(); // This assertion serves as your synchronization pointNetwork-idle waits are unreliable in CI when background requests persist. The panel visibility is the deterministic readiness signal here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/resource-panel-content.spec.ts` around lines 74 - 75, The test currently synchronizes with page.waitForLoadState("networkidle") which is flaky; replace that wait with an explicit visibility wait on the panel selector used in this file: locate the complementary region via page.getByRole("complementary", { name: "Resource details: dgx-001" }) and either call .waitFor({ state: "visible" }) on that locator or immediately assert await expect(panel).toBeVisible() to serve as the synchronization point; remove the page.waitForLoadState("networkidle") call and keep the rest of the navigation/assertion flow unchanged.src/ui/e2e/journeys/workflow-detail-links-tags.spec.ts (1)
144-146: 💤 Low valueOptional: Simplify locator chains by removing redundant
.first()calls.Similar to other specs,
getByTextandgetByRolealready return single locators, making.first()unnecessary. This appears on lines 144, 145, 146, 164, 165, 217, 218, 219.Also applies to: 164-166, 217-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-detail-links-tags.spec.ts` around lines 144 - 146, The test uses redundant .first() calls on Playwright locators (e.g., page.getByText("Dashboard").first(), page.getByText("Grafana").first(), page.getByText("Outputs").first()) — remove the .first() suffix from these getByText/getByRole usages (and the other occurrences on the same file at the ranges you noted) so the assertions use the single locator returned by getByText/getByRole directly; update the assertions to call toBeVisible() on page.getByText("...") or page.getByRole("...") without .first().src/ui/e2e/journeys/workflow-group-detail-content.spec.ts (1)
302-310: 💤 Low valueOptional: Remove redundant
.first()calls ongetByRolelocators.
getByRolealready returns a single locator (the first matching element), so chaining.first()is unnecessary. This pattern appears throughout the file (lines 302, 306, 309, 310, 319, 321, 324, 337, etc.).Example simplification
- await expect(page.getByText("Upstream").first()).toBeVisible({ timeout: 5_000 }); - await expect(page.getByText("Downstream").first()).toBeVisible({ timeout: 5_000 }); + await expect(page.getByText("Upstream")).toBeVisible({ timeout: 5_000 }); + await expect(page.getByText("Downstream")).toBeVisible({ timeout: 5_000 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/e2e/journeys/workflow-group-detail-content.spec.ts` around lines 302 - 310, Remove redundant .first() calls on Playwright locators that already return a single element: update uses such as the dagNode assignment (page.getByRole("treeitem", { name: /train-model/i }).first()) and the expectations (expect(page.getByRole(...).first()), expect(page.getByText("Upstream").first()), expect(page.getByText("Downstream").first())) by calling getByRole/getByText directly without .first(); keep the surrounding waits and the selectDagGroup(page, dagNode) call unchanged and run tests to confirm no behavior change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/e2e/journeys/dashboard-errors.spec.ts`:
- Around line 147-149: The assertions currently use page.getByText("3").first()
/ page.getByText("2").first() which can match unrelated numbers; instead find
the card container for the "Active Workflows" and "Pools Online" regions (locate
the element containing the heading text like "Active Workflows" / "Pools
Online") and scope the count assertion to that container (use the container
locator then check its child text/count). Update the assertions around the uses
of page.getByText("3").first() and page.getByText("2").first() (including the
similar block at lines 169-171) to assert the count within the specific card
container rather than globally.
In `@src/ui/e2e/journeys/dataset-panel-versions.spec.ts`:
- Around line 236-239: The test currently asserts only the "Dataset" column
header; add two more visible assertions for the other table headers by calling
await expect(page.getByText("Version", { exact: true }).first()).toBeVisible()
and await expect(page.getByText("Size", { exact: true }).first()).toBeVisible()
alongside the existing page.getByText("Dataset", { exact: true }).first()
assertions in the dataset-panel-versions.spec.ts test so all three headers
(Dataset, Version, Size) are verified.
In `@src/ui/e2e/journeys/empty-states.spec.ts`:
- Around line 85-95: The test "pools page loads without crash when no pools
exist" (and the similar test around lines 105-115) only asserts generic page
text; change the assertions to verify a concrete empty-state signal after
calling setupPools(createPoolResponse([])). Locate the test function and replace
or add to the expect(page.getByText(/pools/i).first()) check with an assertion
that matches the empty-state UI such as expect(page.getByText(/no pools found|no
resources|0 results/i)).toBeVisible() or assert the table has zero data rows via
a role/selector (e.g., expect(await
page.getByRole('row').count()).toBe(expectedHeaderRows) or
expect(page.locator('table >> tbody >> tr').count()).resolves.toBe(0)), so the
test fails on regressions rather than merely rendering.
In `@src/ui/e2e/journeys/not-found-page.spec.ts`:
- Around line 51-53: The three separate regex checks using
page.getByText(/our/i), page.getByText(/server/i), and page.getByText(/missed/i)
are too loose; replace them with a single assertion that verifies the full
contiguous phrase (e.g., use page.getByText with a regex like
/our\s+server\s+missed\s+one/i) so the OSMO acronym text is matched as the
intended phrase; update the test where page.getByText is called to assert the
complete phrase is visible.
In `@src/ui/e2e/journeys/occupancy-summary-values.spec.ts`:
- Around line 83-102: The numeric assertions are currently global (e.g.,
page.getByText("14").first() and page.getByText("96").first()) and can hit
unrelated text; update the two tests ("GPU" assertion and the "CPU KPI card
shows total CPU count" test) to scope the check to the KPI card container by
first locating the card element using its visible label (e.g., find the card by
the "GPU" or "CPU" label with page.getByText("GPU") / page.getByText("CPU") or a
suitable role/test-id) and then assert that the card-locator contains the
expected number (e.g., cardLocator.getByText("14") or
cardLocator.getByText("96")), replacing the existing page.getByText(...).first()
assertions.
In `@src/ui/e2e/journeys/pool-platform-selector.spec.ts`:
- Around line 81-105: The test "multiple platforms show a dropdown trigger with
the selected platform name" currently only asserts dropdown visibility via the
`panel.getByRole("button", { name: "Select platform" })`; update the test to
match its name by asserting the selected platform label is shown (e.g., check
the button text equals the active platform name) using the existing `panel`
handle and role lookup, or alternatively rename the test to something like
"dropdown trigger is visible when multiple platforms exist" if you intend to
keep only the visibility assertion; adjust the assertion accordingly in the
`test(...)` block that uses `setupPools`, `createPoolResponse`, `PoolStatus`,
and `panel`.
In `@src/ui/e2e/journeys/sidebar-active-state.spec.ts`:
- Around line 55-89: The active-state assertions are too broad: instead of
grabbing any element with '[data-active="true"]', modify the tests (e.g., in the
"Dashboard nav item is active when on root page" and "navigating between pages
changes active state" tests) to locate all active items from sidebar
(sidebar.locator('[data-active="true"]')), assert its count equals 1, and then
assert that the single element's text exactly matches the expected label
("Dashboard", "Pools", or "Workflows") so the test fails if the wrong nav item
is active.
In `@src/ui/e2e/journeys/submit-workflow-form.spec.ts`:
- Line 18: The test imports PoolStatus from "@/mocks/factories" but per
guidelines enums must be imported from the generated API; update the import
statement so that createPoolResponse continues to be imported from
"@/mocks/factories" while PoolStatus is imported directly from
"@/lib/api/generated" (i.e., change the import source for the PoolStatus symbol
only), then run the tests to ensure no other references need updating.
- Around line 51-101: The three duplicated helper functions
setupProfileWithDefaultPool, openFormView, and waitForPoolSelected should be
extracted into a shared helper module (e.g., create
src/ui/e2e/utils/submit-workflow-helpers.ts exporting those functions) and both
submit-workflow-form.spec.ts and submit-workflow-mutation.spec.ts should import
them instead of defining them locally; move the CT_JSON constant and imports
(expect, Locator, Page) into the new module, export the three functions with the
same signatures, then replace the local implementations in both spec files with
imports from "@/e2e/utils/submit-workflow-helpers".
In `@src/ui/e2e/journeys/table-column-toggle.spec.ts`:
- Around line 95-105: Test names claim they verify visible "Status" and
"Backend" columns but only assert row labels; change assertions to target column
headers and their cell values. In the "pool table shows Status and Backend
columns by default" test, replace or add expectations to check for the column
headers (e.g., locate "Status" and "Backend" as columnheader role or header
text) and assert specific cells under those columns contain expected values for
"prod-pool" and "staging-pool" (use cell locators relative to the row or column,
not only getByText for pool names). Apply the same fix pattern to the other
tests referenced by name/position (the tests around the 161-171 and 228-248
ranges): assert column headers using getByRole('columnheader', { name: ... }) or
header text and then assert the corresponding column cells contain the expected
status/backend values for the rows under test.
- Around line 79-93: The test only verifies the column menu opens but never
toggles any options; update the failing test (and the similar tests around the
other ranges) to click a specific column checkbox from the opened menu, assert
the corresponding column header and at least one cell become hidden, then click
the checkbox again and assert the header/cell reappear; locate the menu checkbox
with page.getByRole("menuitemcheckbox", { name: /Column Label/i }) and validate
table visibility with page.getByRole("columnheader", { name: /Column Label/i })
and a representative page.getByRole("cell", { name: /expected cell text/i }) (or
use a row/cell locator) to confirm the toggle actually changes DOM visibility.
In `@src/ui/e2e/journeys/workflow-dag-expand-collapse.spec.ts`:
- Around line 248-261: The test "single-task node does not show expand button or
count badge" currently only asserts absence of aria-expanded on the
downloadNode; add explicit negative assertions to also check the count badge and
expand/collapse buttons are not present: after locating downloadNode
(page.getByRole("treeitem", { name: /download-data/i }).first()) assert that the
count badge selector used elsewhere (the label matching /\d+ tasks/ or the
specific badge element rendered by GroupNode) is not visible or does not exist,
and assert that any expand/collapse buttons (the roles found by
page.getByRole("button", { name: /expand to show|collapse task list/i })) are
not visible or do not exist; keep these assertions consistent with the positive
checks used in other tests so the test fully verifies no badge or toggle is
rendered for single-task GroupNode.
In `@src/ui/e2e/journeys/workflow-group-detail-content.spec.ts`:
- Around line 1-388: Add explicit Playwright type annotations and import the
Route type: update the top import from "@playwright/test" to include "type
Route" (so types are available), then annotate every test callback that
destructures page as async ({ page }: { page: Page }) (this applies to the
beforeEach handlers and all test(...) callbacks using page) and annotate route
handler parameters as (route: Route) in the test.beforeEach blocks that call
page.route; locate the relevant handlers around createWorkflowWithGroupDeps,
createWorkflowWithFailedGroup and selectDagGroup usages and update their
signatures accordingly.
In `@src/ui/e2e/journeys/workflow-group-nav.spec.ts`:
- Around line 232-233: Change the regex used to find the treeitem for the
training DAG to avoid coupling to punctuation: when creating dagNode with
page.getByRole("treeitem", { name: /training,/i }), remove the comma so it
becomes /training/i; update the selector where dagNode is defined (the getByRole
call) to match other group selectors and ensure selectDagGroup(page, dagNode)
continues to receive the same node.
In `@src/ui/e2e/journeys/workflow-resubmit-mutation.spec.ts`:
- Around line 110-139: The "Workflow Resubmit Mutation — Success" test
(test.describe block with wfName = "resubmit-mutation-success-wf") never
exercises the actual resubmit action; update the beforeEach/it flow to perform
the user action that submits the resubmit (click the resubmit button in the
panel and confirm submit), stub or mock the POST used by the server action (the
POST to /api/pool/{pool}/workflow or the localhost:9999 endpoint used when
NEXT_PUBLIC_MOCK_API=true) so it returns a 200 success response, and then assert
the expected success behavior: the panel is closed and the success toast is
shown (use the same UI selectors/helpers used elsewhere in tests). Ensure you
reference the existing helpers (setupDefaultMocks, setupPools,
createCompletedWorkflow) and the wfName variable when adding the submit click
and network mock so the test covers the success path.
In `@src/ui/e2e/journeys/workflow-sequence-nav.spec.ts`:
- Line 18: Replace the raw status string literals with the generated enum
values: import the WorkflowStatus enum from "@/lib/api/generated" (instead of
importing WorkflowStatus from mocks) and replace occurrences of "COMPLETED" (and
any other raw status strings at the mentioned spots) with the corresponding enum
member (e.g., WorkflowStatus.COMPLETED); update the import line to reference the
generated enums and adjust any test setup usages (the symbols to change are the
import line currently importing WorkflowStatus and the usages at the spots that
reference "COMPLETED") so the test consistently uses the generated API enums.
- Around line 125-126: Tests currently use await
page.waitForLoadState("networkidle") which is flaky; replace each occurrence
with a deterministic wait for the mocked workflow API response instead. Update
the five spots (the calls in workflow-sequence-nav.spec.ts) to use
page.waitForResponse or your existing request helper and match the specific
workflow API endpoint(s) used by the test (e.g., wait for responses whose url()
contains the workflow API path and have status 200), or wait for the named
route/handler used to mock the API, so the test proceeds only after the expected
workflow API reply is received.
In `@src/ui/e2e/journeys/workflow-timeline-phases.spec.ts`:
- Around line 149-150: The assertions use global page.getByText(...) which can
match outside the Timeline; create or reuse a locator for the Timeline section
(e.g., timelineLocator = page.locator(/* Timeline selector or test-id */) or
timeline = page.getByRole(.../getByTestId(...)) and replace
page.getByText("Submitted").first(), page.getByText("Started").first(), and
page.getByText("Completed").first() with
timelineLocator.getByText("Submitted").first(),
timelineLocator.getByText("Started").first(), and
timelineLocator.getByText("Completed").first() respectively so the checks are
scoped to the Timeline area.
---
Outside diff comments:
In `@src/ui/e2e/journeys/pool-platform-selector.spec.ts`:
- Around line 1-204: Update the CLI path to use "cd src/ui" (not
external/src/ui) and fix the TypeScript/test environment so the e2e spec
compiles: ensure `@playwright/test` is installed as a devDependency (so import {
test, expect } from "@playwright/test" resolves) and enable Playwright types so
the "page" fixture is properly typed (either add "types": ["@playwright/test"]
to your tsconfig.json/tsconfig.test.json or add a triple-slash reference ///
<reference types="@playwright/test" /> at the top of these spec files like
pool-platform-selector.spec.ts); then run the suggested command sequence (cd
src/ui && pnpm type-check && pnpm lint && pnpm test --run && pnpm format) and
verify zero errors/warnings.
---
Nitpick comments:
In `@src/ui/e2e/journeys/pool-gpu-summary.spec.ts`:
- Around line 154-196: The test "aggregates GPU values across multiple pools"
currently only checks labels and rows; add an assertion that verifies an actual
aggregated GPU value (e.g., total quota_used 20+30 = 50) so the test fails on
aggregation regressions. Locate the test and after navigation/wait (in the same
test), compute the expected aggregate (50) and assert it appears in the GPU
summary card — referencing helpers like setupPools, createPoolResponse and
PoolStatus used to seed data and the page queries (e.g., page.getByText or a
more specific selector targeting the "GPU Quota" summary card) to check the
aggregated value or percentage. Ensure the assertion targets the first visible
GPU summary element so it is robust across render order.
In `@src/ui/e2e/journeys/pool-platform-selector.spec.ts`:
- Around line 65-67: Replace the brittle await
page.waitForLoadState("networkidle") calls used after
page.goto("/pools?all=true&view=single-plat-pool") in
pool-platform-selector.spec.ts with explicit UI-ready locator waits: find a
stable locator that signals the pool panel is rendered (e.g., the panel header
or a unique element inside the panel) and use locator.waitFor({ state: "visible"
}) or Playwright's expect(locator).toBeVisible() instead; apply the same pattern
for the other occurrences mentioned (the waits at the locations corresponding to
the other pool panels on lines 100, 126, 167, and 197) by replacing their
waitForLoadState("networkidle") with waits against each pool's specific panel
locator.
In `@src/ui/e2e/journeys/resource-panel-content.spec.ts`:
- Around line 74-75: The test currently synchronizes with
page.waitForLoadState("networkidle") which is flaky; replace that wait with an
explicit visibility wait on the panel selector used in this file: locate the
complementary region via page.getByRole("complementary", { name: "Resource
details: dgx-001" }) and either call .waitFor({ state: "visible" }) on that
locator or immediately assert await expect(panel).toBeVisible() to serve as the
synchronization point; remove the page.waitForLoadState("networkidle") call and
keep the rest of the navigation/assertion flow unchanged.
In `@src/ui/e2e/journeys/submit-workflow-mutation.spec.ts`:
- Around line 205-214: The test uses page.unrouteAll({ behavior: "ignoreErrors"
}) which is brittle; instead avoid removing all routes and either skip
registering the profile mock for this test or unroute only the profile endpoint:
remove the call to page.unrouteAll and ensure setupDefaultMocks(page) is invoked
without the profile route (or call page.unroute with the specific profile route
pattern before reconfiguring mocks), and keep the rest of the arrangement using
setupPools and not calling setupProfileWithDefaultPool so the pool remains
empty; update the test to reference page.unroute (targeting the profile
endpoint) or modify test setup so setupDefaultMocks does not register the
profile mock for this case.
In `@src/ui/e2e/journeys/workflow-cancel-dialog-ui.spec.ts`:
- Around line 38-39: The test uses a magic time literal when computing
oneHourAgo; replace the repeated expression in this file by introducing a named
constant (e.g., ONE_HOUR_MS) and use it when computing oneHourAgo (and any other
time calculations in this spec), so update the declaration that computes
oneHourAgo to use ONE_HOUR_MS and extract any other repeated time multipliers
into the same constant for clarity and reuse.
- Around line 36-102: Extract the duplicated fixture logic into a shared test
utility: create a new module (e.g., src/ui/e2e/utils/workflow-fixtures.ts) that
exports reusable helpers such as createRunningWorkflow,
createWorkflowForStatusCard, and createWorkflowDetailResponse; update the spec
file (workflow-cancel-dialog-ui.spec.ts) to import createRunningWorkflow from
that utility and remove the local helper, ensuring the exported functions accept
the same parameters/shape (name, timestamps, status, groups, etc.) so existing
tests continue to work without behavior changes.
In `@src/ui/e2e/journeys/workflow-detail-links-tags.spec.ts`:
- Around line 144-146: The test uses redundant .first() calls on Playwright
locators (e.g., page.getByText("Dashboard").first(),
page.getByText("Grafana").first(), page.getByText("Outputs").first()) — remove
the .first() suffix from these getByText/getByRole usages (and the other
occurrences on the same file at the ranges you noted) so the assertions use the
single locator returned by getByText/getByRole directly; update the assertions
to call toBeVisible() on page.getByText("...") or page.getByRole("...") without
.first().
In `@src/ui/e2e/journeys/workflow-detail-tabs.spec.ts`:
- Around line 206-244: The test "Spec tab shows spec viewer toolbar with YAML
and Template buttons" relies on Playwright's route matching order because
specific page.route calls for `/api/workflow/${wfName}/spec` and
`/template_spec` must be registered after the catch-all route set up in
beforeEach (or setupWorkflowDetail) so they win; update the test by adding a
concise comment above the two page.route registrations stating this ordering
dependency and, if you prefer a more robust change, refactor the test setup to
accept parameterized route handlers from setupWorkflowDetail (or extract a
helper like registerWorkflowSpecRoutes) so the specific routes are registered
explicitly after the catch-all without relying on implicit reverse-registration
behavior.
In `@src/ui/e2e/journeys/workflow-group-detail-content.spec.ts`:
- Around line 302-310: Remove redundant .first() calls on Playwright locators
that already return a single element: update uses such as the dagNode assignment
(page.getByRole("treeitem", { name: /train-model/i }).first()) and the
expectations (expect(page.getByRole(...).first()),
expect(page.getByText("Upstream").first()),
expect(page.getByText("Downstream").first())) by calling getByRole/getByText
directly without .first(); keep the surrounding waits and the
selectDagGroup(page, dagNode) call unchanged and run tests to confirm no
behavior change.
In `@src/ui/e2e/journeys/workflow-group-tasks-interaction.spec.ts`:
- Around line 156-162: The selectDagGroup helper is duplicated across two E2E
spec files; extract this function into a single shared module (e.g., an e2e
utils/helpers module), export it, add the necessary imports (expect, Page,
Locator), then replace the local copies in both spec files with an import of the
shared selectDagGroup and remove the duplicated definitions so both tests call
the same exported function and retain identical behavior (focus,
scrollIntoViewIfNeeded, visibility assert, Enter key press).
In `@src/ui/e2e/journeys/workflow-group-timeline-phases.spec.ts`:
- Around line 144-150: Extract the duplicated selectDagGroup function into a
shared E2E helper module and import it from both specs; specifically, move the
function (signature: async function selectDagGroup(page: Page, treeitem:
Locator)) into a new helper module, export it, remove the local copies in
workflow-group-timeline-phases.spec.ts and
workflow-group-tasks-interaction.spec.ts, and replace them with an import of the
exported selectDagGroup; ensure the helper imports expect, Page and Locator from
Playwright and preserves the visibility/scroll/focus/Enter behavior and the
20_000ms timeout.
In `@src/ui/e2e/journeys/workflow-spec-viewer.spec.ts`:
- Around line 107-145: The route registration is more complex than necessary in
setupWorkflowAndSpec; instead register the catch-all
page.route(`**/api/workflow/${name}*`) first (so LIFO runs it last) and then
register the two specific routes for `/spec` and `/template_spec` afterward,
remove the URL.includes(...) conditional and the route.fallback() usage, and
have the catch-all handler simply fulfill the workflow detail (using CT_JSON and
JSON.stringify(data)) while the later-registered specific handlers fulfill their
plain-text bodies; this simplifies page.route usage and eliminates the
special-case branching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ae17ea0-1020-4837-a20a-6f447d87398e
📒 Files selected for processing (66)
src/ui/.gitignoresrc/ui/e2e/journeys/compact-mode-toggle.spec.tssrc/ui/e2e/journeys/credential-data-generic-types.spec.tssrc/ui/e2e/journeys/cross-page-navigation.spec.tssrc/ui/e2e/journeys/dashboard-errors.spec.tssrc/ui/e2e/journeys/dashboard-recent-workflows.spec.tssrc/ui/e2e/journeys/dashboard-stat-values.spec.tssrc/ui/e2e/journeys/dataset-file-browser.spec.tssrc/ui/e2e/journeys/dataset-panel-versions.spec.tssrc/ui/e2e/journeys/dataset-table-columns.spec.tssrc/ui/e2e/journeys/dataset-version-picker.spec.tssrc/ui/e2e/journeys/display-mode-toggle.spec.tssrc/ui/e2e/journeys/empty-states.spec.tssrc/ui/e2e/journeys/log-viewer-content.spec.tssrc/ui/e2e/journeys/log-viewer-recent.spec.tssrc/ui/e2e/journeys/log-viewer-selector.spec.tssrc/ui/e2e/journeys/not-found-page.spec.tssrc/ui/e2e/journeys/occupancy-row-actions.spec.tssrc/ui/e2e/journeys/occupancy-summary-values.spec.tssrc/ui/e2e/journeys/occupancy-toolbar.spec.tssrc/ui/e2e/journeys/occupancy-truncation.spec.tssrc/ui/e2e/journeys/panel-keyboard-interactions.spec.tssrc/ui/e2e/journeys/pool-gpu-summary.spec.tssrc/ui/e2e/journeys/pool-panel-quick-links.spec.tssrc/ui/e2e/journeys/pool-platform-selector.spec.tssrc/ui/e2e/journeys/profile-credential-save-mutation.spec.tssrc/ui/e2e/journeys/profile-credentials-form.spec.tssrc/ui/e2e/journeys/profile-notification-save-mutation.spec.tssrc/ui/e2e/journeys/profile-selection-save-flow.spec.tssrc/ui/e2e/journeys/profile-settings-save.spec.tssrc/ui/e2e/journeys/resource-panel-content.spec.tssrc/ui/e2e/journeys/resource-summary-cards.spec.tssrc/ui/e2e/journeys/sidebar-active-state.spec.tssrc/ui/e2e/journeys/status-display.spec.tssrc/ui/e2e/journeys/submit-workflow-form.spec.tssrc/ui/e2e/journeys/submit-workflow-mutation.spec.tssrc/ui/e2e/journeys/table-column-toggle.spec.tssrc/ui/e2e/journeys/workflow-breadcrumb-nav.spec.tssrc/ui/e2e/journeys/workflow-cancel-dialog-ui.spec.tssrc/ui/e2e/journeys/workflow-cancel-mutation.spec.tssrc/ui/e2e/journeys/workflow-dag-expand-collapse.spec.tssrc/ui/e2e/journeys/workflow-dag-node-a11y.spec.tssrc/ui/e2e/journeys/workflow-detail-actions.spec.tssrc/ui/e2e/journeys/workflow-detail-links-tags.spec.tssrc/ui/e2e/journeys/workflow-detail-overview.spec.tssrc/ui/e2e/journeys/workflow-detail-tabs.spec.tssrc/ui/e2e/journeys/workflow-group-detail-content.spec.tssrc/ui/e2e/journeys/workflow-group-detail-nav.spec.tssrc/ui/e2e/journeys/workflow-group-nav.spec.tssrc/ui/e2e/journeys/workflow-group-tasks-interaction.spec.tssrc/ui/e2e/journeys/workflow-group-timeline-phases.spec.tssrc/ui/e2e/journeys/workflow-lead-badge-viewtype.spec.tssrc/ui/e2e/journeys/workflow-pagination.spec.tssrc/ui/e2e/journeys/workflow-resubmit-mutation.spec.tssrc/ui/e2e/journeys/workflow-resubmit-panel.spec.tssrc/ui/e2e/journeys/workflow-sequence-nav.spec.tssrc/ui/e2e/journeys/workflow-spec-viewer.spec.tssrc/ui/e2e/journeys/workflow-status-hover-resubmit.spec.tssrc/ui/e2e/journeys/workflow-task-detail.spec.tssrc/ui/e2e/journeys/workflow-task-navigation.spec.tssrc/ui/e2e/journeys/workflow-task-overview-sections.spec.tssrc/ui/e2e/journeys/workflow-task-switcher.spec.tssrc/ui/e2e/journeys/workflow-task-timeline-phases.spec.tssrc/ui/e2e/journeys/workflow-tasks-tab-search.spec.tssrc/ui/e2e/journeys/workflow-tasks-tab-tree.spec.tssrc/ui/e2e/journeys/workflow-timeline-phases.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1015 +/- ##
==========================================
+ Coverage 44.92% 44.95% +0.03%
==========================================
Files 220 220
Lines 28559 28561 +2
Branches 4267 4266 -1
==========================================
+ Hits 12830 12840 +10
+ Misses 15106 15094 -12
- Partials 623 627 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fernandol-nvidia
approved these changes
May 14, 2026
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
2 participants
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.
Description
This pull request adds Playwright journey coverage under
src/ui/e2e/journeys/for the dashboard and workflow areas, and adjusts a few specs so they behave reliably in CI (e.g. navigation waits, DAG group selection via tree focus + Enter).Local validation was run from
src/uiwithpnpm type-check, lint/format checks, and targeted Playwright runs against Chromium.Issue #None
Checklist
Summary by CodeRabbit
Tests
Chores