4.19: OLS-3197: Migrate e2e tests from Cypress to Playwright#1997
Merged
openshift-merge-bot[bot] merged 1 commit intoMay 30, 2026
Merged
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates end-to-end testing from Cypress to Playwright: adds Playwright config, fixtures, globalSetup, and comprehensive Playwright tests; updates CI, package.json, docs, and helpers; and removes Cypress-specific files and config. ChangesCypress to Playwright Test Framework Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.tekton/integration-tests/lightspeed-console-pre-commit.yaml (1)
240-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast before handing off the Playwright exit code.
In
.tekton/integration-tests/lightspeed-console-pre-commit.yaml(run-e2e-testsstep), the shell script does not useset -euo pipefail(it only usesset +earoundnpx playwright test), and the step is markedonError: continue. As a result, failures in setup/checkout (git fetch/git checkout,npm ci,cat ${PASSWORD_PATH}, etc.) can be ignored and the pipeline later gates solely on/workspace/playwright-exit-code, which is written from the Playwright run only.🛠️ Proposed fix
script: | + #!/bin/bash + set -euo pipefail echo "COMMIT_SHA: ${COMMIT_SHA}" echo "BASE_URL: ${BASE_URL}" echo "CONSOLE_IMAGE: ${CONSOLE_IMAGE}" echo "KUBECONFIG_PATH: ${KUBECONFIG_PATH}" echo "---------------------------------------------" @@ - export LOGIN_PASSWORD=$(cat ${PASSWORD_PATH}) + export LOGIN_PASSWORD="$(cat "${PASSWORD_PATH}")" set +e npx playwright test err_status=$? + set -e echo -n "${err_status}" > /workspace/playwright-exit-code echo "---------------------------------------------" - ls ./gui_test_screenshots - mv ./gui_test_screenshots /workspace/artifacts/ - set -e + if [ -d ./gui_test_screenshots ]; then + mv ./gui_test_screenshots /workspace/artifacts/ + fi echo "Playwright exit code: ${err_status}"Also guard/remove the
ls ./gui_test_screenshotscall onceset -eis re-enabled, otherwise it can fail the step when the directory is absent.🤖 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.yaml around lines 240 - 289, The script in the run-e2e-tests step fails to fail-fast and only captures Playwright's exit code; enable strict shell failure handling by adding set -euo pipefail near the top (before git fetch/git checkout/npm ci/cat ${PASSWORD_PATH}), remove the surrounding set +e around npx playwright test, capture Playwright's exit code into err_status but still allow the script to exit non-zero (write the exit code artifact but do not mask fatal failures), and guard the ls ./gui_test_screenshots call (e.g., test -d ./gui_test_screenshots && ls ./gui_test_screenshots && mv ./gui_test_screenshots /workspace/artifacts/) so it won't break when the directory is absent; locate these changes around the run-e2e-tests step where commands like git fetch, git checkout, npm ci, cat ${PASSWORD_PATH}, npx playwright test, and the PLAYWRIGHT exit code write occur.
🤖 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 @.tekton/integration-tests/lightspeed-console-pre-commit.yaml:
- Around line 244-246: The task is leaking ephemeral cluster credentials by
running cat "${KUBECONFIG_PATH}" in the Tekton task; remove that line and stop
printing the kubeconfig contents. Replace the dump with a non-sensitive
alternative (e.g., echo the KUBECONFIG_PATH or log a redacted/confirmation
message or log only the current context name) so you do not expose credentials;
specifically edit the block that references KUBECONFIG_PATH and remove the cat
"${KUBECONFIG_PATH}" invocation.
In `@playwright.config.ts`:
- Around line 6-7: The config currently sets storageState based on
fs.existsSync(authStateFile) which causes storageState to be undefined at config
load and prevents Playwright from using the file created by globalSetup; change
the logic so that storageState is always set to authStateFile when globalSetup
is enabled (leave the existence check only to emit a warning or skip message),
and update use.storageState to reference that authStateFile unconditionally when
globalSetup is active; adjust code around the storageState variable,
authStateFile constant, and the use.storageState assignment so the existence
check only controls logging, not the value passed to Playwright.
In `@tests/support/fixtures.ts`:
- Around line 67-71: The adminCLI helper currently constructs a shell command
string and calls execSync, creating a command-injection risk; change adminCLI to
validate process.env.KUBECONFIG_PATH (ensure it exists, is a non-empty string
and optionally check path.isAbsolute or fs.existsSync) and replace the execSync
call with an argument-based invocation (e.g., execFileSync or spawnSync) that
passes the base command as the executable and the kubeconfig path and any other
args as an array (avoid interpolating into a single shell string), preserving
the timeout behavior; reference the adminCLI function and the current use of
execSync and KUBECONFIG_PATH when making the change.
In `@tests/support/global-setup.ts`:
- Around line 87-90: Replace the interpolated shell execSync call with an
argument-array execFileSync invocation: stop building a single command string
and call execFileSync('operator-sdk',
['run','bundle','--timeout=10m','--namespace', OLS_NAMESPACE, bundleImage,
'--kubeconfig', process.env.KUBECONFIG_PATH], { encoding: 'utf-8', timeout: 12 *
MINUTE }); keep the same options (encoding/timeout) and preserve the variables
referenced (OLS_NAMESPACE, bundleImage, process.env.KUBECONFIG_PATH, MINUTE) so
values are passed as separate args to avoid shell interpolation/injection.
- Around line 205-208: The code reads process.env.LOGIN_PASSWORD with a non-null
assertion; add explicit validation for required env vars (at minimum LOGIN_IDP,
LOGIN_USERNAME, LOGIN_PASSWORD) before any page interactions: collect missing
keys by checking process.env for each, and if any are missing throw a clear
Error (e.g. "Missing required env vars: LOGIN_PASSWORD") so the setup fails
fast; only then assign idp/username/password (remove the `!`) and continue.
Ensure you reference the same variables used in the snippet (idp, username,
password) so all downstream code uses validated values.
- Around line 198-253: The setup can leak the Playwright browser if any step
throws; wrap the launch/newContext/newPage and subsequent logic in a try/finally
so cleanup always runs: create browser/context/page as now, then put the
navigation/login/tour/olsButton logic inside a try block and in finally check
and persist context.storageState(STATE_FILE) only if context exists and then
close context/browser (await context.close() and await browser.close()) handling
null checks; reference the existing variables browser, context, page, STATE_FILE
and olsButton to locate where to enclose the try/finally.
- Around line 11-15: The adminCLI helper is vulnerable to shell injection and
doesn't check for a kubeconfig path; change it to fail fast if
process.env.KUBECONFIG_PATH is falsy, and stop interpolating into a shell string
by using an argument-based exec variant (e.g., execFileSync/execFile) so the
executable and each arg are passed as an array instead of a single command
string; update the adminCLI signature or call sites to provide the base
executable and args (or safely parse the command into execFileSync(executable,
[...args, '--kubeconfig', process.env.KUBECONFIG_PATH'])), and ensure
timeout/encoding options are preserved.
In `@tests/tests/lightspeed.spec.ts`:
- Around line 460-465: The current try/catch around
navigator.clipboard.readText() swallows clipboard errors and lets the test pass
after only clicking the copy button; change the catch to fail the test on
clipboard access errors or add an explicit fallback assertion: when awaiting
page.evaluate(() => navigator.clipboard.readText()) in the test, if it throws,
call fail() with the caught error (or assert an alternate copied-state
element/aria attribute) instead of an empty catch block so the test fails when
clipboard reads are not available; still keep the existing equality assertion
against MOCK_STREAMED_RESPONSE_TEXT when the read succeeds.
- Around line 102-142: The destructive cleanup in the test.afterAll hook (in
tests/tests/lightspeed.spec.ts) runs per worker and must be moved or guarded:
either move the adminCLI calls that delete OLSConfig/namespace/RBAC (the
adminCLI invocations that reference OLS_NAMESPACE and
process.env.LOGIN_USERNAME) into a single global teardown (CI post-step or
Playwright globalTeardown) or wrap the existing test.afterAll so it only runs
once (e.g., check workerIndex === 0 or another single-run flag before calling
adminCLI); ensure SKIP_OLS_SETUP logic remains and keep the same adminCLI
commands but ensure they execute only from the single global teardown or single
worker guard.
- Line 176: The test is asserting the textarea draft using toContainText which
checks element text, not the input's value; update both occurrences that
reference promptInput and PROMPT_NOT_SUBMITTED to assert the textarea's current
value by using toHaveValue on page.locator(promptInput) (e.g., await
expect(page.locator(promptInput)).toHaveValue(PROMPT_NOT_SUBMITTED)); keep the
awaited expect and replace toContainText with toHaveValue for accurate draft
persistence checks.
In `@tests/views/pages.ts`:
- Around line 22-26: The current countShouldBeWithin function takes a single
snapshot via await rows.count(), causing flakiness; change it to poll (loop)
until the rows locator ('[data-test-rows="resource-row"]' / variable rows)
returns a count within [min, max] or until a short timeout elapses (e.g., 3–5s)
with a small delay between attempts. Implement the retry logic inside
countShouldBeWithin so callers like listPage.filter.byName and
pages.goToPodDetails don't need to wait; keep using rows.count() each iteration
and then assert the final result or throw a timeout error if the condition never
becomes true.
---
Outside diff comments:
In @.tekton/integration-tests/lightspeed-console-pre-commit.yaml:
- Around line 240-289: The script in the run-e2e-tests step fails to fail-fast
and only captures Playwright's exit code; enable strict shell failure handling
by adding set -euo pipefail near the top (before git fetch/git checkout/npm
ci/cat ${PASSWORD_PATH}), remove the surrounding set +e around npx playwright
test, capture Playwright's exit code into err_status but still allow the script
to exit non-zero (write the exit code artifact but do not mask fatal failures),
and guard the ls ./gui_test_screenshots call (e.g., test -d
./gui_test_screenshots && ls ./gui_test_screenshots && mv ./gui_test_screenshots
/workspace/artifacts/) so it won't break when the directory is absent; locate
these changes around the run-e2e-tests step where commands like git fetch, git
checkout, npm ci, cat ${PASSWORD_PATH}, npx playwright test, and the PLAYWRIGHT
exit code write occur.
🪄 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: e43ff2e5-fadb-4055-9c8f-94c27df2c246
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit.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/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 (10)
- cypress/OWNERS
- .dockerignore
- tests/views/operator-hub-page.ts
- cypress.config.ts
- cypress/tsconfig.json
- tests/tests/lightspeed-install.cy.ts
- cypress/support/commands.ts
- tests/.eslintrc.yml
- tests/views/utils.ts
- cypress/support/e2e.js
Comment on lines
+460
to
+465
| 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.
Don't let clipboard failures turn this into a no-op test.
If navigator.clipboard.readText() throws, the test still passes after only clicking the button. That makes the migrated copy-response check a false positive in the environments where clipboard behavior is most likely to regress. Either fail on the read error or assert an alternate copied-state signal in the fallback path.
🤖 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 460 - 465, The current try/catch
around navigator.clipboard.readText() swallows clipboard errors and lets the
test pass after only clicking the copy button; change the catch to fail the test
on clipboard access errors or add an explicit fallback assertion: when awaiting
page.evaluate(() => navigator.clipboard.readText()) in the test, if it throws,
call fail() with the caught error (or assert an alternate copied-state
element/aria attribute) instead of an empty catch block so the test fails when
clipboard reads are not available; still keep the existing equality assertion
against MOCK_STREAMED_RESPONSE_TEXT when the read succeeds.
kyoto
force-pushed
the
4.19-tests-playwright
branch
from
8dec114 to
611346b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/README.md`:
- Line 34: Update the two compound adjectives in the README: change the phrase
"flexy installed clusters" to "flexy-installed clusters" and change "attachment
related tests" to "attachment-related tests" so the compounds are hyphenated for
clarity; locate these exact phrases in the tests/README.md content and replace
them accordingly.
In `@tests/support/fixtures.ts`:
- Around line 73-107: interceptQuery (and mirror in interceptFeedback) currently
mixes Playwright route registration with the "request observed" promise causing
races; refactor interceptQuery to separate the awaited route setup from the
resolver promise by (1) creating a setupPromise that performs await
page.unroute(pattern) and awaits page.route(...) registration completion, and
(2) keeping the existing Promise.withResolvers promise (the requestSeen promise)
that you resolve()/reject() inside the route handler after assertions and
route.fulfill; return both the setupPromise and the requestSeen promise (e.g. {
setup, seen }) so callers can await setup before triggering the UI, and apply
the same change to interceptFeedback, keeping existing symbols like
Promise.withResolvers, pattern, route.fulfill, resolve/reject and preserving the
assertion logic inside the handler.
In `@tests/support/global-setup.ts`:
- Around line 18-21: Resolve the login username once and reuse it for both RBAC
grants and browser login: compute a single variable (e.g., resolvedUsername)
from process.env.LOGIN_USERNAME with the same fallback used later (kubeadmin)
and replace direct uses of process.env.LOGIN_USERNAME in the adminCLI calls (and
the later browser-login/fallback logic around kubeadmin) so both the role
bindings (adminCLI(... add-cluster-role-to-user ...)) and the browser login use
the exact same resolvedUsername value.
- Around line 161-179: Replace the shell-constructed execSync invocations that
interpolate process.env.KUBECONFIG_PATH and echo OLS_CONFIG_YAML with
argument-based process calls: stop using backtick/bash -c strings in the block
around execSync and instead call child_process.spawnSync/execFileSync (or
execSync with an args array) passing ['get', OLS_CONFIG_KIND, OLS_CONFIG_NAME,
'--kubeconfig', process.env.KUBECONFIG_PATH'] for the polling loop (implement
the wait with a JS loop and sleep, not a bash until), and send OLS_CONFIG_YAML
to oc create -f - via the child process stdin (e.g., spawnSync/execFileSync with
args ['create','-f','-','--kubeconfig', process.env.KUBECONFIG_PATH'] and pass
OLS_CONFIG_YAML as input) so no shell interpolation or echo piping is used; keep
references to execSync, OLS_CONFIG_YAML, OLS_CONFIG_KIND, OLS_CONFIG_NAME, and
KUBECONFIG_PATH to locate the changes.
🪄 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: 14a10bdf-0f05-4f65-8556-20fad059cf9e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit.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/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 (10)
- tests/.eslintrc.yml
- cypress/support/e2e.js
- cypress.config.ts
- tests/views/utils.ts
- tests/views/operator-hub-page.ts
- cypress/tsconfig.json
- cypress/OWNERS
- tests/tests/lightspeed-install.cy.ts
- cypress/support/commands.ts
- .dockerignore
✅ Files skipped from review due to trivial changes (5)
- .cursor/skills/test/SKILL.md
- .claude/settings.json
- .gitignore
- .claude/commands/test.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/tsconfig.json
- playwright.config.ts
- package.json
- tests/views/pages.ts
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- tests/tests/lightspeed.spec.ts
Comment on lines
+161
to
+179
| execSync( | ||
| `timeout 180 bash -c 'until ! oc get ${OLS_CONFIG_KIND} ${OLS_CONFIG_NAME} --kubeconfig ${process.env.KUBECONFIG_PATH} 2>/dev/null; do echo "Waiting for deletion..."; sleep 5; done'`, | ||
| { encoding: 'utf-8', timeout: 4 * MINUTE }, | ||
| ); | ||
| configExists = false; | ||
| } else { | ||
| console.log('OLSConfig already exists. Using existing config.'); | ||
| configExists = true; | ||
| } | ||
| } catch { | ||
| configExists = false; | ||
| } | ||
|
|
||
| if (!configExists) { | ||
| console.log('Creating OLSConfig...'); | ||
| execSync( | ||
| `echo '${OLS_CONFIG_YAML}' | oc create -f - --kubeconfig ${process.env.KUBECONFIG_PATH}`, | ||
| { encoding: 'utf-8' }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the target file (in case path differs) and print the relevant sections with line numbers
FILE="tests/support/global-setup.ts"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE"
exit 1
fi
echo "---- File: $FILE (around lines 140-210) ----"
sed -n '140,210p' "$FILE" | nl -ba
echo
echo "---- All execSync occurrences in $FILE ----"
rg -n "execSync\\(" "$FILE" -n
echo
echo "---- Any occurrences of OLS_CONFIG_YAML and KUBECONFIG_PATH in $FILE ----"
rg -n "OLS_CONFIG_YAML|KUBECONFIG_KIND|OLS_CONFIG_NAME|KUBECONFIG_PATH|oc create|oc get" "$FILE"Repository: openshift/lightspeed-console
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/support/global-setup.ts"
echo "File exists? $([ -f "$FILE" ] && echo yes || echo no): $FILE"
ls -la "$FILE" 2>/dev/null || true
wc -l "$FILE" 2>/dev/null || true
echo
echo "---- Head (first 60 lines) ----"
head -n 60 "$FILE" 2>/dev/null || true
echo
echo "---- Around lines 140-210 ----"
if [ -f "$FILE" ]; then
start=120
end=230
sed -n "${start},${end}p" "$FILE" | cat -n | sed -n '1,220p'
fi
echo
echo "---- All execSync occurrences in $FILE ----"
rg -n "execSync\\(" "$FILE" || true
echo
echo "---- Any occurrences of oc create / oc apply / oc get and related vars ----"
rg -n "oc (create|apply|get)|OLS_CONFIG_YAML|KUBECONFIG_PATH|OLS_CONFIG_KIND|OLS_CONFIG_NAME" "$FILE" || trueRepository: openshift/lightspeed-console
Length of output: 8393
Avoid shell-constructed execSync for OLSConfig reconciliation.
The remaining execSync calls still build shell strings by interpolating process.env.KUBECONFIG_PATH and embedding OLS_CONFIG_YAML in echo '${OLS_CONFIG_YAML}' | oc create ..., which is quoting/injection-brittle. Use argument-based process execution and pass the manifest to oc create -f - via stdin (and similarly avoid bash -c for the deletion wait).
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 161-164: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 176-179: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 161 - 179, Replace the
shell-constructed execSync invocations that interpolate
process.env.KUBECONFIG_PATH and echo OLS_CONFIG_YAML with argument-based process
calls: stop using backtick/bash -c strings in the block around execSync and
instead call child_process.spawnSync/execFileSync (or execSync with an args
array) passing ['get', OLS_CONFIG_KIND, OLS_CONFIG_NAME, '--kubeconfig',
process.env.KUBECONFIG_PATH'] for the polling loop (implement the wait with a JS
loop and sleep, not a bash until), and send OLS_CONFIG_YAML to oc create -f -
via the child process stdin (e.g., spawnSync/execFileSync with args
['create','-f','-','--kubeconfig', process.env.KUBECONFIG_PATH'] and pass
OLS_CONFIG_YAML as input) so no shell interpolation or echo piping is used; keep
references to execSync, OLS_CONFIG_YAML, OLS_CONFIG_KIND, OLS_CONFIG_NAME, and
KUBECONFIG_PATH to locate the changes.
611346b to
93824fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/README.md`:
- Around line 40-42: The README's `UI_INSTALL` description is wrong: setting
UI_INSTALL actually causes tests/support/global-setup.ts to throw instead of
performing a UI install; update the README to remove or correct the
`UI_INSTALL=true` bullet so it does not instruct users to enable UI
installation, and instead document the actual behavior (that UI_INSTALL must be
unset/false because global-setup.ts will throw and UI-based installs are
unsupported in the current Playwright setup), referencing the env var name
`UI_INSTALL` and the file `tests/support/global-setup.ts`.
🪄 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: a2d87498-f7af-4fa5-a071-3b7bc78a9d9c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit.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/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 (10)
- tests/views/operator-hub-page.ts
- cypress.config.ts
- tests/views/utils.ts
- cypress/OWNERS
- cypress/tsconfig.json
- .dockerignore
- cypress/support/e2e.js
- cypress/support/commands.ts
- tests/tests/lightspeed-install.cy.ts
- tests/.eslintrc.yml
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- .claude/settings.json
- .cursor/skills/test/SKILL.md
- .claude/commands/test.md
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- tests/tsconfig.json
- tests/views/pages.ts
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- tests/tests/lightspeed.spec.ts
- playwright.config.ts
kyoto
force-pushed
the
4.19-tests-playwright
branch
from
93824fe to
8b5d807
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 95-151: The code may exit early after scaling
lightspeed-operator-controller-manager to 0, so wrap the CSV fetch/parse/patch
logic in a try/finally (or ensure a finally-style rollback) to always call
adminCLI to scale the deployment back to 1; locate the scale-down call
(adminCLI(`oc scale --replicas=0
deployment/lightspeed-operator-controller-manager
--namespace=${OLS_NAMESPACE}`)), the CSV operations (adminCLI(`oc get ${csvName}
...`), JSON.parse, relatedImageOps construction, and the patch call), and add a
guaranteed scale-up adminCLI(`oc scale --replicas=1
deployment/lightspeed-operator-controller-manager --namespace=${OLS_NAMESPACE}`)
in the finally/cleanup block so the operator is restored even on errors.
🪄 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: d28d8f7e-7f9f-4a25-8c15-305fb58321c9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.claude/commands/test.md.claude/settings.json.cursor/skills/test/SKILL.md.dockerignore.gitignore.tekton/integration-tests/lightspeed-console-pre-commit.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/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 (10)
- tests/views/utils.ts
- tests/views/operator-hub-page.ts
- cypress/support/e2e.js
- cypress/OWNERS
- cypress/tsconfig.json
- cypress.config.ts
- tests/tests/lightspeed-install.cy.ts
- tests/.eslintrc.yml
- cypress/support/commands.ts
- .dockerignore
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- .cursor/skills/test/SKILL.md
- tests/README.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .claude/settings.json
- package.json
- tests/tsconfig.json
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- playwright.config.ts
- tests/tests/lightspeed.spec.ts
- tests/views/pages.ts
- .claude/commands/test.md
kyoto
force-pushed
the
4.19-tests-playwright
branch
2 times, most recently
from
8b5d807 to
b372186
Compare
|
Actionable comments posted: 0 |
Member
Author
|
/retest |
kyoto
force-pushed
the
4.19-tests-playwright
branch
9 times, most recently
from
695dcf7 to
848366c
Compare
kyoto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
kyoto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
kyoto
force-pushed
the
4.19-tests-playwright
branch
from
9796d9c to
b4361c4
Compare
Contributor
|
/lgtm |
Member
Author
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kyoto 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 |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit ac86787
into
openshift:release-4.19
5 checks passed
This was referenced Jun 1, 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
3 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.
Summary by CodeRabbit
Refactor
Tests
Documentation
Chores