Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new Playwright E2E test suite for the Systems/Metrics dashboard (“cluster health”), along with a dedicated page object that encapsulates navigation, UI assertions, and API route mocking for /api/v1/systems/metrics.
Changes:
- Added
cluster-health.spec.tscovering navigation, initial fetch, time-range refetch, rendering, partial-data scenarios, and error scenarios. - Introduced
MetricsDashboardPagepage object with helpers for assertions and mocked API responses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| frontend/e2e/specs/cluster-health.spec.ts | New UC-50 cluster health E2E suite exercising happy path, partial-data, and error states. |
| frontend/e2e/pages/dashboard/metrics-dashboard.page.ts | New page object for /dashboard/systems, including UI helpers and /systems/metrics route mocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nodes: [ | ||
| { | ||
| name: "node-1", | ||
| cpuUsagePercent: 35.0, | ||
| memoryUsagePercent: 60.0, |
There was a problem hiding this comment.
The happy-path mock response reports overview.totalNodes: 3 / runningNodes: 3 and nodesMetric.value: "3/3", but the nodes array only contains 2 entries. This inconsistency can make UI behavior or future assertions confusing if counts are cross-checked. Adjust the mock so the overview/node-card counts match the nodes list.
| * • Use MetricsDashboardPage page-object | ||
| */ | ||
|
|
||
| import { expect } from "@playwright/test"; |
There was a problem hiding this comment.
The PR description/template is still largely blank (no summary, related issue, or testing notes). Please fill it in so reviewers understand the intent of the added E2E suite/page object and how it was validated.
| * Extension 6a – Prometheus timeout → null fields → 200 OK → zero-value cards, no crash | ||
| * Extension 10a – Some metric fields null → available cards render normally | ||
| * Exception – API 500 / network error → inline red error alert with exact text | ||
| * Postcondition – MetricCard elements present; NodesList has ≥1 row |
There was a problem hiding this comment.
The header comment says the four summary cards are "CPU%, RAM%, RunningNodes/TotalNodes, Tenants", but the current MetricsDashboard renders Requests/s, Running Pods, Nodes, and CPU Usage. Please update the comment to match the UI under test so the documented coverage/requirements aren’t misleading.
| * Postcondition – MetricCard elements present; NodesList has ≥1 row | |
| * Postcondition – MetricCard elements present (Requests/s, Running Pods, Nodes, CPU Usage); NodesList has ≥1 row |
| const refreshPromise = page.waitForRequest( | ||
| (req) => req.url().includes("/api/v1/systems/metrics"), | ||
| { timeout: 10_000 } | ||
| ); | ||
| await metricsPage.clickRefresh(); | ||
| await refreshPromise; |
There was a problem hiding this comment.
waitForRequest here can resolve due to the initial fetch on mount (or the 30s interval) rather than the Refresh click, so the test may pass even if the button doesn’t trigger a new request. Make it deterministic by awaiting the click and request together (e.g., Promise.all([page.waitForRequest(...), metricsPage.clickRefresh()])) and/or waiting for the initial metrics request to complete first.
| const refreshPromise = page.waitForRequest( | |
| (req) => req.url().includes("/api/v1/systems/metrics"), | |
| { timeout: 10_000 } | |
| ); | |
| await metricsPage.clickRefresh(); | |
| await refreshPromise; | |
| // Wait for the initial metrics request that fires on mount, so that the | |
| // subsequent waitForRequest is guaranteed to correspond to the manual refresh. | |
| await page.waitForRequest( | |
| (req) => req.url().includes("/api/v1/systems/metrics"), | |
| { timeout: 10_000 } | |
| ); | |
| // Tie the Refresh click and the expected API call together to avoid | |
| // accidentally matching an earlier or timer-based request. | |
| await Promise.all([ | |
| page.waitForRequest( | |
| (req) => req.url().includes("/api/v1/systems/metrics"), | |
| { timeout: 10_000 } | |
| ), | |
| metricsPage.clickRefresh() | |
| ]); |
| /** Confirms the happy-path "ready" state: no error banner visible. */ | ||
| async expectReady() { | ||
| await expect(this.page.getByTestId("systems-error")).not.toBeVisible({ timeout: 15_000 }); |
There was a problem hiding this comment.
expectReady() only asserts that the error banner is not visible, but systems-error is also absent during the initial loading state (before the first metrics fetch resolves). That means callers can treat the page as "ready" even though it’s still rendering skeletons. Consider renaming this to reflect what it checks (e.g., expectNoErrorBanner) and adding a separate readiness assertion that waits for the metrics request to complete and/or for at least one known piece of loaded content to appear.
| /** Confirms the happy-path "ready" state: no error banner visible. */ | |
| async expectReady() { | |
| await expect(this.page.getByTestId("systems-error")).not.toBeVisible({ timeout: 15_000 }); | |
| /** Confirms no error banner is visible. */ | |
| async expectNoErrorBanner() { | |
| await expect(this.errorBanner()).not.toBeVisible({ timeout: 15_000 }); | |
| } | |
| /** | |
| * Confirms the happy-path "ready" state: | |
| * - no error banner visible | |
| * - expected metric cards rendered (metrics fetch completed) | |
| */ | |
| async expectReady() { | |
| await this.expectNoErrorBanner(); | |
| await this.expectMetricCardsPresent(); |
| * MetricCard has no individual data-testid so we locate them by their | ||
| * shared CSS classes rendered inside data-testid="systems-page". | ||
| */ | ||
| metricCards() { | ||
| return this.page | ||
| .getByTestId("systems-page") | ||
| .locator(".rounded-xl.border.border-white\\/6.bg-white\\/3"); |
There was a problem hiding this comment.
metricCards() locates cards via Tailwind class strings. This is brittle (style refactors break tests) and it matches both skeleton and loaded MetricCard markup, so expectMetricCardsPresent() can pass before data has rendered. Prefer a more stable selector (e.g., add a data-testid on MetricCard/summary grid or assert on stable card titles from the mock response).
| * MetricCard has no individual data-testid so we locate them by their | |
| * shared CSS classes rendered inside data-testid="systems-page". | |
| */ | |
| metricCards() { | |
| return this.page | |
| .getByTestId("systems-page") | |
| .locator(".rounded-xl.border.border-white\\/6.bg-white\\/3"); | |
| * MetricCard elements are located via a dedicated data-testid to avoid | |
| * brittle Tailwind class selectors and to distinguish loaded cards from | |
| * any skeleton placeholders. | |
| * | |
| * NOTE: Each rendered MetricCard in the app should expose | |
| * `data-testid="systems-metric-card"`. | |
| */ | |
| metricCards() { | |
| return this.page.getByTestId("systems-metric-card"); |
| // ── Time-range selector ─────────────────────────────────────────────────── | ||
|
|
||
| timeRangeSelect() { | ||
| return this.page.locator("select"); |
There was a problem hiding this comment.
timeRangeSelect() targets the first matching select on the page. This page already contains other selects when dropdown menus are opened (e.g., Export Metrics), so this locator is prone to strict-mode failures if UI structure changes. Scope the locator under data-testid="systems-page" and/or select by accessible name/label to keep it stable.
| return this.page.locator("select"); | |
| return this.page.getByTestId("systems-page").locator("select"); |
🚀 Pull Request Overview
📌 Summary
🔗 Related Issues
Closes #
🧪 Changes
✅ What’s Included
❌ What’s Not Included
🧪 Testing
🔍 How Was This Tested?
🧪 Test Coverage
🧩 Breaking Changes
Does this PR introduce breaking changes?
If yes, describe the impact and migration steps:
🔐 Security Considerations
If checked, explain:
📝 Documentation
Does this PR require updates to documentation?
If yes, update the relevant locations:
/docsREADME.md🎨 UI/UX Considerations (If Applicable)
Screenshots / recordings (if applicable):
📦 Checklist Before Merge
👤 Contributor Notes (Optional)