Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: XiyunZhao 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 |
|
@XiyunZhao: The label(s) 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. |
|
/label do-not-merge/hold |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@XiyunZhao: The label(s) 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. |
📝 WalkthroughWalkthroughThis change introduces parallel execution support for Cypress integration tests across two shell scripts. The test-cypress.sh script adds a 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/integration-tests/test-cypress.sh (1)
93-199:⚠️ Potential issue | 🔴 CriticalResolve merge conflict markers blocking script execution
Lines 93–199 contain unresolved VCS conflict markers (
<<<<<<<,=======,>>>>>>>) that prevent Bash from parsing the file. Merge the conflicting changes (sequential vs. parallel test execution) and remove all markers before this can run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/integration-tests/test-cypress.sh` around lines 93 - 199, Remove the VCS conflict markers (<<<<<<<, =======, >>>>>>>) and keep a single coherent implementation: keep the new parallel-execution branch (the if [ -n "${parallel-}" ]; ... fi block that declares pids, packages, run_package_test, writes logs to "${SCREENSHOTS_DIR}/${package}.log", waits on pids and prints summary) and the else fallback that runs the original sequential yarn run test-cypress-*-headless commands; ensure only one copy of the tests exists (no duplicated lists) and that all references to run_package_test, pids, packages, failed_packages and SCREENSHOTS_DIR remain consistent and unblocked by conflict markers.test-prow-e2e.sh (1)
31-60:⚠️ Potential issue | 🔴 CriticalResolve merge-conflict markers before merging
Lines 31–60 contain unresolved merge conflict markers (
<<<<<<<,=======,>>>>>>>), which cause bash syntax errors and break CI execution. Resolve the conflict by choosing between upstream (SCENARIO dispatch logic) or stashed changes (completion banner), or merge both correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-prow-e2e.sh` around lines 31 - 60, There are unresolved git conflict markers (<<<<<<<, =======, >>>>>>>) around the SCENARIO dispatch logic which breaks the script; remove the markers and produce a single coherent block that preserves the SCENARIO-based calls to ./integration-tests/test-cypress.sh (for cases using SCENARIO, e.g., "nightly-cypress", "e2e"/"release", "login", "olmFull", "dev-console", "pipelines", "knative") ensuring the closing fi is present, keep the subsequent env NO_SANDBOX=true yarn test-puppeteer-csp and popd lines, and then append the completion banner (the echo "" / "E2E Tests Completed" / separators) so both the scenario logic and the completion message remain without any conflict markers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/integration-tests/test-cypress.sh`:
- Around line 40-47: The script treats any non-empty -P value as enabling
parallel mode, so passing "-P false" still turns it on; update the option
parsing/conditional to interpret the parallel flag as a boolean string (e.g.,
only enable parallel when parallel == "true" or "1"), not just non-empty.
Specifically, change the conditional that checks the variable parallel
(currently using [ -n "${parallel-}" ]) to explicitly compare the value of
parallel (e.g., [[ "${parallel}" == "true" ]] or similar) and adjust any help
text or defaults to match this behavior, ensuring the getopts handling of the P
flag and subsequent branch that uses parallel consistently expect
"true"/"false".
In `@test-prow-e2e.sh`:
- Around line 27-30: Resolve the unresolved git merge conflict in
test-prow-e2e.sh by removing the conflict markers and choosing either the
unconditional full-headless invocation (./integration-tests/test-cypress.sh -h
true ${PARALLEL_FLAG}) or the stashed scenario-based routing that checks the
SCENARIO variable (checks for values like nightly-cypress, login, olmFull,
dev-console, pipelines, knative, etc.); verify which to keep by inspecting your
external Prow job definitions for any SCENARIO usage and if scenario jobs still
exist keep and restore the conditional block (preserving the SCENARIO checks and
corresponding invocations), otherwise keep the upstream unconditional call, then
save the cleaned script and ensure it executes without conflict markers.
---
Outside diff comments:
In `@frontend/integration-tests/test-cypress.sh`:
- Around line 93-199: Remove the VCS conflict markers (<<<<<<<, =======,
>>>>>>>) and keep a single coherent implementation: keep the new
parallel-execution branch (the if [ -n "${parallel-}" ]; ... fi block that
declares pids, packages, run_package_test, writes logs to
"${SCREENSHOTS_DIR}/${package}.log", waits on pids and prints summary) and the
else fallback that runs the original sequential yarn run test-cypress-*-headless
commands; ensure only one copy of the tests exists (no duplicated lists) and
that all references to run_package_test, pids, packages, failed_packages and
SCREENSHOTS_DIR remain consistent and unblocked by conflict markers.
In `@test-prow-e2e.sh`:
- Around line 31-60: There are unresolved git conflict markers (<<<<<<<,
=======, >>>>>>>) around the SCENARIO dispatch logic which breaks the script;
remove the markers and produce a single coherent block that preserves the
SCENARIO-based calls to ./integration-tests/test-cypress.sh (for cases using
SCENARIO, e.g., "nightly-cypress", "e2e"/"release", "login", "olmFull",
"dev-console", "pipelines", "knative") ensuring the closing fi is present, keep
the subsequent env NO_SANDBOX=true yarn test-puppeteer-csp and popd lines, and
then append the completion banner (the echo "" / "E2E Tests Completed" /
separators) so both the scenario logic and the completion message remain without
any conflict markers.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
frontend/integration-tests/test-cypress.shtest-prow-e2e.sh
| while getopts p:s:h:l:n:P: flag | ||
| do | ||
| case "${flag}" in | ||
| p) pkg=${OPTARG};; | ||
| s) spec=${OPTARG};; | ||
| h) headless=${OPTARG};; | ||
| n) nightly=${OPTARG};; | ||
| P) parallel=${OPTARG};; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files frontend/integration-tests/test-cypress.shRepository: openshift/console
Length of output: 103
🏁 Script executed:
sed -n '40,47p; 58p; 104,105p' frontend/integration-tests/test-cypress.shRepository: openshift/console
Length of output: 412
🏁 Script executed:
sed -n '40,110p' frontend/integration-tests/test-cypress.sh | cat -nRepository: openshift/console
Length of output: 3938
🏁 Script executed:
grep -n "parallel" frontend/integration-tests/test-cypress.shRepository: openshift/console
Length of output: 501
-P false still enables parallel mode
The conditional at line 104 checks only for non-empty input using [ -n "${parallel-}" ], so -P false still takes the parallel branch, contradicting the documented -P true usage in the help text (line 58).
Proposed fix
while getopts p:s:h:l:n:P: flag
do
case "${flag}" in
p) pkg=${OPTARG};;
s) spec=${OPTARG};;
h) headless=${OPTARG};;
n) nightly=${OPTARG};;
- P) parallel=${OPTARG};;
+ P) parallel="$(printf '%s' "${OPTARG}" | tr '[:upper:]' '[:lower:]')";;
esac
done
+
+if [ -n "${parallel-}" ] && [ "${parallel}" != "true" ] && [ "${parallel}" != "false" ]; then
+ echo "Invalid -P value '${parallel}'. Expected true|false." >&2
+ exit 2
+fi
@@
- if [ -n "${parallel-}" ]; then
+ if [ "${parallel-false}" = "true" ]; thenAlso applies to: 58, 104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/integration-tests/test-cypress.sh` around lines 40 - 47, The script
treats any non-empty -P value as enabling parallel mode, so passing "-P false"
still turns it on; update the option parsing/conditional to interpret the
parallel flag as a boolean string (e.g., only enable parallel when parallel ==
"true" or "1"), not just non-empty. Specifically, change the conditional that
checks the variable parallel (currently using [ -n "${parallel-}" ]) to
explicitly compare the value of parallel (e.g., [[ "${parallel}" == "true" ]] or
similar) and adjust any help text or defaults to match this behavior, ensuring
the getopts handling of the P flag and subsequent branch that uses parallel
consistently expect "true"/"false".
| # Run Cypress tests | ||
| # shellcheck disable=SC2086 | ||
| ./integration-tests/test-cypress.sh -h true ${PARALLEL_FLAG} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "test-prow-e2e\.sh"Repository: openshift/console
Length of output: 77
🏁 Script executed:
cat -n test-prow-e2e.sh | head -70Repository: openshift/console
Length of output: 2372
🏁 Script executed:
# Find prow config files
find . -type f \( -name "*prow*" -o -name "*job*" -o -name "*ci*" \) -path "*/.git" -prune -o -type f \( -name "*.yaml" -o -name "*.yml" \) -print | grep -iE "(prow|job|ci)" | head -20Repository: openshift/console
Length of output: 1187
🏁 Script executed:
cat -n .ci-operator.yaml | head -100Repository: openshift/console
Length of output: 166
🏁 Script executed:
# Search for SCENARIO variable usage in the codebase
rg -n '\bSCENARIO\b' --type sh --type yaml --type ymlRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for SCENARIO variable usage
rg -n '\bSCENARIO\b'Repository: openshift/console
Length of output: 518
🏁 Script executed:
# Search for references to the specific scenario job names
rg -n "nightly-cypress|olmFull|dev-console.*pipelines|knative.*release" --type shRepository: openshift/console
Length of output: 186
🏁 Script executed:
# Find prow job definitions
find . -type d -name "*prow*" -o -name "*jobs*" -o -name "*ci*" 2>/dev/null | grep -v node_modules | grep -v vendor | head -20Repository: openshift/console
Length of output: 1220
🏁 Script executed:
# Search for job definitions or CI configuration in root and common locations
find . -maxdepth 2 -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*prow*" \) ! -path "./vendor/*" ! -path "./node_modules/*" ! -path "./.git/*"Repository: openshift/console
Length of output: 354
🏁 Script executed:
# Check git history to understand this change
git log --oneline test-prow-e2e.sh | head -20Repository: openshift/console
Length of output: 85
🏁 Script executed:
# Check the actual changes in the recent commit
git show 4d1972ac23 --statRepository: openshift/console
Length of output: 50374
🏁 Script executed:
# Look for how test-prow-e2e.sh is called/configured - check for prow config references
find . -maxdepth 2 -type f -name "*.yaml" ! -path "./vendor/*" -exec grep -l "test-prow-e2e\|SCENARIO" {} \;Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for any references to test-prow-e2e or test-cypress in configuration
rg -n "test-prow-e2e|test-cypress" --type yaml --type shRepository: openshift/console
Length of output: 6050
🏁 Script executed:
# Check README or documentation for CI job information
find . -maxdepth 2 -type f \( -name "README*" -o -name "CONTRIBUTING*" -o -name "TESTING*" \) ! -path "./vendor/*" -exec grep -l "SCENARIO\|prow\|job\|cypress" {} \;Repository: openshift/console
Length of output: 85
🏁 Script executed:
grep -n "SCENARIO\|cypress\|prow\|job" TESTING.md | head -50Repository: openshift/console
Length of output: 528
🏁 Script executed:
# Check if there's any git status showing this unresolved conflict
git status test-prow-e2e.shRepository: openshift/console
Length of output: 126
Resolve unresolved git merge conflict in test-prow-e2e.sh before merging
The file contains unresolved merge conflict markers (lines 31–60) with both upstream and stashed versions. The upstream version (line 29) runs an unconditional full headless suite, while the stashed version (lines 32–50) contains scenario-based conditional routing (SCENARIO variable checks for nightly-cypress, login, olmFull, dev-console, pipelines, knative, etc.).
This conflict must be explicitly resolved. Before choosing the upstream version, verify whether these scenario-based test jobs are still active in your Prow configuration. If they are, removing the conditional routing changes test coverage and duration. If they've been deprecated, the stashed code is dead and the upstream version is correct. The repository itself contains no SCENARIO references outside this conflict, so verification requires checking your external Prow job definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-prow-e2e.sh` around lines 27 - 30, Resolve the unresolved git merge
conflict in test-prow-e2e.sh by removing the conflict markers and choosing
either the unconditional full-headless invocation
(./integration-tests/test-cypress.sh -h true ${PARALLEL_FLAG}) or the stashed
scenario-based routing that checks the SCENARIO variable (checks for values like
nightly-cypress, login, olmFull, dev-console, pipelines, knative, etc.); verify
which to keep by inspecting your external Prow job definitions for any SCENARIO
usage and if scenario jobs still exist keep and restore the conditional block
(preserving the SCENARIO checks and corresponding invocations), otherwise keep
the upstream unconditional call, then save the cleaned script and ensure it
executes without conflict markers.
|
@XiyunZhao: The following tests failed, say
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. |
1 participant
testparrallel
Summary by CodeRabbit
Release Notes