OLS-3197: Migrate e2e tests from Cypress to Playwright#2004
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
💤 Files with no reviewable changes (11)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR completes a comprehensive migration from Cypress to Playwright for end-to-end testing. The changes introduce Playwright configuration, global setup/teardown scaffolding for OpenShift cluster preparation and auth state capture, a new comprehensive test suite covering UI functionality and API integration, removal of all Cypress artifacts, and updated CI/CD pipelines and documentation. ChangesEnd-to-End Test 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: 2
🧹 Nitpick comments (1)
tests/support/fixtures.ts (1)
67-71: 💤 Low valueConsider adding a guard for missing
KUBECONFIG_PATH.The non-null assertion on
process.env.KUBECONFIG_PATH!will cause a cryptic error at runtime if the environment variable is not set. A clearer error message would help debugging.💡 Suggested improvement
-export const oc = (args: string[]): string => - execFileSync('oc', [...args, '--kubeconfig', process.env.KUBECONFIG_PATH!], { +export const oc = (args: string[]): string => { + if (!process.env.KUBECONFIG_PATH) { + throw new Error('KUBECONFIG_PATH environment variable is not set'); + } + return execFileSync('oc', [...args, '--kubeconfig', process.env.KUBECONFIG_PATH], { 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 oc helper uses process.env.KUBECONFIG_PATH! with a non-null assertion which will throw a cryptic error when KUBECONFIG_PATH is missing; update the oc function to first read and validate process.env.KUBECONFIG_PATH (referencing the oc function and process.env.KUBECONFIG_PATH) and, if it's undefined or empty, throw a descriptive Error (e.g., "KUBECONFIG_PATH environment variable is required for tests") or return a clear failure before calling execFileSync so callers get a helpful message rather than a runtime null assertion.
🤖 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 246-247: Add an explicit runtime check for the LOGIN_PASSWORD
environment variable before using the non-null assertion: read
process.env.LOGIN_PASSWORD into a local (e.g., password) and if it is undefined
or empty, throw an Error or call process.exit with a clear message like
"LOGIN_PASSWORD environment variable must be set" so the code in
tests/support/global-setup.ts fails fast instead of crashing at runtime when
using the password variable; update the block where idp and password are set to
validate password and provide actionable guidance.
- Around line 201-204: The execSync call that builds a shell string using
interpolated variables (execSync(...`timeout 180 bash -c 'until ! oc get
${OLS_CONFIG_KIND} ${OLS_CONFIG_NAME} --kubeconfig ${KUBECONFIG} ...'`)) is
vulnerable to command injection; replace it with a safe execFileSync-based
invocation (require('child_process').execFileSync) or equivalent that passes the
binary ("timeout" or "bash") and each argument as an array instead of a single
interpolated shell string, or explicitly validate/escape OLS_CONFIG_KIND,
OLS_CONFIG_NAME, and KUBECONFIG beforehand; locate the call to execSync in
tests/support/global-setup.ts and change it to use execFileSync with args array
(or validated values) and preserve the timeout option passed via the options
object.
---
Nitpick comments:
In `@tests/support/fixtures.ts`:
- Around line 67-71: The oc helper uses process.env.KUBECONFIG_PATH! with a
non-null assertion which will throw a cryptic error when KUBECONFIG_PATH is
missing; update the oc function to first read and validate
process.env.KUBECONFIG_PATH (referencing the oc function and
process.env.KUBECONFIG_PATH) and, if it's undefined or empty, throw a
descriptive Error (e.g., "KUBECONFIG_PATH environment variable is required for
tests") or return a clear failure before calling execFileSync so callers get a
helpful message rather than a runtime null assertion.
🪄 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: 29016ee3-69c2-4fcc-9e14-c2a780e25315
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.ai/spec/how/project-structure.md.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.tsrenovate.jsontests/.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
- cypress/support/e2e.js
- cypress.config.ts
- tests/.eslintrc.yml
- cypress/tsconfig.json
- .dockerignore
- tests/tests/lightspeed-install.cy.ts
- tests/views/operator-hub-page.ts
- cypress/OWNERS
- tests/views/pages.ts
- cypress/support/commands.ts
| const idp = process.env.LOGIN_IDP || 'kube:admin'; | ||
| const password = process.env.LOGIN_PASSWORD!; |
There was a problem hiding this comment.
Missing validation for LOGIN_PASSWORD environment variable.
The non-null assertion on LOGIN_PASSWORD will cause a runtime error if the variable is not set. Consider adding an early validation with a clear error message to fail fast with actionable guidance.
💡 Proposed fix
const idp = process.env.LOGIN_IDP || 'kube:admin';
- const password = process.env.LOGIN_PASSWORD!;
+ const password = process.env.LOGIN_PASSWORD;
+ if (!password) {
+ throw new Error('LOGIN_PASSWORD environment variable is required for global setup');
+ }📝 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.
| const idp = process.env.LOGIN_IDP || 'kube:admin'; | |
| const password = process.env.LOGIN_PASSWORD!; | |
| const idp = process.env.LOGIN_IDP || 'kube:admin'; | |
| const password = process.env.LOGIN_PASSWORD; | |
| if (!password) { | |
| throw new Error('LOGIN_PASSWORD environment variable is required for global setup'); | |
| } |
🤖 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 246 - 247, Add an explicit
runtime check for the LOGIN_PASSWORD environment variable before using the
non-null assertion: read process.env.LOGIN_PASSWORD into a local (e.g.,
password) and if it is undefined or empty, throw an Error or call process.exit
with a clear message like "LOGIN_PASSWORD environment variable must be set" so
the code in tests/support/global-setup.ts fails fast instead of crashing at
runtime when using the password variable; update the block where idp and
password are set to validate password and provide actionable guidance.
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. |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
New changes are detected. LGTM label has been removed. |
|
/override "Red Hat Konflux / e2e-console-pf6 / lightspeed-console" |
|
@kyoto: Overrode contexts on behalf of kyoto: Red Hat Konflux / e2e-console-pf6 / lightspeed-console DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Manual cherry pick of #1997
Summary by CodeRabbit
New Features
Documentation
Chores
Tests