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 1 commit into
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
| # | ||
| # 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>
vikram-raj
force-pushed
the
prow-playwright-e2e
branch
2 times, most recently
from
2289d0f to
1fb3066
Compare
| ARTIFACT_DIR=${ARTIFACT_DIR:-/tmp/artifacts} | ||
| INSTALLER_DIR=${INSTALLER_DIR:=${ARTIFACT_DIR}/installer} |
There was a problem hiding this comment.
should we export them and remove them from the other file?
vikram-raj
force-pushed
the
prow-playwright-e2e
branch
from
1fb3066 to
b25bc59
Compare
Add Playwright E2E testing infrastructure for OpenShift CI/Prow, mirroring the existing Cypress test-prow-e2e.sh workflow. **Files Added**: - test-prow-playwright-e2e.sh: Prow/CI wrapper at repository root - frontend/integration-tests/test-playwright-e2e.sh: Frontend test runner **Prow Script (test-prow-playwright-e2e.sh)**: - Mirrors test-prow-e2e.sh workflow (kubeadmin password, BRIDGE_BASE_ADDRESS, create-user) - Supports e2e, release, and smoke scenarios - Smoke scenario uses Playwright's native --project=smoke flag - Includes same CSP validation (test-puppeteer-csp) as Cypress Prow script - Delegates to frontend/integration-tests/test-playwright-e2e.sh for test execution **Frontend Script (frontend/integration-tests/test-playwright-e2e.sh)**: - Run from frontend/ directory, similar to test-cypress.sh - Supports -c flag to run contrib/create-user.sh before tests - Sets up environment variables: BRIDGE_BASE_ADDRESS, BRIDGE_BASE_PATH, WEB_CONSOLE_URL - Validates running from correct directory and checks for oc availability - Uses Playwright's native --project flag for test selection (e.g., --project=helm) - Delegates all Playwright arguments directly to the Playwright CLI **Usage Examples**: # Prow/CI (from repository root) ./test-prow-playwright-e2e.sh e2e ./test-prow-playwright-e2e.sh smoke # Frontend (from frontend/ directory) ./integration-tests/test-playwright-e2e.sh --project=helm ./integration-tests/test-playwright-e2e.sh --project=smoke ./integration-tests/test-playwright-e2e.sh -c --project=dev-console Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
vikram-raj
force-pushed
the
prow-playwright-e2e
branch
from
b25bc59 to
31d9209
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. This PR adds Playwright E2E testing infrastructure for Prow/CI, providing an alternative E2E testing framework that can run in the CI environment.
Solution description:
Added Playwright E2E testing infrastructure for OpenShift CI/Prow, mirroring the existing Cypress test-prow-e2e.sh workflow.
Files Added
test-prow-playwright-e2e.sh (Prow/CI wrapper at repository root):
--project=smokeflagfrontend/integration-tests/test-playwright-e2e.sh (Frontend test runner):
-cflag to run contrib/create-user.sh before tests--projectflag for test selection (e.g.,--project=helm,--project=smoke)Usage Examples
Prow/CI (from repository root):
Frontend (from frontend/ directory):
./integration-tests/test-playwright-e2e.sh --project=helm ./integration-tests/test-playwright-e2e.sh --project=smoke ./integration-tests/test-playwright-e2e.sh -c --project=dev-console ./integration-tests/test-playwright-e2e.sh --help # Shows Playwright's native helpDesign Decisions
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 --project=smoketo test with smoke project./frontend/integration-tests/test-playwright-e2e.sh -cto verify user creation works./test-prow-playwright-e2e.sh e2efrom repo root (Prow/CI entry point)./test-prow-playwright-e2e.sh smokefrom repo rootBrowser conformance:
N/A - Infrastructure/tooling change (scripts support any browser Playwright supports)
Additional info:
Reviewers and assignees:
🤖 Generated with Claude Code