CONSOLE-5197: Add Playwright E2E test infrastructure for Prow/CI#16374
CONSOLE-5197: Add Playwright E2E test infrastructure for Prow/CI#16374vikram-raj wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vikram-raj: This pull request references CONSOLE-5197 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vikram-raj 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 |
📝 WalkthroughWalkthroughTwo new executable shell scripts are introduced to standardize Playwright E2E test execution. 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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.
🧹 Nitpick comments (3)
frontend/integration-tests/test-playwright-e2e.sh (1)
49-54: Declare and assign separately to avoid masking return values.Per ShellCheck SC2155: combining
exportwith command substitution masks the exit status ofcat. While the file existence check mitigates risk, separating declaration and assignment is the more robust pattern.♻️ Proposed fix
if [ -z "${BRIDGE_KUBEADMIN_PASSWORD:-}" ]; then pass_file="${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password}" if [ -f "$pass_file" ]; then - export BRIDGE_KUBEADMIN_PASSWORD="$(cat "$pass_file")" + BRIDGE_KUBEADMIN_PASSWORD="$(cat "$pass_file")" + export BRIDGE_KUBEADMIN_PASSWORD fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/integration-tests/test-playwright-e2e.sh` around lines 49 - 54, The existing combined export with command substitution masks cat's exit status; change the BRIDGE_KUBEADMIN_PASSWORD assignment to be done in two steps: set a local variable (e.g., tmp_pass) by running cat "$pass_file" and check its exit status if desired, then export BRIDGE_KUBEADMIN_PASSWORD="$tmp_pass". Update the block that computes pass_file (using KUBEADMIN_PASSWORD_FILE and INSTALLER_DIR) and replace the single-line export with this two-step assignment so BRIDGE_KUBEADMIN_PASSWORD, pass_file, KUBEADMIN_PASSWORD_FILE and INSTALLER_DIR are used but command substitution is not performed inside export.test-prow-playwright-e2e.sh (2)
26-27: Minor inconsistency in parameter expansion operators.Line 26 uses
:-(substitute only) while line 27 uses:=(set and substitute). This inconsistency could cause subtle issues ifINSTALLER_DIRis referenced later without a default. For consistency with line 26 and the pattern intest-playwright-e2e.sh, consider using:-.♻️ Proposed fix for consistency
ARTIFACT_DIR=${ARTIFACT_DIR:-/tmp/artifacts} -INSTALLER_DIR=${INSTALLER_DIR:=${ARTIFACT_DIR}/installer} +INSTALLER_DIR=${INSTALLER_DIR:-${ARTIFACT_DIR}/installer}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-prow-playwright-e2e.sh` around lines 26 - 27, The assignment uses inconsistent parameter expansion: change INSTALLER_DIR from using := to :- to avoid assigning into the environment (use the same pattern as ARTIFACT_DIR); specifically update the INSTALLER_DIR expansion so INSTALLER_DIR=${INSTALLER_DIR:-${ARTIFACT_DIR}/installer} (referencing the INSTALLER_DIR and ARTIFACT_DIR variables) to ensure it only substitutes a default without mutating the variable.
29-33: Declare and assign separately to avoid masking return values.Per ShellCheck SC2155: combining
exportwith command substitution masks the exit status ofcatandoc get. Whileset -ewill still catch downstream failures, separating declaration and assignment makes error attribution clearer.Additionally, unlike
test-playwright-e2e.sh, this script doesn't guard against a missing password file—if it's absent, the script fails immediately. This is likely intentional for CI, but worth documenting explicitly.♻️ Proposed fix
# don't log kubeadmin-password set +x -export BRIDGE_KUBEADMIN_PASSWORD="$(cat "${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password}")" +BRIDGE_KUBEADMIN_PASSWORD="$(cat "${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password}")" +export BRIDGE_KUBEADMIN_PASSWORD set -x -export BRIDGE_BASE_ADDRESS="$(oc get consoles.config.openshift.io cluster -o jsonpath='{.status.consoleURL}')" +BRIDGE_BASE_ADDRESS="$(oc get consoles.config.openshift.io cluster -o jsonpath='{.status.consoleURL}')" +export BRIDGE_BASE_ADDRESS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-prow-playwright-e2e.sh` around lines 29 - 33, Split the combined export+command-substitution assignments so the exit status of cat/oc is not masked: first assign BRIDGE_KUBEADMIN_PASSWORD="$(cat "${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password"}")" to a variable (without export), check the command exit status or test file existence (e.g. guard that the kubeadmin password file exists) and then export BRIDGE_KUBEADMIN_PASSWORD; do the same for BRIDGE_BASE_ADDRESS by assigning the output of oc get consoles.config.openshift.io ... to a variable, verify the command succeeded, and then export BRIDGE_BASE_ADDRESS; reference BRIDGE_KUBEADMIN_PASSWORD, KUBEADMIN_PASSWORD_FILE, INSTALLER_DIR, BRIDGE_BASE_ADDRESS and the oc get consoles.config.openshift.io call when locating the lines to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/integration-tests/test-playwright-e2e.sh`:
- Around line 49-54: The existing combined export with command substitution
masks cat's exit status; change the BRIDGE_KUBEADMIN_PASSWORD assignment to be
done in two steps: set a local variable (e.g., tmp_pass) by running cat
"$pass_file" and check its exit status if desired, then export
BRIDGE_KUBEADMIN_PASSWORD="$tmp_pass". Update the block that computes pass_file
(using KUBEADMIN_PASSWORD_FILE and INSTALLER_DIR) and replace the single-line
export with this two-step assignment so BRIDGE_KUBEADMIN_PASSWORD, pass_file,
KUBEADMIN_PASSWORD_FILE and INSTALLER_DIR are used but command substitution is
not performed inside export.
In `@test-prow-playwright-e2e.sh`:
- Around line 26-27: The assignment uses inconsistent parameter expansion:
change INSTALLER_DIR from using := to :- to avoid assigning into the environment
(use the same pattern as ARTIFACT_DIR); specifically update the INSTALLER_DIR
expansion so INSTALLER_DIR=${INSTALLER_DIR:-${ARTIFACT_DIR}/installer}
(referencing the INSTALLER_DIR and ARTIFACT_DIR variables) to ensure it only
substitutes a default without mutating the variable.
- Around line 29-33: Split the combined export+command-substitution assignments
so the exit status of cat/oc is not masked: first assign
BRIDGE_KUBEADMIN_PASSWORD="$(cat
"${KUBEADMIN_PASSWORD_FILE:-${INSTALLER_DIR}/auth/kubeadmin-password"}")" to a
variable (without export), check the command exit status or test file existence
(e.g. guard that the kubeadmin password file exists) and then export
BRIDGE_KUBEADMIN_PASSWORD; do the same for BRIDGE_BASE_ADDRESS by assigning the
output of oc get consoles.config.openshift.io ... to a variable, verify the
command succeeded, and then export BRIDGE_BASE_ADDRESS; reference
BRIDGE_KUBEADMIN_PASSWORD, KUBEADMIN_PASSWORD_FILE, INSTALLER_DIR,
BRIDGE_BASE_ADDRESS and the oc get consoles.config.openshift.io call when
locating the lines to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 377bba5c-95b1-42b3-bcf9-a9d6a82af102
📒 Files selected for processing (2)
frontend/integration-tests/test-playwright-e2e.shtest-prow-playwright-e2e.sh
📜 Review details
🧰 Additional context used
🪛 Shellcheck (0.11.0)
frontend/integration-tests/test-playwright-e2e.sh
[warning] 52-52: Declare and assign separately to avoid masking return values.
(SC2155)
test-prow-playwright-e2e.sh
[warning] 31-31: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 33-33: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (6)
frontend/integration-tests/test-playwright-e2e.sh (4)
1-43: LGTM on script header and initialization.The documentation is thorough, strict bash options (
set -euo pipefail) are correctly applied, and thefrontend/working directory enforcement is a sensible guard. TheNO_COLORhandling for CI environments aligns with the existing Cypress test patterns.
56-72: LGTM on BRIDGE_BASE_ADDRESS derivation.Good defensive checks: verifying
ocavailability before use, validating the query result is non-empty, and providing actionable error messages. The JSONPath query forconsoles.config.openshift.io/clusteris correct for fetching the console URL.
74-93: LGTM on argument parsing and dependency handling.The
getoptshandling is clean, the usage message covers the expected interface, and the conditionalyarn installguard avoids redundant installs. Thecreate-user.shinvocation correctly usesREPO_ROOTfor the absolute path.
95-120: LGTM on path validation and Playwright execution.The directory traversal prevention (lines 102-105) is well-implemented: it resolves the path canonically before checking containment, handling both exact matches and subdirectories. The fallback to
frontend/node_modulesfor the Playwright binary is a pragmatic approach for monorepo subdirectory packages. Usingexecto replace the shell process is efficient.test-prow-playwright-e2e.sh (2)
35-51: LGTM on user creation and scenario dispatch.The
create-user.shinvocation is safe since the script is idempotent (it checks for existing htpasswd secret). Scenario handling is clean with explicit error on unknown values. Thee2e/tests/smokepath argument for the smoke scenario correctly passes toplaywright test.
53-55: LGTM on CSP check and directory cleanup.The
NO_SANDBOX=true yarn test-puppeteer-cspfollows the established pattern from Cypress Prow tests. Thepopdcorrectly balances the earlierpushd.
|
/cc @jhadvig |
| # Environment: | ||
| # BRIDGE_BASE_ADDRESS If unset, from oc get consoles.config.openshift.io cluster | ||
| # BRIDGE_BASE_PATH Default / | ||
| # PLAYWRIGHT_BASE_URL Default ${BRIDGE_BASE_ADDRESS}${BRIDGE_BASE_PATH} |
There was a problem hiding this comment.
rename to WEB_CONSOLE_URL
| # | ||
| # Usage (always from repo frontend/): | ||
| # ./integration-tests/test-playwright-e2e.sh [playwright test arguments...] | ||
| # ./integration-tests/test-playwright-e2e.sh -d packages/my-plugin/e2e tests/smoke.spec.ts |
There was a problem hiding this comment.
we're going to set up a Playwright project for each package, so we won't need this option. The script will pass --project=my-plugin to Playwright.
maybe we can replace this option with -n <project-name>
| # BRIDGE_BASE_PATH Default / | ||
| # PLAYWRIGHT_BASE_URL Default ${BRIDGE_BASE_ADDRESS}${BRIDGE_BASE_PATH} | ||
| # INSTALLER_DIR, ARTIFACT_DIR, KUBEADMIN_PASSWORD_FILE Kubeadmin password for login flows | ||
| # OPENSHIFT_CI If true, sets NO_COLOR=1 (same as test-cypress.sh) |
| # BRIDGE_BASE_PATH Default / | ||
| # PLAYWRIGHT_BASE_URL Default ${BRIDGE_BASE_ADDRESS}${BRIDGE_BASE_PATH} | ||
| # INSTALLER_DIR, ARTIFACT_DIR, KUBEADMIN_PASSWORD_FILE Kubeadmin password for login flows | ||
| # OPENSHIFT_CI If true, sets NO_COLOR=1 (same as test-cypress.sh) |
There was a problem hiding this comment.
let's set CI=true as Playwright will recognize it and use CI ad-hoc settings
| if [ "$OPENSHIFT_CI" = true ]; then | ||
| export NO_COLOR=1 | ||
| fi |
There was a problem hiding this comment.
not need to set NO_COLOR, it'll be handled by Playwright if CI is true
| exit 1 | ||
| fi | ||
|
|
||
| exec "$playwright_bin" test "$@" |
There was a problem hiding this comment.
should we build a set of scripts defined in package.json as we do in Cypress?
There was a problem hiding this comment.
see package.json in https://github.com/openshift/console/pull/16320/changes
vikram-raj
force-pushed
the
prow-playwright-e2e
branch
from
4114813 to
2289d0f
Compare
…2E tests
Enhance the Playwright E2E test infrastructure with package selection
similar to the existing Cypress workflow, plus convenience yarn scripts.
**Package Selection**:
- Add -p flag to run tests for specific packages (console, olm, dev-console, etc.)
- Add -l flag to list available package IDs and their paths
- Support 10 packages: console, olm, dev-console, shipwright, webterminal,
telemetry, knative, helm, topology, container-security
- Mutual exclusion validation between -p and -d flags
**Yarn Scripts**:
- Add test-playwright-e2e (run from frontend root)
- Add test-playwright-e2e-list (show available packages)
- Add test-playwright-{package} scripts for all 10 supported packages
**Breaking Change**:
- Rename PLAYWRIGHT_BASE_URL to WEB_CONSOLE_URL for consistency with
test-cypress.sh. Scripts using PLAYWRIGHT_BASE_URL will need to update.
**Other Changes**:
- Simplify usage documentation
- Move option parsing before environment setup for better UX
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
vikram-raj
force-pushed
the
prow-playwright-e2e
branch
from
2289d0f to
1fb3066
Compare
|
@vikram-raj: 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. |
3 participants
Analysis / Root cause:
OpenShift Console currently uses Cypress for E2E testing in Prow/CI with package selection support. This PR enhances the Playwright E2E test infrastructure to provide similar package selection functionality, making it easier for developers to run tests for specific packages.
Solution description:
Enhanced the Playwright E2E test infrastructure with three main improvements:
1. Package Selection System
Added package selection similar to existing Cypress workflow:
-p <package>flag: Run tests for specific packages by name (e.g.,-p console,-p helm)-lflag: List all available package IDs and their paths-pand-dflags to prevent user error2. Yarn Script Integration
Added convenience scripts to package.json for easier test execution:
3. Environment Variable Alignment
Breaking Change: Renamed
PLAYWRIGHT_BASE_URLtoWEB_CONSOLE_URLfor consistency with test-cypress.shThis aligns the Playwright test script with the existing Cypress script environment variables. Scripts or CI/CD pipelines using
PLAYWRIGHT_BASE_URLwill need to update toWEB_CONSOLE_URL.Other Improvements
Screenshots / screen recording:
N/A - Infrastructure/tooling change only
Test setup:
Prerequisites to test this PR:
ocCLI, or set BRIDGE_BASE_ADDRESScd frontend yarn add -D @playwright/test yarn playwright installTest cases:
./frontend/integration-tests/test-playwright-e2e.sh -lto list packages./frontend/integration-tests/test-playwright-e2e.sh -p consoleto test console packageyarn test-playwright-e2e-listfrom frontend directoryyarn test-playwright-helmto test helm package./integration-tests/test-playwright-e2e.sh -p console -d .(should error)./integration-tests/test-playwright-e2e.sh -p invalid(should show helpful message)-cflag to verify user creation works./test-prow-playwright-e2e.sh e2efrom repo rootBrowser conformance:
N/A - Infrastructure/tooling change (scripts support any browser Playwright supports)
Additional info:
-dflag for custom pathsMigration Guide for Breaking Change:
If you have scripts or CI/CD pipelines using
PLAYWRIGHT_BASE_URL, update them to useWEB_CONSOLE_URL:Reviewers and assignees:
🤖 Generated with Claude Code