CONSOLE-5212: Fix ESM compatibility for Playwright e2e tests#16445
Conversation
|
@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. |
📝 WalkthroughWalkthroughThis pull request refactors the Playwright E2E testing infrastructure for the Console. It replaces top-level Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/e2e/setup/admin-auth.setup.ts (1)
12-14: ⚡ Quick winEmpty password default may cause confusing failures.
Line 14 defaults
BRIDGE_KUBEADMIN_PASSWORDto an empty string. While login will fail with a clear timeout error, it would be more user-friendly to explicitly skip the setup or throw a clear error when the required credential is missing.Consider:
setup.skip(!process.env.BRIDGE_KUBEADMIN_PASSWORD, 'BRIDGE_KUBEADMIN_PASSWORD not set');This matches the pattern used in
developer-auth.setup.ts(lines 12-15).🤖 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 12 - 14, The file sets const password = process.env.BRIDGE_KUBEADMIN_PASSWORD || '' which silently defaults to an empty password and causes confusing failures; change the setup to explicitly skip or error when BRIDGE_KUBEADMIN_PASSWORD is missing by checking process.env.BRIDGE_KUBEADMIN_PASSWORD and calling setup.skip('BRIDGE_KUBEADMIN_PASSWORD not set') (matching the pattern in developer-auth.setup.ts) before using the password variable so tests are skipped with a clear message instead of attempting login with an empty string.frontend/e2e/setup/login-helper.ts (1)
18-20: ⚡ Quick winType safety issue with
window.SERVER_FLAGS.The code uses
(window as any).SERVER_FLAGS?.authDisabledwhich bypasses TypeScript type checking. Consider:
- Adding a proper type declaration for
SERVER_FLAGSin a global type file- Or using a more specific type assertion:
(window as { SERVER_FLAGS?: { authDisabled?: boolean } })🔒 Proposed fix for type safety
const authDisabled = await page - .evaluate(() => (window as any).SERVER_FLAGS?.authDisabled) + .evaluate(() => (window as { SERVER_FLAGS?: { authDisabled?: boolean } }).SERVER_FLAGS?.authDisabled) .catch(() => false);Or better yet, add a type declaration file at
e2e/types/global.d.ts:declare global { interface Window { SERVER_FLAGS?: { authDisabled?: boolean; // ... other flags }; } } export {};Then use:
.evaluate(() => window.SERVER_FLAGS?.authDisabled)🤖 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/login-helper.ts` around lines 18 - 20, The use of (window as any).SERVER_FLAGS?.authDisabled bypasses TypeScript safety; add a proper global Window type for SERVER_FLAGS (e.g., declare global { interface Window { SERVER_FLAGS?: { authDisabled?: boolean; /* other flags */ } } } export {}; ) or change the assertion to a specific shape and then update the page.evaluate call to use window.SERVER_FLAGS?.authDisabled instead of casting to any; ensure the new type file is picked up by the compiler so page.evaluate() references the typed Window and removes the any cast.
🤖 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/tests/smoke/developer/smoke-test.spec.ts`:
- Around line 5-7: The test is using
page.locator('[data-test-id="perspective-switcher-toggle"]') which bypasses the
shared Playwright testIdAttribute convention; replace that locator with
page.getByTestId('perspective-switcher-toggle') and update the expectation call
(the existing expect(...) .toContainText('Developer', { timeout: 60_000 })
should be applied to the getByTestId result) so the suite uses the standardized
testId lookup everywhere (locators to change: page.locator -> page.getByTestId).
---
Nitpick comments:
In `@frontend/e2e/setup/admin-auth.setup.ts`:
- Around line 12-14: The file sets const password =
process.env.BRIDGE_KUBEADMIN_PASSWORD || '' which silently defaults to an empty
password and causes confusing failures; change the setup to explicitly skip or
error when BRIDGE_KUBEADMIN_PASSWORD is missing by checking
process.env.BRIDGE_KUBEADMIN_PASSWORD and calling
setup.skip('BRIDGE_KUBEADMIN_PASSWORD not set') (matching the pattern in
developer-auth.setup.ts) before using the password variable so tests are skipped
with a clear message instead of attempting login with an empty string.
In `@frontend/e2e/setup/login-helper.ts`:
- Around line 18-20: The use of (window as any).SERVER_FLAGS?.authDisabled
bypasses TypeScript safety; add a proper global Window type for SERVER_FLAGS
(e.g., declare global { interface Window { SERVER_FLAGS?: { authDisabled?:
boolean; /* other flags */ } } } export {}; ) or change the assertion to a
specific shape and then update the page.evaluate call to use
window.SERVER_FLAGS?.authDisabled instead of casting to any; ensure the new type
file is picked up by the compiler so page.evaluate() references the typed Window
and removes the any cast.
🪄 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: 35715610-fdb6-4c00-aa71-1f25d29b2815
📒 Files selected for processing (17)
.claude/migration-context.mdTESTING.mdfrontend/.eslintignorefrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/fixtures/index.tsfrontend/e2e/global.setup.tsfrontend/e2e/global.teardown.tsfrontend/e2e/package.jsonfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/tsconfig.jsonfrontend/package.jsonfrontend/playwright.config.ts
💤 Files with no reviewable changes (2)
- frontend/e2e/global.teardown.ts
- frontend/e2e/global.setup.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx}: Never import from package index files (barrel imports) like@console/sharedin new code, as they create circular dependencies and slow builds. Import from specific file paths instead.
Never use backticks in i18n t() function calls as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Never import from deprecated packages or use code marked with@deprecatedTSdoc tag in new code.
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.ts
frontend/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths. The console runs behind a proxy under an arbitrary path.
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/tsconfig.jsonfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/package.jsonfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.tsfrontend/package.json
**/*
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files (to avoid git issues with case-insensitive file systems)
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/tsconfig.jsonfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/package.jsonfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsTESTING.mdfrontend/playwright.config.tsfrontend/package.json
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code; the app should be able to run behind a proxy under an arbitrary path
Use PascalCase for component file names, kebab-case for utility files, and*.spec.ts(x)for test filesUse camelCase for variable names in TypeScript and JavaScript files
**/*.{ts,tsx,js,jsx}: Any usage of i18next'sTFunction(rather than react-i18next'sTFunction) must be performed inside a function or component.
Don't use backticks inside of aTFunction. Our code parser will not automatically pick up the keys that contain backticks. Use single or double quotes instead.
Specify possible static values in comments for dynamic i18next keys that can't be interpolated by i18next-parser, such ast(key),t('key' + id), ort(key${id}).
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx}: Prefer functional programming patterns and immutable data structures in TypeScript/JavaScript
Use React functional components with hooks instead of class components in TypeScript
Use React hooks and Context API for state management (migrating away from legacy Redux/Immutable.js)
Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
Use k8s resource hooks for data fetching andconsoleFetchJSONfor HTTP requests in TypeScript
Place plugin routes in plugin-specific route files
Check existing types inconsole-sharedbefore creating new types
Use SCSS modules co-located with components, PatternFly design system components, and avoid any SCSS/CSS if possible
Follow WCAG 2.1 AA standards for accessibility; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers
UseuseTranslation('namespace')hook withkeyformat for translation keys in TypeScript
Use ErrorBoundary components and graceful degradation patterns for error handling in TypeScript
UseuseCallbackto memoize callbacks and prevent unnecessary re-renders in React
UseuseMemofor expensive filtering and computations to prevent re-computation on every render
UseReact.lazy()to lazy load heavy components
Avoid using theanytype in TypeScript; suggest proper type definitions instead
Check that null/undefined are properly handled in TypeScript (e.g.,string | undefined)
Verify exported types for reusable components in TypeScript
Reuse types from existing components rather than duplicating type definitions in component props
UseusePluginInfohook for plugin data in TypeScript
Avoid deprecated components; check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefixes
Use direct imports to specific files instead of barrel exports (index.ts) to avoid circular dependency cycles and improve build performance
Useimport typefor importing type...
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.ts
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
Follow internationalization guidelines as documented in INTERNATIONALIZATION.md for all frontend code
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
**/*.{ts,tsx,jsx}: Thearia-label,aria-placeholder,aria-roledescription, andaria-valuetextattributes should be internationalized.
The optionali18nKeyproperty on the react-i18next Trans component should only be used as a last resort when the parser incorrectly generates keys containing HTML tags.
Files:
frontend/e2e/fixtures/index.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/setup/developer-auth.setup.tsfrontend/e2e/tests/smoke/developer/smoke-test.spec.tsfrontend/e2e/setup/admin-auth.setup.tsfrontend/e2e/setup/cluster.setup.tsfrontend/e2e/setup/login-helper.tsfrontend/e2e/setup/teardown.setup.tsfrontend/playwright.config.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
TypeScript tests should follow a similar 'test tables' convention as used in Go where applicable
Files:
frontend/e2e/tests/smoke/developer/smoke-test.spec.ts
frontend/e2e/tests/**/*.spec.ts
📄 CodeRabbit inference engine (TESTING.md)
Use Playwright for E2E testing to validate full user workflows against a real OpenShift cluster. E2E tests live in
frontend/e2e/tests/<package>/Each Playwright test block must be self-contained: create its own resources, assert independently, and clean up via the
cleanupfixtureAlways use
page.getByTestId('x')to query[data-test="x"]in Playwright tests. If a React element only has a legacy test attribute, adddata-testto the element. Never remove legacy attributesUse
KubernetesClientfrome2e/clients/kubernetes-client.tsfor cluster interactions in Playwright tests. Never use shell commands in testsImport
testandexpectfrome2e/fixturesin Playwright tests, not from@playwright/test. Use custom fixtures that providecleanup,testConfig, andk8sClient
Files:
frontend/e2e/tests/smoke/developer/smoke-test.spec.ts
frontend/package.json
📄 CodeRabbit inference engine (README.md)
frontend/package.json: Pin frontend dependencies to exact versions without caret (^) or tilde () operators, except for) operator for version ranges@patternflypackages which should use tilde (
When upgrading@patternflypackages, run 'yarn dedupe --strategy highest' to prevent multiple versions of the same@patternflypackage from being pulled in and to avoid JavaScript heap memory issues
Files:
frontend/package.json
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-14T15:45:07.467Z
Learning: Use `gen-rtl-test` skill to generate unit tests that follow best practices and conventions
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-14T15:45:07.467Z
Learning: Use Cypress for integration tests with specialized test suites: Core Console, OLM, Dev Console, Shipwright, Web Terminal, Telemetry, Knative, Helm, and Topology
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-14T15:45:07.467Z
Learning: Cypress integration tests support headless and interactive modes
🪛 LanguageTool
TESTING.md
[style] ~155-~155: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... yarn test-playwright-developer — run only developer persona tests ## End-to-End ...
(ADVERB_REPETITION_PREMIUM)
🔇 Additional comments (23)
frontend/.eslintignore (1)
16-16: LGTM!frontend/e2e/package.json (1)
1-3: LGTM!frontend/e2e/tsconfig.json (2)
5-5: LGTM!
3-3: ⚡ Quick winThe
es2022target is justified—import.meta.dirnameis actively used in e2e setup/fixtures, which requires Node 21.2.0+.Good news:
frontend/package.jsonalready enforces>=22.x, exceeding the minimum requirement. However, consider adding a.nvmrcfile at the repository root to make the Node version requirement explicit for local development environments and CI systems.frontend/e2e/setup/login-helper.ts (3)
1-8: LGTM!
26-30: LGTM!
46-50: LGTM!frontend/e2e/setup/admin-auth.setup.ts (1)
1-8: LGTM!frontend/e2e/setup/developer-auth.setup.ts (2)
1-8: LGTM!
9-22: LGTM!.claude/migration-context.md (4)
13-18: LGTM!
21-108: LGTM!
111-258: LGTM!
422-430: LGTM!frontend/e2e/setup/cluster.setup.ts (3)
1-14: LGTM!
15-37: LGTM!
55-64: ⚡ Quick winConfig file properly excluded from version control. The
.test-config.jsonwritten at lines 55–64 contains sensitive data (authTokenandkubeConfigPath), and is already correctly listed in.gitignoreat/frontend/e2e/.test-config.json, preventing accidental exposure.frontend/e2e/fixtures/cleanup-fixture.ts (1)
40-40: LGTM!frontend/e2e/fixtures/index.ts (1)
29-29: LGTM!frontend/e2e/setup/teardown.setup.ts (1)
1-50: LGTM!TESTING.md (1)
136-157: LGTM!frontend/playwright.config.ts (1)
30-30: LGTM!Also applies to: 83-119, 125-125, 135-135
frontend/package.json (1)
57-57: LGTM!
| await expect( | ||
| page.locator('[data-test-id="perspective-switcher-toggle"]'), | ||
| ).toContainText('Developer', { timeout: 60_000 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use getByTestId instead of raw [data-test-id] selectors.
This bypasses the shared Playwright testIdAttribute convention and makes selector behavior inconsistent across suites.
Suggested fix
- await expect(
- page.locator('[data-test-id="perspective-switcher-toggle"]'),
- ).toContainText('Developer', { timeout: 60_000 });
+ await expect(page.getByTestId('perspective-switcher-toggle')).toContainText('Developer', {
+ timeout: 60_000,
+ });As per coding guidelines: "Always use page.getByTestId('x') to query [data-test="x"] in Playwright tests."
📝 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.
| await expect( | |
| page.locator('[data-test-id="perspective-switcher-toggle"]'), | |
| ).toContainText('Developer', { timeout: 60_000 }); | |
| await expect(page.getByTestId('perspective-switcher-toggle')).toContainText('Developer', { | |
| timeout: 60_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 `@frontend/e2e/tests/smoke/developer/smoke-test.spec.ts` around lines 5 - 7,
The test is using page.locator('[data-test-id="perspective-switcher-toggle"]')
which bypasses the shared Playwright testIdAttribute convention; replace that
locator with page.getByTestId('perspective-switcher-toggle') and update the
expectation call (the existing expect(...) .toContainText('Developer', {
timeout: 60_000 }) should be applied to the getByTestId result) so the suite
uses the standardized testId lookup everywhere (locators to change: page.locator
-> page.getByTestId).
@kubernetes/client-node v1.4.0 is pure ESM, which causes require() failures on CI where Playwright loads test files as CommonJS. Add e2e/package.json with "type": "module" so Playwright treats e2e files as ESM, and replace __dirname with import.meta.dirname accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add Playwright e2e testing section to TESTING.md covering test structure, selectors, page objects, and available commands. Simplify migration-context.md test selector guidance to always prefer getByTestId over legacy attribute locators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
56ef875 to
bac2e51
Compare
Override navigator.userAgent to ConsoleIntegrationTestEnvironment via context.addInitScript() so the existing INTEGRATION_TEST feature flag activates and disables the guided tour. Remove the now-redundant guided tour ConfigMap patch from setupConsoleUserSettings(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bac2e51 to
0434b12
Compare
|
/label px-approved |
|
@logonoff: 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. |
|
/retest |
|
thanks @stefanonardo 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, logonoff, stefanonardo The full list of commands accepted by this bot can be found here. The pull request process is described 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. |
Summary
require()of ESM module error on CI for@kubernetes/client-nodev1.4.0 (pure ESM) by addingfrontend/e2e/package.jsonwith"type": "module"so Playwright loads e2e files as ESM__dirname(CJS-only) withimport.meta.dirname(Node 21.2.0+) in 7 e2e filese2e/tsconfig.jsontarget toes2022and moduleResolution tobundlerConsoleIntegrationTestEnvironmentuser agent in Playwright fixtures to activate theINTEGRATION_TESTfeature flag, disabling the guided tour during testsTESTING.mdgetByTestIdDetails
ESM fix: The
@kubernetes/client-nodev1.4.0 declares"type": "module". On CI, Playwright findsfrontend/package.json(CJS default), loads test files viarequire(), and Babel transforms imports intorequire()calls. Node.js rejects therequire()of the ESM-only package. Addinge2e/package.jsonwith"type": "module"makes Playwright load e2e files as ESM, preservingimportstatements for native resolution.Integration test user agent: Override
navigator.userAgentviacontext.addInitScript()in the test fixtures toConsoleIntegrationTestEnvironment. This activates the existingINTEGRATION_TESTfeature flag which disables the guided tour (same mechanism used by Cypress).Test plan
cd frontend && npx tsc -p e2e/tsconfig.json --noEmitpassescd frontend && npx playwright test --listdiscovers all tests without errorsrequire() of ES Moduleerror)Screenshots / Recordings
N/A — no UI changes
🤖 Generated with Claude Code