OCPNETUI-56: Add Cypress test specs and runner scripts - #470
Conversation
Set up Cypress test infrastructure for networking plugin E2E tests: - cypress.config.js with esbuild preprocessor, timeouts, reporter - Support files: login commands, navigation helpers, selectors - Plugin config with dotenv and webpack preprocessor - ESLint, tsconfig, reporter config for the test suite - .env.example with required environment variables - Package deps: esbuild-loader, dotenv, cypress-multi-reporters - Minor fixes in integration-tests/ login and example spec Jira: OCPNETUI-56 Co-authored-by: Cursor <cursoragent@cursor.com>
Add view helpers and constants for networking E2E tests: - Views: NAD form helpers, UDN form helpers, common selectors, actions - Constants: base test values, NAD config constants - Types: NAD type definitions These are the page object layer used by test specs (next PR). Jira: OCPNETUI-56 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@lkladnit: This pull request references OCPNETUI-56 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. |
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: WalkthroughThe pull request migrates networking E2E coverage to Cypress. It adds shared test support, networking specs, OpenShift namespace lifecycle scripts, CI execution, report collection, flakiness research, and migration documentation. ChangesCypress E2E migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant test_cypress.sh
participant setup.sh
participant Cypress
participant OpenShift
participant cleanup.sh
GitHubActions->>test_cypress.sh: run headless E2E tests
test_cypress.sh->>setup.sh: prepare test namespaces
setup.sh->>OpenShift: create or reconcile namespaces
test_cypress.sh->>Cypress: execute networking specs
Cypress->>OpenShift: create and inspect networking resources
test_cypress.sh->>cleanup.sh: clean resources after success
cleanup.sh->>OpenShift: delete test resources
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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 APPROVED This pull-request has been approved by: lkladnit 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
ui-tests-cy/support/commands.ts-38-44 (1)
38-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-query the filter after
.clear().Cypress marks chaining commands after
.clear()as unsafe, even though.clear()yields the same subject. Split the chain before.type(projectName).🤖 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 `@ui-tests-cy/support/commands.ts` around lines 38 - 44, Split the Cypress command chain after .clear() in the filter interaction, then re-query the same filter selector before calling .type(projectName). Preserve the existing first-match selection, timeout, clearing behavior, and projectName input.Source: Linters/SAST tools
ui-tests-cy/views/nad.ts-29-30 (1)
29-30: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSplit each
.clear().type()chain.Start a new
cy.get()before.type()for each affected input, including lines 29–30, 35–37, and 53–56. This prevents failures when the form replaces the input after.clear().🤖 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 `@ui-tests-cy/views/nad.ts` around lines 29 - 30, Split each affected clear-and-type chain in the NAD form test into separate Cypress commands: keep cy.get(...).clear() and begin a new cy.get(...) before .type(). Apply this consistently to the inputs in the indicated sections, including the fields using name and description.Source: Linters/SAST tools
ui-tests-cy/utils/types/nad.ts-2-10 (1)
2-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReorder the
NadDatafields alphabetically:bridge,description,exclude,macSpoof,mtu,name,subnet,type,vlan. Theperfectionist/sort-object-typesrule reports five errors.🤖 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 `@ui-tests-cy/utils/types/nad.ts` around lines 2 - 10, Reorder the fields in the NadData type alphabetically as bridge, description, exclude, macSpoof, mtu, name, subnet, type, vlan to satisfy the perfectionist/sort-object-types rule.Source: Linters/SAST tools
ui-tests-cy/tests/setup/visit-pages.cy.ts-3-35 (1)
3-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a login setup to
visit-pages.cy.ts.support/index.tshas no setup hook, andlogin.cy.tsis a separate spec.testIsolation: falsedoes not share state across specs, so direct runs can executecy.clickNavLinkwithout a console page.🤖 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 `@ui-tests-cy/tests/setup/visit-pages.cy.ts` around lines 3 - 35, Add a login/setup hook within the “Visit networking pages” spec in visit-pages.cy.ts, using the existing login helper or flow from login.cy.ts so direct runs establish a console page before any cy.clickNavLink calls. Do not rely on support/index.ts or state shared by another spec; ensure the setup runs before the networking page tests.package.json-16-23 (1)
16-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReorder script keys to pass the repository lint check.
The static analysis report requires
lintbeforets-node,cypress-generatebeforecypress-merge, andcypress-mergebeforetest-e2e-open. Reorder the keys without changing command behavior.The static analysis hint reports these
perfectionist/sort-objectserrors.🤖 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 `@package.json` around lines 16 - 23, Reorder the package scripts to satisfy the repository’s object-sorting rules: place lint before ts-node, cypress-generate before cypress-merge, and cypress-merge before test-e2e-open. Keep every script command unchanged.Source: Linters/SAST tools
ui-tests-cy/MIGRATION.md-17-17 (1)
17-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the plan path in the migration log.
Line 17 references
ui/PLAN.md, but the added file isui-tests-cy/PLAN.md. Update the entry so readers can locate the plan.Proposed fix
-- Plan saved to `ui/PLAN.md` +- Plan saved to `ui-tests-cy/PLAN.md`The reviewed files establish
ui-tests-cy/PLAN.mdas the plan location.🤖 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 `@ui-tests-cy/MIGRATION.md` at line 17, Update the plan reference in the migration log to point to ui-tests-cy/PLAN.md instead of ui/PLAN.md, matching the location of the added plan file..env.example-5-5 (1)
5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParse
HIDE_XHRas a boolean before passing it to Cypress.
dotenvprovides"false"as a string. The truthiness check inui-tests-cy/support/index.ts:10therefore remains enabled. Convert"true"and"false"to booleans, or document that onlytrueis supported.🤖 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 @.env.example at line 5, Update the HIDE_XHR configuration handling used by Cypress support initialization so the dotenv string "false" becomes boolean false rather than a truthy value. Parse both "true" and "false" explicitly before passing the setting to Cypress, while preserving the existing behavior for enabled HIDE_XHR.
🧹 Nitpick comments (1)
ui-tests-cy/utils/types/nad.ts (1)
1-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefine
NadDataas a discriminated union forcreateNAD.
NadDatacurrently allows{}and arbitrarytypevalues.createNADalways passesnameanddescriptionto.type(). The Bridge and Localnet branches also requirebridge, and the Localnet branch requiresmtu. An unsupported or missingtypeskips every switch case and still submits. Require the common and branch-specific fields. Constraintypeto'Bridge' | 'OVN' | 'Localnet'. Keepsubnetandexcludeoptional as a pair becauseNAD_LOCALNETomits both.🤖 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 `@ui-tests-cy/utils/types/nad.ts` around lines 1 - 11, Redefine NadData as a discriminated union keyed by type, requiring name and description in every variant, restricting type to 'Bridge' | 'OVN' | 'Localnet', and requiring bridge for Bridge and Localnet plus mtu for Localnet. Keep subnet and exclude optional together so the NAD_LOCALNET variant can omit them, and remove the permissive empty/object and arbitrary-type cases.
🤖 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 @.github/workflows/e2e.yml:
- Line 27: Update the GitHub Actions references for checkout, setup-node, and
upload-artifact to use their full immutable commit SHAs instead of version tags,
while retaining an adjacent comment identifying each action’s version.
- Around line 12-27: Update the e2e job configuration to add an explicit
permissions block granting only contents: read, and configure the Checkout step
to set persist-credentials to false. Keep the existing checkout action and job
behavior unchanged.
- Around line 29-33: Update the Node.js version configured in the “Setup
Node.js” workflow step to a Cypress 15-supported version, such as 20.1.0, 22, or
24+, while preserving the existing npm cache configuration.
In `@cleanup.sh`:
- Line 20: Update the cleanup command to delete only the test-owned
NodeNetworkConfigurationPolicy resource, using its specific name or a dedicated
test label selector instead of the --all flag. Preserve the existing
ignore-not-found and non-blocking wait behavior.
In `@package.json`:
- Around line 18-20: Make test-e2e the canonical headless runner for the
migrated Cypress suite by aligning the relevant package.json scripts, then
update ui-tests-cy/PLAN.md:174 to invoke that same command; ensure all affected
sites use the migrated cypress suite rather than integration-tests.
- Around line 16-17: Use ui-tests-cy as the single Cypress project root: update
package.json lines 16-17 to lint ui-tests-cy, package.json lines 20-21 to run
Cypress commands from ui-tests-cy, and revise the architecture paths in
ui-tests-cy/PLAN.md lines 74-105 plus the final file tree in lines 194-233 to
match the checked-in root.
- Line 64: Update the dotenv dependency declaration in package.json from the
caret range to the exact version 17.4.2, preserving the lockfile’s resolved
version and metadata.
In `@test-cypress.sh`:
- Around line 3-4: Preserve failure propagation across the test runner: in
test-cypress.sh lines 3-4, keep errexit enabled except for the Cypress pipeline
that needs captured exit codes, then run post-reporting and exit with
test_exit_code at lines 30-37; update setup.sh lines 13-42 to return nonzero
when any required namespace operation fails; update cleanup.sh lines 12-21 to
return nonzero for cleanup failures other than missing resources.
- Around line 25-30: Update the Cypress invocation in both the interactive and
headless branches of test-cypress.sh to target the configured ui-tests-cy
project, including artifact creation and the working directory or project
argument. Ensure Cypress uses ui-tests-cy’s cypress.config.js, test patterns,
support file, and artifact paths in both modes.
In `@ui-tests-cy/cypress.config.js`:
- Around line 3-18: Fix asynchronous Cypress environment access across
ui-tests-cy/cypress.config.js lines 3-18, integration-tests/support/login.ts
lines 11-27, and integration-tests/tests/example-page.cy.ts line 4: configure
PLUGIN_TEMPLATE_PULL_SPEC under Cypress expose (or pass it with --expose) so
Cypress.expose() returns its value; in the login flow, resolve
cy.env('BRIDGE_KUBEADMIN_PASSWORD') inside .then(...) before using it or
deriving loginUsername. Update the cited example-page site only as needed to
consume the configured exposed value.
In `@ui-tests-cy/support/commands.ts`:
- Around line 14-22: Update both cy.exec calls in the resource deletion command
to remove failOnNonZeroExit: false, while retaining --ignore-not-found=true and
the existing timeout settings so genuine authentication, API, and deletion
failures fail the test.
In `@ui-tests-cy/support/index.ts`:
- Around line 6-8: Remove the global uncaught:exception handler registered via
Cypress.on, or narrow it to return false only for a documented exact known error
while allowing all other application exceptions to fail the UI tests.
In `@ui-tests-cy/support/login.ts`:
- Line 43: Update the cy.type call for originPwd in the login flow to pass the
option that disables command logging, ensuring the password does not appear in
Cypress’s Command Log while preserving the existing password entry behavior.
In `@ui-tests-cy/tests/all.cy.ts`:
- Around line 1-10: Restrict Cypress spec discovery so the aggregator all.cy.ts
does not execute alongside its imported leaf specs. Update the Cypress
specPattern/configuration to target tests/all.cy.ts only, or remove the
all.cy.ts aggregator while preserving direct leaf-spec execution; ensure each
test runs exactly once without --spec.
In `@ui-tests-cy/tests/networking/nad-localnet.cy.ts`:
- Around line 10-13: Make the it('delete NAD') test self-contained by creating
NAD_LOCALNET within the test before calling deleteNAD. Preserve the existing
deletion target and ensure the test no longer relies on state from preceding
tests or test execution order.
In `@ui-tests-cy/tests/networking/routes.cy.ts`:
- Around line 8-11: Update the service setup around the cy.exec call to fail
closed: validate TEST_NS, stop interpolating it into a shell command, and pass
it as an argument to a Node task that performs the oc lookup/creation. Remove
the || fallback and failOnNonZeroExit: false so authentication, API, lookup, and
creation errors propagate instead of being treated as a missing service.
---
Minor comments:
In @.env.example:
- Line 5: Update the HIDE_XHR configuration handling used by Cypress support
initialization so the dotenv string "false" becomes boolean false rather than a
truthy value. Parse both "true" and "false" explicitly before passing the
setting to Cypress, while preserving the existing behavior for enabled HIDE_XHR.
In `@package.json`:
- Around line 16-23: Reorder the package scripts to satisfy the repository’s
object-sorting rules: place lint before ts-node, cypress-generate before
cypress-merge, and cypress-merge before test-e2e-open. Keep every script command
unchanged.
In `@ui-tests-cy/MIGRATION.md`:
- Line 17: Update the plan reference in the migration log to point to
ui-tests-cy/PLAN.md instead of ui/PLAN.md, matching the location of the added
plan file.
In `@ui-tests-cy/support/commands.ts`:
- Around line 38-44: Split the Cypress command chain after .clear() in the
filter interaction, then re-query the same filter selector before calling
.type(projectName). Preserve the existing first-match selection, timeout,
clearing behavior, and projectName input.
In `@ui-tests-cy/tests/setup/visit-pages.cy.ts`:
- Around line 3-35: Add a login/setup hook within the “Visit networking pages”
spec in visit-pages.cy.ts, using the existing login helper or flow from
login.cy.ts so direct runs establish a console page before any cy.clickNavLink
calls. Do not rely on support/index.ts or state shared by another spec; ensure
the setup runs before the networking page tests.
In `@ui-tests-cy/utils/types/nad.ts`:
- Around line 2-10: Reorder the fields in the NadData type alphabetically as
bridge, description, exclude, macSpoof, mtu, name, subnet, type, vlan to satisfy
the perfectionist/sort-object-types rule.
In `@ui-tests-cy/views/nad.ts`:
- Around line 29-30: Split each affected clear-and-type chain in the NAD form
test into separate Cypress commands: keep cy.get(...).clear() and begin a new
cy.get(...) before .type(). Apply this consistently to the inputs in the
indicated sections, including the fields using name and description.
---
Nitpick comments:
In `@ui-tests-cy/utils/types/nad.ts`:
- Around line 1-11: Redefine NadData as a discriminated union keyed by type,
requiring name and description in every variant, restricting type to 'Bridge' |
'OVN' | 'Localnet', and requiring bridge for Bridge and Localnet plus mtu for
Localnet. Keep subnet and exclude optional together so the NAD_LOCALNET variant
can omit them, and remove the permissive empty/object and arbitrary-type cases.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82e1a448-2a7e-40e8-b370-29474c6989fd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.env.example.github/workflows/e2e.yml.gitignorecleanup.shintegration-tests/support/login.tsintegration-tests/tests/example-page.cy.tspackage.jsonresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/MIGRATION.mdui-tests-cy/PLAN.mdui-tests-cy/cypress.config.jsui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/networking/ingresses.cy.tsui-tests-cy/tests/networking/nad-bridge.cy.tsui-tests-cy/tests/networking/nad-localnet.cy.tsui-tests-cy/tests/networking/nad-ovn.cy.tsui-tests-cy/tests/networking/net-policies.cy.tsui-tests-cy/tests/networking/routes.cy.tsui-tests-cy/tests/networking/services.cy.tsui-tests-cy/tests/networking/udn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tests/setup/visit-pages.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/utils/const/nad.tsui-tests-cy/utils/types/nad.tsui-tests-cy/views/actions.tsui-tests-cy/views/nad.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/udn.ts
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin every GitHub Action to a full commit SHA.
actions/checkout@v4, actions/setup-node@v4, and actions/upload-artifact@v4 use mutable tags. Replace each tag with a full commit SHA and retain an adjacent version comment.
As per path instructions, “Pin actions by full SHA, not tag.”
Also applies to: 30-30, 43-43
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 26-27: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/e2e.yml at line 27, Update the GitHub Actions references
for checkout, setup-node, and upload-artifact to use their full immutable commit
SHAs instead of version tags, while retaining an adjacent comment identifying
each action’s version.
Source: Path instructions
| "lint": "eslint ./src ./integration-tests ./cypress", | ||
| "lint-fix": "eslint ./src ./integration-tests ./cypress --fix", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one Cypress project-root path.
The supplied runtime configuration is under ui-tests-cy/, but the package scripts and plan document cypress/. Align the commands and documentation before merge.
package.json#L16-L17: lintui-tests-cyinstead of./cypress.package.json#L20-L21: run the Cypress commands fromui-tests-cy.ui-tests-cy/PLAN.md#L74-L105: update the architecture paths to the checked-in project root.ui-tests-cy/PLAN.md#L194-L233: update the final file tree to the checked-in project root.
The supplied review context identifies ui-tests-cy as the new Cypress project root.
🧰 Tools
🪛 ESLint
[error] 16-16: Expected "lint" to come before "ts-node"
(perfectionist/sort-objects)
📍 Affects 2 files
package.json#L16-L17(this comment)package.json#L20-L21ui-tests-cy/PLAN.md#L74-L105ui-tests-cy/PLAN.md#L194-L233
🤖 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 `@package.json` around lines 16 - 17, Use ui-tests-cy as the single Cypress
project root: update package.json lines 16-17 to lint ui-tests-cy, package.json
lines 20-21 to run Cypress commands from ui-tests-cy, and revise the
architecture paths in ui-tests-cy/PLAN.md lines 74-105 plus the final file tree
in lines 194-233 to match the checked-in root.
| it('delete NAD', () => { | ||
| cy.visitNAD(); | ||
| deleteNAD(NAD_LOCALNET.name); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file=$(git ls-files | rg '(^|/)ui-tests-cy/tests/networking/nad-localnet\.cy\.ts$' | head -n1)
printf '%s\n' "FILE=$file"
test -n "$file"
wc -l "$file"
cat -n "$file"
printf '%s\n' '--- related NAD symbols and hooks ---'
rg -n --glob '*.{ts,js}' 'NAD_LOCALNET|createNAD|deleteNAD|visitNAD|beforeEach|afterEach' ui-tests-cyRepository: openshift/networking-console-plugin
Length of output: 1988
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- NAD view helpers ---'
cat -n ui-tests-cy/views/nad.ts
printf '%s\n' '--- NAD fixture ---'
cat -n ui-tests-cy/utils/const/nad.ts
printf '%s\n' '--- suite wrapper and navigation ---'
cat -n ui-tests-cy/utils/const/base.ts
cat -n ui-tests-cy/support/nav.ts
printf '%s\n' '--- Cypress configuration and test scripts ---'
rg -n --glob 'package.json' --glob '*config*' --glob '*.ts' 'testIsolation|experimentalSessionAndOrigin|cypress run|ui-tests-cy' .Repository: openshift/networking-console-plugin
Length of output: 6883
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
test = Path("ui-tests-cy/tests/networking/nad-localnet.cy.ts").read_text()
config = Path("ui-tests-cy/cypress.config.js").read_text()
create_tests = re.findall(r"\bit\(\s*(['\"])(.*?)\1\s*,\s*\(\)\s*=>\s*\{(.*?)\n\s*\}\s*\)", test, re.S)
delete_body = next((body for _, title, body in create_tests if title == "delete NAD"), None)
print("delete_test_found:", delete_body is not None)
print("delete_test_calls_createNAD:", bool(delete_body and re.search(r"\bcreateNAD\s*\(", delete_body)))
print("delete_test_calls_deleteNAD:", bool(delete_body and re.search(r"\bdeleteNAD\s*\(", delete_body)))
print("testIsolation_false:", bool(re.search(r"\btestIsolation\s*:\s*false\b", config)))
print("create_and_delete_are_separate_tests:", len([title for _, title, _ in create_tests if "NAD" in title]) >= 2)
PYRepository: openshift/networking-console-plugin
Length of output: 335
Make the delete test independent.
it('delete NAD') does not create NAD_LOCALNET. With testIsolation: false, it depends on the preceding test and fails when run alone or after creation fails. Combine creation and deletion, or create the NAD within the delete test.
🤖 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 `@ui-tests-cy/tests/networking/nad-localnet.cy.ts` around lines 10 - 13, Make
the it('delete NAD') test self-contained by creating NAD_LOCALNET within the
test before calling deleteNAD. Preserve the existing deletion target and ensure
the test no longer relies on state from preceding tests or test execution order.
| cy.exec( | ||
| `oc get service ${EXAMPLE} -n ${TEST_NS} 2>/dev/null || oc create service clusterip ${EXAMPLE} --tcp=80:80 -n ${TEST_NS}`, | ||
| { failOnNonZeroExit: false }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n ui-tests-cy/tests/networking/routes.cy.ts
printf '%s\n' '--- related Cypress and oc.exec usages ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
'Cypress\.env|cy\.exec|oc get service|clusterip|TEST_NS|EXAMPLE' \
ui-tests-cy
printf '%s\n' '--- repository files near Cypress configuration ---'
git ls-files ui-tests-cy | sed -n '1,160p'Repository: openshift/networking-console-plugin
Length of output: 6158
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Cypress configuration and plugin setup ---'
cat -n ui-tests-cy/cypress.config.js
cat -n ui-tests-cy/plugins/index.ts
printf '%s\n' '--- command helpers ---'
cat -n ui-tests-cy/support/commands.ts
printf '%s\n' '--- namespace and test constants ---'
cat -n ui-tests-cy/utils/const/base.ts
printf '%s\n' '--- package versions and test scripts ---'
rg -n --glob 'package.json' --glob 'package-lock.json' --glob 'yarn.lock' \
'"cypress"|"test:e2e"|"ui-tests-cy"' .
printf '%s\n' '--- all Cypress exec call sites ---'
rg -n -A5 -B3 --glob '*.ts' --glob '*.js' 'cy\.exec\(' ui-tests-cyRepository: openshift/networking-console-plugin
Length of output: 7125
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import subprocess
template = (
"oc get service {example} -n {namespace} 2>/dev/null || "
"oc create service clusterip {example} --tcp=80:80 -n {namespace}"
)
for namespace in ("cy-test-ns", "safe; printf INJECTED"):
command = template.format(example="example", namespace=namespace)
# Use a harmless command prefix to demonstrate shell parsing without invoking
# `oc` or any repository code.
probe = command.replace("oc ", "printf OC_ >/dev/null; ", 2)
result = subprocess.run(
["/bin/sh", "-c", probe],
text=True,
capture_output=True,
check=False,
)
print(f"namespace={namespace!r}")
print(f"command={command!r}")
print(f"returncode={result.returncode}")
print(f"stdout={result.stdout!r}")
print(f"stderr={result.stderr!r}")
PYRepository: openshift/networking-console-plugin
Length of output: 713
🌐 Web query:
Cypress 15 cy.exec failOnNonZeroExit false documentation command shell behavior
💡 Result:
In Cypress, the cy.exec command is used to execute system commands [1]. The option failOnNonZeroExit, which defaults to true, determines whether the Cypress test fails if the executed command returns a non-zero exit code [1][2]. When you set failOnNonZeroExit: false, Cypress will not automatically fail the test if the command exits with a non-zero code [1][2]. Instead, the command promise will resolve, and you can inspect the result object—which includes the exitCode, stdout, and stderr—to perform your own assertions [1][2]. Example usage: cy.exec('your-command', { failOnNonZeroExit: false }).then((result) => { expect(result.exitCode).to.eq(1); expect(result.stderr).to.contain('expected error message'); }); Regarding command shell behavior: Cypress executes commands via a child process. Historically and in current implementations, cy.exec relies on the underlying shell to interpret commands [3]. On Windows, this behavior can be environment-dependent; if the system shell (such as cmd.exe or a shell defined by environment variables) is not explicitly handled or available, commands requiring shell features—like pipes (|), redirections (>), or environment variable expansion—may fail if they are executed without a shell context [4][3]. Cypress generally attempts to resolve a suitable shell for execution, but complexities can arise depending on the host operating system and available shell environments [4][3]. Note that cy.exec requires the command to eventually exit; commands that do not exit (e.g., long-running servers) will timeout and cause the test to fail [1][5][2].
Citations:
- 1: https://docs.cypress.io/api/commands/exec
- 2: https://github.com/cypress-io/cypress-documentation/blob/main/docs/api/commands/exec.mdx
- 3: dependency: bump execa versions to 4.1.0 cypress-io/cypress#32027
- 4: cy.exec fails on windows cypress-io/cypress#789
- 5: https://docs.cypress.io/llm/markdown/api/commands/exec.md
Make service setup fail closed.
Do not suppress oc errors or set failOnNonZeroExit: false. The || treats authentication and API errors as “service missing,” then Cypress continues if creation also fails. Validate TEST_NS and pass it as an argument to a Node task instead of interpolating it into the shell command. The current interpolation permits shell command injection.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 8-11: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@ui-tests-cy/tests/networking/routes.cy.ts` around lines 8 - 11, Update the
service setup around the cy.exec call to fail closed: validate TEST_NS, stop
interpolating it into a shell command, and pass it as an argument to a Node task
that performs the oc lookup/creation. Remove the || fallback and
failOnNonZeroExit: false so authentication, API, lookup, and creation errors
propagate instead of being treated as a missing service.
ac9fd8b to
406078e
Compare
lkladnit
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
9bc19ab to
696798c
Compare
696798c to
79921b9
Compare
lkladnit
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
pcbailey
added
the
do-not-merge/work-in-progress
lkladnit
removed
the
do-not-merge/work-in-progress
Add E2E test specs for all networking resources: - NADs: bridge, localnet, OVN topologies - Network policies, routes, services, ingresses - User-defined networks (UDN + ClusterUDN) - Login and page navigation verification Add test runner infrastructure: - setup.sh / cleanup.sh for test namespace lifecycle - test-cypress.sh with GUI (-g) and headless modes - research-flakiness.sh for repeated execution analysis - e2e.yml GitHub Actions workflow for CI - Migration and planning docs Depends on: cypress-infra (1a) + cypress-views (1b) Jira: OCPNETUI-56 Co-authored-by: Cursor <cursoragent@cursor.com>
79921b9 to
a1e0829
Compare
|
@lkladnit: 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. |
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
it may be a follow-up but we should update all the actions - the current is v7
| cache: npm | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci |
There was a problem hiding this comment.
in this install path we rely on post-install scripts which is insecure.
Alternative way is to disable post-install and install cypress explicitly:
npm clean-install --ignore-scripts --no-audit
npx cypress install
| } | ||
| }); | ||
| cy.get( | ||
| 'input[data-test="dropdown-text-filter"], input[placeholder*="name"], input[aria-label*="filter"]', |
There was a problem hiding this comment.
note that most likely we are using data test IDs not compatible with PatternFly.
check my comment for a similar PR for kubevirt plugin.
| timeout-minutes: 30 | ||
|
|
||
| env: | ||
| BRIDGE_BASE_ADDRESS: ${{ secrets.CONSOLE_URL }} |
There was a problem hiding this comment.
the tests seem to be run via dev mode - in case of networking plugin it may be easier to build an image and update the env var in cluster network operator.
There was a problem hiding this comment.
for hot cluster it makes sense. however still we should be using a cluster network operator compatible with our plugin. Do we have some way of checking that?
| timeout-minutes: 30 | ||
|
|
||
| env: | ||
| BRIDGE_BASE_ADDRESS: ${{ secrets.CONSOLE_URL }} |
There was a problem hiding this comment.
for hot cluster it makes sense. however still we should be using a cluster network operator compatible with our plugin. Do we have some way of checking that?
|
|
||
| env: | ||
| BRIDGE_BASE_ADDRESS: ${{ secrets.CONSOLE_URL }} | ||
| BRIDGE_KUBEADMIN_PASSWORD: ${{ secrets.KUBEADMIN_PASSWORD }} |
| BRIDGE_BASE_ADDRESS: ${{ secrets.CONSOLE_URL }} | ||
| BRIDGE_KUBEADMIN_PASSWORD: ${{ secrets.KUBEADMIN_PASSWORD }} | ||
| BRIDGE_E2E_BROWSER_NAME: electron | ||
| TEST_NS: ${{ secrets.TEST_NS || 'cy-test-ns' }} |
There was a problem hiding this comment.
why we use secrets to store a namspace name?
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
|
|
||
| env: |
There was a problem hiding this comment.
HIDE_XHR needs to be set to true for public env
| args.push('-n', namespace); | ||
| } | ||
| args.push(kind, name, '--wait=true', '--timeout=300s'); | ||
| cy.exec(args.join(' '), { failOnNonZeroExit: false, timeout: 5 * MINUTE }); |
There was a problem hiding this comment.
exec is deprecated and unsafe - we should switch to task
| } | ||
|
|
||
| Cypress.Commands.add('deleteResource', (kind: string, name: string, namespace?: string) => { | ||
| const args = ['oc', 'delete', '--ignore-not-found=true']; |
There was a problem hiding this comment.
not sure if oc is available on that env - maybe we should use JS client as the kubevirt plugin?
| integration-tests/.DS_Store | ||
| yarn-error.log | ||
| .DS_Store | ||
| cypress/gui-test-screenshots/ |
There was a problem hiding this comment.
the path cypress does not exist
| screenshotOnRunFailure: true, | ||
| screenshotsFolder: './gui-test-screenshots/screenshots/', | ||
| trashAssetsBeforeRuns: true, | ||
| video: true, |
| run: npm run test-e2e | ||
|
|
||
| - name: Upload test artifacts | ||
| if: always() |
There was a problem hiding this comment.
maybe it's enough to upload screenshots on failure?
4 participants
Summary
Add E2E test specs for all networking resources:
Add test runner infrastructure:
PR 3 of 3 in the Cypress E2E series (1a: infra, 1b: views, 1c: tests).
Depends on #468 and #469.
Jira: OCPNETUI-56
Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Testing
Documentation