Pr 1691 playwright#1707
Open
sriroopar wants to merge 2 commits into
Open
Conversation
…tenant/ols-20260610-131408-000
📝 WalkthroughWalkthroughThis PR migrates the Lightspeed console E2E testing framework from Cypress to Playwright, updating two Tekton pipelines and the test-run script, and updates the ClusterServiceVersion manifest with new RHEL9 image digests and timestamp. ChangesTesting Framework Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.tekton/integration-tests/scripts/run-console-playwright-tests.sh (2)
79-81: 💤 Low valueShellCheck warnings are benign here.
The
run_playwrightfunction accepts"$@"to allow passing additional arguments if needed in the future, but currently the default invocation without arguments is intentional. ShellCheck flags this pattern (SC2119/SC2120) but it's a valid wrapper design.If you want to silence these warnings explicitly, you can either:
- Remove
"$@"from the function if extra args will never be needed- Add a shellcheck directive to suppress the warning
Either approach is acceptable; this is purely optional cleanup.
Also applies to: 84-84, 90-90
🤖 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/scripts/run-console-playwright-tests.sh around lines 79 - 81, ShellCheck is flagging the run_playwright wrapper for accepting "$@" (SC2119/SC2120); either remove the unused "$@" from the run_playwright function if you will never forward args, or explicitly suppress the warning by adding a shellcheck disable directive for SC2119/SC2120 immediately above the run_playwright function (and do the same for the other identical wrappers mentioned) so the intent is clear and the linter stops reporting a false-positive.Source: Linters/SAST tools
93-93: 💤 Low valueLegacy artifact filename retained.
The exit code is still written to
/workspace/cypress-exit-codedespite migrating to Playwright. If downstream steps (likelist-artifactsorfail-if-any-step-failed) depend on this specific filename, keeping it is fine for backward compatibility. Otherwise, consider renaming to/workspace/playwright-exit-codefor consistency with the migration.🤖 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/scripts/run-console-playwright-tests.sh at line 93, The script writes the Playwright exit code to a legacy filename; update the echo that writes err_status (the line using echo -n "${err_status}" >/workspace/cypress-exit-code) to write to /workspace/playwright-exit-code instead, and update any downstream consumers (e.g., steps like list-artifacts or fail-if-any-step-failed) to read /workspace/playwright-exit-code; if you need backward compatibility, write to both paths or leave the old path but document why it must be retained.bundle/manifests/lightspeed-operator.clusterserviceversion.yaml (1)
41-41: ⚡ Quick winConsider separating infrastructure changes from test framework changes.
This PR combines two orthogonal changes:
- Test framework migration (Cypress → Playwright)
- Infrastructure update (image snapshot refresh)
While the changes are internally consistent, combining them increases coupling:
- Reverting the Playwright migration would also revert the image updates
- Issues with new image snapshots would block the test framework work
- Debugging failures becomes more complex (is it the new images or the new test framework?)
Unless these changes are intentionally coupled (e.g., the Playwright tests require the new image versions), consider splitting into two PRs for cleaner change isolation and independent rollback capability.
Also applies to: 877-884, 892-892, 1003-1017
🤖 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 `@bundle/manifests/lightspeed-operator.clusterserviceversion.yaml` at line 41, This PR mixes two orthogonal changes—test framework migration (Cypress → Playwright) and infrastructure/image snapshot updates (e.g., the manifest createdAt and image tag entries) — please split them into two separate PRs: one that contains only the Playwright migration (test code, Playwright config, and CI/test pipeline changes) and another that contains only the infrastructure/image snapshot updates (manifest metadata like createdAt and image tag/version bumps and related YAML entries), ensuring each PR touches only the relevant files/keys so they can be reviewed, reverted, and tested independently.
🤖 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.
Nitpick comments:
In @.tekton/integration-tests/scripts/run-console-playwright-tests.sh:
- Around line 79-81: ShellCheck is flagging the run_playwright wrapper for
accepting "$@" (SC2119/SC2120); either remove the unused "$@" from the
run_playwright function if you will never forward args, or explicitly suppress
the warning by adding a shellcheck disable directive for SC2119/SC2120
immediately above the run_playwright function (and do the same for the other
identical wrappers mentioned) so the intent is clear and the linter stops
reporting a false-positive.
- Line 93: The script writes the Playwright exit code to a legacy filename;
update the echo that writes err_status (the line using echo -n "${err_status}"
>/workspace/cypress-exit-code) to write to /workspace/playwright-exit-code
instead, and update any downstream consumers (e.g., steps like list-artifacts or
fail-if-any-step-failed) to read /workspace/playwright-exit-code; if you need
backward compatibility, write to both paths or leave the old path but document
why it must be retained.
In `@bundle/manifests/lightspeed-operator.clusterserviceversion.yaml`:
- Line 41: This PR mixes two orthogonal changes—test framework migration
(Cypress → Playwright) and infrastructure/image snapshot updates (e.g., the
manifest createdAt and image tag entries) — please split them into two separate
PRs: one that contains only the Playwright migration (test code, Playwright
config, and CI/test pipeline changes) and another that contains only the
infrastructure/image snapshot updates (manifest metadata like createdAt and
image tag/version bumps and related YAML entries), ensuring each PR touches only
the relevant files/keys so they can be reviewed, reverted, and tested
independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21dbad68-7ae3-4a2b-841b-3bde38f06c2c
⛔ Files ignored due to path filters (1)
related_images.jsonis excluded by!related_images.json
📒 Files selected for processing (4)
.tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf5.yaml.tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf6.yaml.tekton/integration-tests/scripts/run-console-playwright-tests.shbundle/manifests/lightspeed-operator.clusterserviceversion.yaml
raptorsun
reviewed
Jun 10, 2026
| image: cypress/browsers:26.0.0 | ||
| image: mcr.microsoft.com/playwright:v1.60.0-noble | ||
| script: | | ||
| set -euo pipefail |
Contributor
There was a problem hiding this comment.
lets try adding
script: |
#!/bin/bash
set -euo pipefail
raptorsun
reviewed
Jun 10, 2026
| value: "$(params.bundle-image)" | ||
| image: cypress/browsers:26.0.0 | ||
| image: mcr.microsoft.com/playwright:v1.60.0-noble | ||
| script: | |
Contributor
There was a problem hiding this comment.
same here
script: |
#!/bin/bash
set -euo pipefail
sriroopar
force-pushed
the
pr-1691-playwright
branch
from
7dec37d to
ddd89a4
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.tekton/integration-tests/scripts/run-console-playwright-tests.sh (2)
49-49: ⚡ Quick winClarify the comment about
$PATH.The phrase "must not reuse $PATH" is ambiguous. Consider rewording to "must be a distinct directory; do not set to $PATH variable" for clarity.
🤖 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/scripts/run-console-playwright-tests.sh at line 49, Update the inline comment that currently reads "must not reuse $PATH (breaks browser runtime)" to a clearer phrasing that explains the requirement and the variable to avoid, e.g., "must be a distinct directory; do not set this to your $PATH variable (doing so breaks the browser runtime)"; locate and replace the existing comment containing "$PATH" in the Playwright/Chromium path note so it explicitly instructs not to use the $PATH environment variable.
79-81: ⚡ Quick winAddress shellcheck warnings about unused function parameters.
The
run_playwrightfunction declares"$@"parameters but the current invocations (lines 84, 90) don't pass any arguments, triggering SC2120/SC2119 warnings.Consider either:
- Remove
"$@"if no arguments will be passed:npx playwright test- Keep
"$@"for future flexibility and add a comment explaining the design choiceThis is flagged by shellcheck and affects maintainability.
🤖 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/scripts/run-console-playwright-tests.sh around lines 79 - 81, The helper function run_playwright currently uses "$@" but is always invoked without arguments, triggering shellcheck warnings; either remove the unused parameter by changing the function to call npx playwright test directly, or keep "$@" for future flexibility by leaving it and adding a short inline comment above run_playwright explaining why varargs are preserved (so shellcheck reviewers and maintainers understand the design choice); update all invocations (the ones at the test calls) if you remove "$@" to ensure behavior remains identical.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.tekton/integration-tests/scripts/run-console-playwright-tests.sh:
- Line 49: Update the inline comment that currently reads "must not reuse $PATH
(breaks browser runtime)" to a clearer phrasing that explains the requirement
and the variable to avoid, e.g., "must be a distinct directory; do not set this
to your $PATH variable (doing so breaks the browser runtime)"; locate and
replace the existing comment containing "$PATH" in the Playwright/Chromium path
note so it explicitly instructs not to use the $PATH environment variable.
- Around line 79-81: The helper function run_playwright currently uses "$@" but
is always invoked without arguments, triggering shellcheck warnings; either
remove the unused parameter by changing the function to call npx playwright test
directly, or keep "$@" for future flexibility by leaving it and adding a short
inline comment above run_playwright explaining why varargs are preserved (so
shellcheck reviewers and maintainers understand the design choice); update all
invocations (the ones at the test calls) if you remove "$@" to ensure behavior
remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c175049-2e22-40f5-b5ab-b8fa65ee2c65
📒 Files selected for processing (3)
.tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf5.yaml.tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf6.yaml.tekton/integration-tests/scripts/run-console-playwright-tests.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf5.yaml
- .tekton/integration-tests/pipelines/lightspeed-console-e2e-test-pipeline-pf6.yaml
|
@sriroopar: 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. |
Contributor
Author
|
/retest |
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
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.
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit