CONSOLE-5212: Migrate Playwright setup to project dependencies#16408
CONSOLE-5212: Migrate Playwright setup to project dependencies#16408stefanonardo wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@stefanonardo: This pull request references CONSOLE-5212 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @fsgreco |
📝 WalkthroughWalkthroughThe Playwright E2E testing infrastructure was refactored from a monolithic global setup/teardown model to a modular, project-based architecture. The original 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
frontend/e2e/setup/admin-auth.setup.ts (1)
9-45: ⚡ Quick winExtract a shared login helper to eliminate ~35 lines of near-identical code with
developer-auth.setup.ts.The page navigation,
SERVER_FLAGS.authDisabledevaluation, IDP button detection, form fill, andstorageStatewrite/chmod sequence is duplicated verbatim. When one copy is patched (retry logic, error handling, different locator strategy) the other will inevitably drift.♻️ Suggested refactor — shared login helper
Create
frontend/e2e/setup/login-helper.ts:import * as fs from 'fs'; import * as path from 'path'; import type { Page } from '@playwright/test'; export async function performLogin( page: Page, baseURL: string, username: string, password: string, idpName?: string, ): Promise<void> { fs.mkdirSync(path.resolve(__dirname, '..', '.auth'), { recursive: true, mode: 0o700 }); await page.goto(baseURL, { timeout: 90_000, waitUntil: 'domcontentloaded' }); const authDisabled = await page .evaluate(() => (window as any).SERVER_FLAGS?.authDisabled) .catch(() => false); if (!authDisabled) { await page .locator('[data-test-id="login"]') .or(page.locator('#inputUsername')) .first() .waitFor({ state: 'visible', timeout: 30_000 }); if (idpName) { const providerButton = page.getByText(idpName, { exact: true }); if ((await providerButton.count()) > 0) await providerButton.click(); } await page.locator('#inputUsername').fill(username); await page.locator('#inputPassword').fill(password); await page.locator('button[type="submit"]').click(); await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible({ timeout: 60_000 }); } } export async function saveStorageState(page: Page, stateFilePath: string): Promise<void> { await page.context().storageState({ path: stateFilePath }); fs.chmodSync(stateFilePath, 0o600); }Then simplify
admin-auth.setup.tsanddeveloper-auth.setup.tsto callperformLogin+saveStorageState.🤖 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 `@frontend/e2e/setup/admin-auth.setup.ts` around lines 9 - 45, Duplicate login/setup logic exists in admin-auth.setup.ts and developer-auth.setup.ts; extract it into a shared helper (e.g., performLogin and saveStorageState in frontend/e2e/setup/login-helper.ts) that does: create STORAGE_STATE_DIR, navigate to baseURL, evaluate SERVER_FLAGS.authDisabled, handle optional idp button click, wait for locators, fill username/password and submit, then write storageState and chmod the file; update admin-auth.setup.ts and developer-auth.setup.ts to call performLogin(page, baseURL, username, password, idpName?) and saveStorageState(page, adminStorageState) to remove the ~35-line duplication and centralize future fixes.frontend/playwright.config.ts (1)
54-59: ⚡ Quick win
htmlreporter is absent from both CI and local reporter configs, yet the PR's test plan requires HTML report verification.The HTML reporter produces a self-contained folder with the test run report; it must be explicitly added to the
reporterarray — whenreporteris set, Playwright uses only the listed reporters. With the current config, no HTML report is ever produced locally (list) or in CI (dot+junit), making it impossible to verify the stated PR objective of "HTML report shows setup/teardown/auth as proper test entries with traces on failure" without passing--reporter=htmlmanually.Adding
htmlto the local branch at minimum would let the PR author verify the traces on setup failures that motivated this whole migration:✅ Suggested addition
reporter: isCI ? [ ['dot'], ['junit', { outputFile: path.resolve(__dirname, 'test-results', 'junit-results.xml') }], + ['html', { open: 'never', outputFolder: path.resolve(__dirname, 'test-results', 'playwright-report') }], ] - : [['list']], + : [['list'], ['html', { open: 'on-failure' }]],🤖 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 `@frontend/playwright.config.ts` around lines 54 - 59, The Playwright config's reporter array (symbol: reporter) currently omits the 'html' reporter for both branches (isCI true/false), so add the 'html' reporter entry to the CI array (alongside 'dot' and 'junit') and to the local array (alongside 'list') so Playwright emits an HTML report by default; update the reporter definitions in frontend/playwright.config.ts where isCI is used to construct reporter to include 'html' in both branches so HTML output is generated without passing --reporter=html manually.
🤖 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 `@frontend/e2e/setup/cluster.setup.ts`:
- Around line 25-35: The try/catch around creating and authenticating
KubernetesClient swallows authentication errors causing downstream projects to
run; update the catch in the block that constructs k8sClient and calls
verifyAuthentication() so that instead of silently setting k8sClient = null and
leaving clusterAvailable false it re-throws the caught exception (or calls
Playwright's expect.fail) so the cluster-setup project fails fast; locate the
try { new KubernetesClient(...); await verifyAuthentication(); clusterAvailable
= true } catch { ... } and replace the silent catch with a rethrow or explicit
test failure, preserving any error details in the thrown/failure message.
In `@frontend/e2e/setup/developer-auth.setup.ts`:
- Line 18: The htpasswdIdp constant is incorrectly defaulting to htpasswdUser
which is the username, not the identity provider name, causing provider
selection to fail; change the assignment for htpasswdIdp (the const declaration
named htpasswdIdp) to either throw/error if process.env.BRIDGE_HTPASSWD_IDP is
missing or default to a conventional provider string like 'htpasswd' (do not
reference htpasswdUser), and ensure any subsequent logic that clicks the
provider button uses this provider name.
In `@frontend/e2e/tests/smoke/developer/smoke-test.spec.ts`:
- Around line 3-6: The current smoke test ("console loads in developer
perspective") uses a weak assertion on body not.toBeEmpty; update it to assert a
developer-specific UI element is present and visible (e.g., the user dropdown or
the perspective switcher) after page.goto('/'). Locate the element in the test
(replace the body locator) and use a visibility/assertion API (e.g.,
page.locator(...).toBeVisible() or expect(...).toBeVisible()) to verify the
console loaded in developer context; keep the test name and flow but change the
assertion to target the developer perspective element.
---
Nitpick comments:
In `@frontend/e2e/setup/admin-auth.setup.ts`:
- Around line 9-45: Duplicate login/setup logic exists in admin-auth.setup.ts
and developer-auth.setup.ts; extract it into a shared helper (e.g., performLogin
and saveStorageState in frontend/e2e/setup/login-helper.ts) that does: create
STORAGE_STATE_DIR, navigate to baseURL, evaluate SERVER_FLAGS.authDisabled,
handle optional idp button click, wait for locators, fill username/password and
submit, then write storageState and chmod the file; update admin-auth.setup.ts
and developer-auth.setup.ts to call performLogin(page, baseURL, username,
password, idpName?) and saveStorageState(page, adminStorageState) to remove the
~35-line duplication and centralize future fixes.
In `@frontend/playwright.config.ts`:
- Around line 54-59: The Playwright config's reporter array (symbol: reporter)
currently omits the 'html' reporter for both branches (isCI true/false), so add
the 'html' reporter entry to the CI array (alongside 'dot' and 'junit') and to
the local array (alongside 'list') so Playwright emits an HTML report by
default; update the reporter definitions in frontend/playwright.config.ts where
isCI is used to construct reporter to include 'html' in both branches so HTML
output is generated without passing --reporter=html manually.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9ca063ea-4a83-4c78-ab1a-00e3c06d6a48
📒 Files selected for processing (9)
frontend/e2e/global.setup.tsfrontend/e2e/global.teardown.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/setup/teardown.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/package.jsonfrontend/playwright.config.ts
💤 Files with no reviewable changes (2)
- frontend/e2e/global.setup.ts
- frontend/e2e/global.teardown.ts
📜 Review details
🔇 Additional comments (3)
frontend/e2e/setup/teardown.setup.ts (1)
10-49: LGTM. The multi-layered defensive reads (missing file → parse error → missingtestNamespace) are all handled gracefully, the debug-mode skip is consistent with the rest of the config, and surfacing namespace-deletion failures viaexpect()is the right call for visibility.frontend/playwright.config.ts (1)
30-30: AI summary inconsistency:dev-consolewas not removed fromdevPackages.The AI-generated summary states
dev-consolewas removed from the developer-persona mapping, but the code clearly showsdevPackages = ['smoke', 'dev-console', 'shipwright', 'topology']—dev-consoleremains andsmokewas added. The summary is incorrect on this point.frontend/package.json (1)
52-57: LGTM. Consolidating individual per-project scripts intotest-playwright-adminandtest-playwright-developergroups is clean. The new--project=smoke-developerintest-playwright-developercorrectly maps to thesmoke-developerproject generated fromdevPackagesinplaywright.config.ts.
| try { | ||
| k8sClient = new KubernetesClient({ | ||
| clusterUrl: process.env.CLUSTER_URL || '', | ||
| username, | ||
| password, | ||
| }); | ||
| await k8sClient.verifyAuthentication(); | ||
| clusterAvailable = true; | ||
| } catch { | ||
| k8sClient = null; | ||
| } |
There was a problem hiding this comment.
Swallowing the cluster-auth exception causes cascading, misleading failures across all dependent projects.
When verifyAuthentication() throws, the catch block nullifies the client and leaves clusterAvailable = false, but the test itself passes (no re-throw, no assertion). In Playwright's project-dependency model, a passing cluster-setup project triggers admin-auth and developer-auth to run. Both will then attempt page.goto(baseURL) and time out with cryptic network errors instead of a clean "dependency failed — did not run" status.
Re-throwing (or using expect.fail()) makes cluster-setup fail, which Playwright uses to skip all dependent projects automatically:
🐛 Proposed fix
try {
k8sClient = new KubernetesClient({
clusterUrl: process.env.CLUSTER_URL || '',
username,
password,
});
await k8sClient.verifyAuthentication();
clusterAvailable = true;
} catch {
k8sClient = null;
+ throw new Error(
+ 'Cluster authentication failed — downstream auth/test projects will not run. ' +
+ 'Check CLUSTER_URL, OPENSHIFT_USERNAME, and BRIDGE_KUBEADMIN_PASSWORD.',
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| k8sClient = new KubernetesClient({ | |
| clusterUrl: process.env.CLUSTER_URL || '', | |
| username, | |
| password, | |
| }); | |
| await k8sClient.verifyAuthentication(); | |
| clusterAvailable = true; | |
| } catch { | |
| k8sClient = null; | |
| } | |
| try { | |
| k8sClient = new KubernetesClient({ | |
| clusterUrl: process.env.CLUSTER_URL || '', | |
| username, | |
| password, | |
| }); | |
| await k8sClient.verifyAuthentication(); | |
| clusterAvailable = true; | |
| } catch { | |
| k8sClient = null; | |
| throw new Error( | |
| 'Cluster authentication failed — downstream auth/test projects will not run. ' + | |
| 'Check CLUSTER_URL, OPENSHIFT_USERNAME, and BRIDGE_KUBEADMIN_PASSWORD.', | |
| ); | |
| } |
🤖 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 `@frontend/e2e/setup/cluster.setup.ts` around lines 25 - 35, The try/catch
around creating and authenticating KubernetesClient swallows authentication
errors causing downstream projects to run; update the catch in the block that
constructs k8sClient and calls verifyAuthentication() so that instead of
silently setting k8sClient = null and leaving clusterAvailable false it
re-throws the caught exception (or calls Playwright's expect.fail) so the
cluster-setup project fails fast; locate the try { new KubernetesClient(...);
await verifyAuthentication(); clusterAvailable = true } catch { ... } and
replace the silent catch with a rethrow or explicit test failure, preserving any
error details in the thrown/failure message.
There was a problem hiding this comment.
@stefanonardo the quick fix proposed here seems ok 👍🏽
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| setup.skip(!htpasswdUser || !htpasswdPass, 'No developer credentials configured'); | ||
|
|
||
| const baseURL = process.env.WEB_CONSOLE_URL || 'http://localhost:9000'; | ||
| const htpasswdIdp = process.env.BRIDGE_HTPASSWD_IDP || htpasswdUser!; |
There was a problem hiding this comment.
htpasswdIdp defaulting to the username value is semantically incorrect and can cause a hard-to-debug login failure.
BRIDGE_HTPASSWD_IDP is the identity provider name shown on the login selector screen (e.g. "HTPasswd"), not the username. When the env var is unset and the console shows a provider selection screen, page.getByText(htpasswdUser, { exact: true }) won't find the provider button, the click is skipped, but #inputUsername is also not yet visible — the waitFor on line 29-33 already resolved on [data-test-id="login"]. The subsequent fill('#inputUsername') call will time out with a misleading error.
Either require BRIDGE_HTPASSWD_IDP explicitly, or default to a conventional value like 'htpasswd':
- const htpasswdIdp = process.env.BRIDGE_HTPASSWD_IDP || htpasswdUser!;
+ const htpasswdIdp = process.env.BRIDGE_HTPASSWD_IDP || 'htpasswd';🤖 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 `@frontend/e2e/setup/developer-auth.setup.ts` at line 18, The htpasswdIdp
constant is incorrectly defaulting to htpasswdUser which is the username, not
the identity provider name, causing provider selection to fail; change the
assignment for htpasswdIdp (the const declaration named htpasswdIdp) to either
throw/error if process.env.BRIDGE_HTPASSWD_IDP is missing or default to a
conventional provider string like 'htpasswd' (do not reference htpasswdUser),
and ensure any subsequent logic that clicks the provider button uses this
provider name.
| test('console loads in developer perspective', async ({ page }) => { | ||
| await page.goto('/'); | ||
| await expect(page.locator('body')).not.toBeEmpty(); | ||
| }); |
There was a problem hiding this comment.
Smoke assertion on body is trivially weak and doesn't verify the developer perspective.
not.toBeEmpty() on body passes for any page, including 500/404 error pages. A meaningful smoke test should assert something that confirms the console actually loaded in the developer context — e.g. the user dropdown or the perspective switcher being visible.
🔍 Suggested stronger assertion
test('console loads in developer perspective', async ({ page }) => {
await page.goto('/');
- await expect(page.locator('body')).not.toBeEmpty();
+ await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('console loads in developer perspective', async ({ page }) => { | |
| await page.goto('/'); | |
| await expect(page.locator('body')).not.toBeEmpty(); | |
| }); | |
| test('console loads in developer perspective', async ({ page }) => { | |
| await page.goto('/'); | |
| await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible(); | |
| }); |
🤖 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 `@frontend/e2e/tests/smoke/developer/smoke-test.spec.ts` around lines 3 - 6,
The current smoke test ("console loads in developer perspective") uses a weak
assertion on body not.toBeEmpty; update it to assert a developer-specific UI
element is present and visible (e.g., the user dropdown or the perspective
switcher) after page.goto('/'). Locate the element in the test (replace the body
locator) and use a visibility/assertion API (e.g.,
page.locator(...).toBeVisible() or expect(...).toBeVisible()) to verify the
console loaded in developer context; keep the test name and flow but change the
assertion to target the developer perspective element.
There was a problem hiding this comment.
@fsgreco, sounds good! When you implement it, just make sure the data-testid attribute you target (e.g., user-dropdown-toggle or the perspective switcher) actually exists in the rendered DOM for the developer perspective — a quick rg --type=tsx 'data-testid' in the relevant component can confirm the exact identifier. That'll ensure the smoke test fails meaningfully on a real regression rather than a structural issue. Let me know if you need help locating the right selector!
Replace globalSetup/globalTeardown with Playwright project dependencies so setup failures produce proper traces, screenshots, and HTML report entries instead of opaque errors. Split setup into three independent projects: - cluster-setup: K8s auth verification, namespace creation, test config - admin-auth: kubeadmin browser login (depends on cluster-setup) - developer-auth: htpasswd user browser login (depends on cluster-setup) Admin test projects depend on admin-auth, developer test projects depend on developer-auth. Running e.g. --project=smoke-developer only triggers cluster-setup and developer-auth, skipping the admin login entirely. Also adds a smoke-developer project for developer perspective smoke testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
CONSOLE-5212-2
branch
from
8f2a150 to
050f9e3
Compare
|
/honk |
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fsgreco, stefanonardo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@stefanonardo: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/verified by @fsgreco |
|
@fsgreco: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
verified
3 participants
Summary
globalSetup/globalTeardownwith Playwright project dependencies so setup failures produce proper traces, screenshots, and report entriescluster-setup,admin-auth,developer-auth) so running developer-only tests skips admin loginsmoke-developerproject for developer perspective smoke testingDetails
The dependency graph is:
Running
--project=smoke-developeronly triggerscluster-setup→developer-auth, skipping admin login entirely.Ref: https://playwright.dev/docs/test-global-setup-teardown#option-1-project-dependencies
Test plan
npx playwright test --project=smoke— confirms admin path:cluster-setup→admin-auth→smokenpx playwright test --project=smoke-developer— confirms developer path:cluster-setup→developer-auth→smoke-developer(no admin login)npx playwright test— all projects run with correct dependencies🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor