PF5: OLS-3197: Migrate e2e tests from Cypress to Playwright#2005
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 1, 2026
Merged
Conversation
kyoto
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
📝 WalkthroughWalkthroughThis PR migrates end-to-end tests from Cypress to Playwright: it adds Playwright config, fixtures, global setup/teardown, and a new Playwright test suite; removes Cypress configs/support; and updates CI, scripts, and docs to use Playwright and SKIP_OLS_SETUP. ChangesEnd-to-End Testing Framework Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (1)
.tekton/integration-tests/lightspeed-console-pre-commit-417.yaml (1)
149-149: Confirm Playwright image tag and flag minor lint-image overhead
mcr.microsoft.com/playwright:v1.60.0-nobleis a published MCR tag, so the version reference is valid.- Using the Playwright image for the lint task (which doesn’t run Playwright) is unnecessary overhead; consider a smaller Node-based image for lint if you want to optimize.
🤖 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 @.tekton/integration-tests/lightspeed-console-pre-commit-417.yaml at line 149, The pipeline currently uses the Playwright container image string "image: mcr.microsoft.com/playwright:v1.60.0-noble"; confirm that tag is intentional for steps that actually run Playwright, but for the lint task replace that image with a lightweight Node image (for example node:18-alpine or node:lts-alpine) to avoid unnecessary Playwright overhead; keep the Playwright image only for jobs/steps that run browser tests and update the "image: mcr.microsoft.com/playwright:v1.60.0-noble" entry accordingly.
🤖 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 `@tests/support/fixtures.ts`:
- Around line 67-71: The helper oc currently uses a non-null assertion on
process.env.KUBECONFIG_PATH, causing opaque runtime failures; modify oc in
tests/support/fixtures.ts to validate that process.env.KUBECONFIG_PATH is set
(or read it into a validated constant) and throw a clear, early error if missing
so tests fail fast; update the oc function (and any module-level initialization)
to check the env var before calling execFileSync so calls from
tests/support/global-setup.ts and global-teardown.ts produce an explicit failure
message when KUBECONFIG_PATH is not provided.
In `@tests/support/global-setup.ts`:
- Around line 201-204: The current execSync call builds a bash -c string
interpolating KUBECONFIG and risks command injection; replace this shell loop
with safe JS polling that calls the existing oc helper (or use
execFileSync/spawnSync with argv form) so kubeconfig is passed as a normal
argument. Specifically, remove the backticked bash -c execSync invocation and
implement a loop in tests/support/global-setup.ts that repeatedly calls the oc
helper (using OLS_CONFIG_KIND and OLS_CONFIG_NAME) with KUBECONFIG passed as an
argument, sleeping between retries and timing out after the same interval; this
avoids embedding ${KUBECONFIG} in a shell string and eliminates
injection/quoting issues.
In `@tests/tests/lightspeed.spec.ts`:
- Around line 412-417: Don't swallow assertion failures by wrapping the expect
in the same try/catch; instead only guard the clipboard read. Call
navigator.clipboard.readText() inside a try and assign to clipboardText, then
perform expect(clipboardText).toBe(MOCK_STREAMED_RESPONSE_TEXT) outside that try
so assertion failures surface; alternatively, if you prefer a single catch,
rethrow any caught error that is not a clipboard-permission error (e.g., check
error.name or message for "NotAllowedError"/"NotFoundError"/"permission" and
only suppress those), referencing navigator.clipboard.readText(), clipboardText
and MOCK_STREAMED_RESPONSE_TEXT to locate the lines to change.
---
Nitpick comments:
In @.tekton/integration-tests/lightspeed-console-pre-commit-417.yaml:
- Line 149: The pipeline currently uses the Playwright container image string
"image: mcr.microsoft.com/playwright:v1.60.0-noble"; confirm that tag is
intentional for steps that actually run Playwright, but for the lint task
replace that image with a lightweight Node image (for example node:18-alpine or
node:lts-alpine) to avoid unnecessary Playwright overhead; keep the Playwright
image only for jobs/steps that run browser tests and update the "image:
mcr.microsoft.com/playwright:v1.60.0-noble" entry accordingly.
🪄 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: 574d04e5-434c-471b-8f8a-0658cdf0793e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit-417.yamlAGENTS.mdcypress.config.tscypress/OWNERScypress/support/commands.tscypress/support/e2e.jscypress/tsconfig.jsonpackage.jsonplaywright.config.tstests/.eslintrc.ymltests/README.mdtests/support/fixtures.tstests/support/global-setup.tstests/support/global-teardown.tstests/tests/lightspeed-install.cy.tstests/tests/lightspeed.spec.tstests/tsconfig.jsontests/views/operator-hub-page.tstests/views/pages.tstests/views/utils.ts
💤 Files with no reviewable changes (11)
- tests/tests/lightspeed-install.cy.ts
- tests/.eslintrc.yml
- tests/views/utils.ts
- tests/views/operator-hub-page.ts
- tests/views/pages.ts
- cypress/OWNERS
- cypress.config.ts
- cypress/tsconfig.json
- .dockerignore
- cypress/support/e2e.js
- cypress/support/commands.ts
Comment on lines
+67
to
+71
| export const oc = (args: string[]): string => | ||
| execFileSync('oc', [...args, '--kubeconfig', process.env.KUBECONFIG_PATH!], { | ||
| encoding: 'utf-8', | ||
| timeout: 180_000, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unguarded kubeconfig env assertions in e2e support/setup files
rg -n -C2 'KUBECONFIG_PATH!' tests/support/fixtures.ts tests/support/global-setup.tsRepository: openshift/lightspeed-console
Length of output: 389
🏁 Script executed:
#!/bin/bash
rg -n --hidden -S "from ['\"].*fixtures['\"]|import .*oc|export const oc|\\boc\\(" tests/support tests | head -n 200Repository: openshift/lightspeed-console
Length of output: 4078
Fail fast when KUBECONFIG_PATH is missing (tests e2e reliability).
tests/support/fixtures.ts uses a non-null assertion (process.env.KUBECONFIG_PATH!) in oc, and both tests/support/global-setup.ts and tests/support/global-teardown.ts call oc(...), so an unset env var causes a late/opaque failure for the whole e2e run.
Proposed fix
export const oc = (args: string[]): string =>
- execFileSync('oc', [...args, '--kubeconfig', process.env.KUBECONFIG_PATH!], {
- encoding: 'utf-8',
- timeout: 180_000,
- });
+ {
+ const kubeconfigPath = process.env.KUBECONFIG_PATH;
+ if (!kubeconfigPath) {
+ throw new Error('KUBECONFIG_PATH must be set for Playwright e2e setup');
+ }
+ return execFileSync('oc', [...args, '--kubeconfig', kubeconfigPath], {
+ encoding: 'utf-8',
+ timeout: 180_000,
+ });
+ };📝 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.
Suggested change
| export const oc = (args: string[]): string => | |
| execFileSync('oc', [...args, '--kubeconfig', process.env.KUBECONFIG_PATH!], { | |
| encoding: 'utf-8', | |
| timeout: 180_000, | |
| }); | |
| export const oc = (args: string[]): string => | |
| { | |
| const kubeconfigPath = process.env.KUBECONFIG_PATH; | |
| if (!kubeconfigPath) { | |
| throw new Error('KUBECONFIG_PATH must be set for Playwright e2e setup'); | |
| } | |
| return execFileSync('oc', [...args, '--kubeconfig', kubeconfigPath], { | |
| encoding: 'utf-8', | |
| timeout: 180_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 `@tests/support/fixtures.ts` around lines 67 - 71, The helper oc currently uses
a non-null assertion on process.env.KUBECONFIG_PATH, causing opaque runtime
failures; modify oc in tests/support/fixtures.ts to validate that
process.env.KUBECONFIG_PATH is set (or read it into a validated constant) and
throw a clear, early error if missing so tests fail fast; update the oc function
(and any module-level initialization) to check the env var before calling
execFileSync so calls from tests/support/global-setup.ts and global-teardown.ts
produce an explicit failure message when KUBECONFIG_PATH is not provided.
Comment on lines
+412
to
+417
| try { | ||
| const clipboardText = await page.evaluate(() => navigator.clipboard.readText()); | ||
| expect(clipboardText).toBe(MOCK_STREAMED_RESPONSE_TEXT); | ||
| } catch { | ||
| // Clipboard access may be denied in headless mode | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad catch blocks around clipboard assertions in Playwright tests
rg -n -C3 'clipboard\.readText\(\)|catch \{' tests/tests/lightspeed.spec.tsRepository: openshift/lightspeed-console
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Inspect surrounding Playwright test code for the clipboard assertion to ensure the catch also swallows expect failures.
sed -n '360,460p' tests/tests/lightspeed.spec.tsRepository: openshift/lightspeed-console
Length of output: 4496
🏁 Script executed:
#!/bin/bash
# Inspect surrounding Playwright test code for the clipboard assertion to ensure the catch also swallows expect failures.
sed -n '380,450p' tests/tests/lightspeed.spec.tsRepository: openshift/lightspeed-console
Length of output: 3292
🏁 Script executed:
#!/bin/bash
# Inspect surrounding Playwright test code for the clipboard assertion to ensure the catch also swallows expect failures.
sed -n '380,450p' tests/tests/lightspeed.spec.tsRepository: openshift/lightspeed-console
Length of output: 3292
Stop swallowing clipboard assertion failures.
In tests/tests/lightspeed.spec.ts the broad try/catch wraps expect(clipboardText).toBe(...), so if the assertion fails (not just clipboard-read access), the test still passes.
Proposed fix
- try {
- const clipboardText = await page.evaluate(() => navigator.clipboard.readText());
- expect(clipboardText).toBe(MOCK_STREAMED_RESPONSE_TEXT);
- } catch {
- // Clipboard access may be denied in headless mode
- }
+ const clipboardRead = await page.evaluate(async () => {
+ try {
+ return { ok: true as const, text: await navigator.clipboard.readText() };
+ } catch (error) {
+ return { ok: false as const, message: String(error) };
+ }
+ });
+ if (clipboardRead.ok) {
+ expect(clipboardRead.text).toBe(MOCK_STREAMED_RESPONSE_TEXT);
+ }📝 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.
Suggested change
| try { | |
| const clipboardText = await page.evaluate(() => navigator.clipboard.readText()); | |
| expect(clipboardText).toBe(MOCK_STREAMED_RESPONSE_TEXT); | |
| } catch { | |
| // Clipboard access may be denied in headless mode | |
| } | |
| const clipboardRead = await page.evaluate(async () => { | |
| try { | |
| return { ok: true as const, text: await navigator.clipboard.readText() }; | |
| } catch (error) { | |
| return { ok: false as const, message: String(error) }; | |
| } | |
| }); | |
| if (clipboardRead.ok) { | |
| expect(clipboardRead.text).toBe(MOCK_STREAMED_RESPONSE_TEXT); | |
| } |
🤖 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 `@tests/tests/lightspeed.spec.ts` around lines 412 - 417, Don't swallow
assertion failures by wrapping the expect in the same try/catch; instead only
guard the clipboard read. Call navigator.clipboard.readText() inside a try and
assign to clipboardText, then perform
expect(clipboardText).toBe(MOCK_STREAMED_RESPONSE_TEXT) outside that try so
assertion failures surface; alternatively, if you prefer a single catch, rethrow
any caught error that is not a clipboard-permission error (e.g., check
error.name or message for "NotAllowedError"/"NotFoundError"/"permission" and
only suppress those), referencing navigator.clipboard.readText(), clipboardText
and MOCK_STREAMED_RESPONSE_TEXT to locate the lines to change.
|
@kyoto: This pull request references OLS-3197 which is a valid jira issue. 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
jira/valid-reference
Co-authored-by: Cursor <cursoragent@cursor.com>
kyoto
force-pushed
the
pf5-tests-playwright
branch
from
f736d07 to
aaa88b0
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/support/global-setup.ts`:
- Around line 111-122: The code extracts csvName via the oc(...) chain and
assumes it exists; if the CSV is not yet present csvName can be undefined and
later oc calls will fail. After the extraction of csvName (the variable assigned
from the oc(...).trim().split('\n').filter(Boolean)[0] expression) add a guard:
if (!csvName) either retry with a short backoff loop until a timeout or
throw/log a clear error and exit the setup flow so the later oc patch using
csvName is never invoked with an invalid value; ensure any retry uses the same
oc command and includes a timeout to avoid infinite loops.
- Around line 14-17: The setup reads required runtime inputs (baseURL from
config.projects[0].use.baseURL, username from process.env.LOGIN_USERNAME, and
KUBECONFIG from process.env.KUBECONFIG_PATH) without validating them; add
explicit guards at the start of the global-setup flow to validate these values
and throw or log a clear, actionable error if any are missing or empty. Locate
the initialization of baseURL, username, and KUBECONFIG in
tests/support/global-setup.ts (and the later usage around lines ~252–254) and
replace the silent assumptions with checks that: assert/throw with a descriptive
message if baseURL is falsy, assert/throw if KUBECONFIG_PATH is missing, and
optionally default username only after logging a warning or require it via an
env check so failures surface immediately.
🪄 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: 81897967-56ee-4ecf-a024-11ab71aed93e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit-417.yamlAGENTS.mdcypress.config.tscypress/OWNERScypress/support/commands.tscypress/support/e2e.jscypress/tsconfig.jsonpackage.jsonplaywright.config.tstests/.eslintrc.ymltests/README.mdtests/support/fixtures.tstests/support/global-setup.tstests/support/global-teardown.tstests/tests/lightspeed-install.cy.tstests/tests/lightspeed.spec.tstests/tsconfig.jsontests/views/operator-hub-page.tstests/views/pages.tstests/views/utils.ts
💤 Files with no reviewable changes (11)
- tests/views/utils.ts
- .dockerignore
- tests/.eslintrc.yml
- cypress/OWNERS
- cypress/support/e2e.js
- cypress.config.ts
- tests/views/pages.ts
- cypress/tsconfig.json
- tests/views/operator-hub-page.ts
- tests/tests/lightspeed-install.cy.ts
- cypress/support/commands.ts
✅ Files skipped from review due to trivial changes (4)
- .cursor/skills/test/SKILL.md
- .gitignore
- tests/README.md
- .claude/commands/test.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .claude/settings.json
- tests/tsconfig.json
- playwright.config.ts
- tests/support/global-teardown.ts
- tests/support/fixtures.ts
- package.json
- .tekton/integration-tests/lightspeed-console-pre-commit-417.yaml
- tests/tests/lightspeed.spec.ts
Comment on lines
+14
to
+17
| const baseURL = config.projects[0].use.baseURL!; | ||
| const username = process.env.LOGIN_USERNAME || 'kubeadmin'; | ||
| const KUBECONFIG = process.env.KUBECONFIG_PATH; | ||
|
|
There was a problem hiding this comment.
Validate required runtime inputs before setup steps start.
Line 14, Line 16, and Line 253 rely on required values without an explicit guard. When one is missing, later failures are indirect (oc/login steps) and hard to diagnose.
Suggested fix
const globalSetup = async (config: FullConfig) => {
- const baseURL = config.projects[0].use.baseURL!;
+ const requireEnv = (name: string): string => {
+ const value = process.env[name];
+ if (!value) {
+ throw new Error(`Missing required environment variable: ${name}`);
+ }
+ return value;
+ };
+
+ const baseURL = config.projects[0]?.use?.baseURL;
+ if (!baseURL) {
+ throw new Error('Missing Playwright baseURL in project configuration.');
+ }
const username = process.env.LOGIN_USERNAME || 'kubeadmin';
- const KUBECONFIG = process.env.KUBECONFIG_PATH;
+ const kubeconfig = requireEnv('KUBECONFIG_PATH');
@@
- KUBECONFIG!,
+ kubeconfig,
@@
- const password = process.env.LOGIN_PASSWORD!;
+ const password = requireEnv('LOGIN_PASSWORD');Also applies to: 252-254
🤖 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 `@tests/support/global-setup.ts` around lines 14 - 17, The setup reads required
runtime inputs (baseURL from config.projects[0].use.baseURL, username from
process.env.LOGIN_USERNAME, and KUBECONFIG from process.env.KUBECONFIG_PATH)
without validating them; add explicit guards at the start of the global-setup
flow to validate these values and throw or log a clear, actionable error if any
are missing or empty. Locate the initialization of baseURL, username, and
KUBECONFIG in tests/support/global-setup.ts (and the later usage around lines
~252–254) and replace the silent assumptions with checks that: assert/throw with
a descriptive message if baseURL is falsy, assert/throw if KUBECONFIG_PATH is
missing, and optionally default username only after logging a warning or require
it via an env check so failures surface immediately.
Comment on lines
+111
to
+122
| const csvName = oc([ | ||
| 'get', | ||
| 'clusterserviceversion', | ||
| '--namespace', | ||
| OLS_NAMESPACE, | ||
| '-o', | ||
| 'name', | ||
| ]) | ||
| .trim() | ||
| .split('\n') | ||
| .filter(Boolean)[0]; | ||
|
|
There was a problem hiding this comment.
Guard against empty CSV discovery before patching.
Line 121 can resolve to undefined when the CSV is not yet present; Line 131 then calls oc with an invalid resource name. This makes CONSOLE_IMAGE flows flaky.
Suggested fix
- const csvName = oc([
- 'get',
- 'clusterserviceversion',
- '--namespace',
- OLS_NAMESPACE,
- '-o',
- 'name',
- ])
- .trim()
- .split('\n')
- .filter(Boolean)[0];
+ let csvName: string | undefined;
+ const csvDeadline = Date.now() + 3 * MINUTE;
+ while (!csvName && Date.now() < csvDeadline) {
+ csvName = oc([
+ 'get',
+ 'clusterserviceversion',
+ '--namespace',
+ OLS_NAMESPACE,
+ '-o',
+ 'name',
+ ])
+ .trim()
+ .split('\n')
+ .filter(Boolean)[0];
+
+ if (!csvName) {
+ await new Promise((resolve) => setTimeout(resolve, 5_000));
+ }
+ }
+
+ if (!csvName) {
+ throw new Error(`Timed out waiting for CSV in namespace ${OLS_NAMESPACE}.`);
+ }Also applies to: 131-131
🤖 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 `@tests/support/global-setup.ts` around lines 111 - 122, The code extracts
csvName via the oc(...) chain and assumes it exists; if the CSV is not yet
present csvName can be undefined and later oc calls will fail. After the
extraction of csvName (the variable assigned from the
oc(...).trim().split('\n').filter(Boolean)[0] expression) add a guard: if
(!csvName) either retry with a short backoff loop until a timeout or throw/log a
clear error and exit the setup flow so the later oc patch using csvName is never
invoked with an invalid value; ensure any retry uses the same oc command and
includes a timeout to avoid infinite loops.
openshift-merge-bot
Bot
merged commit 534c899
into
openshift:pattern-fly-5
5 checks passed
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.
Manual cherry pick of #1997
Summary by CodeRabbit
New Features
Changes
Documentation