OCPNETUI-56: Add Cypress test specs and runner scripts - #470
Conversation
|
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
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ 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 |
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.
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
79921b9 to
a1e0829
Compare
| 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
There was a problem hiding this comment.
Sorry for not being clear in my comment - I was referring to built-in Cypress task. Cypress exec has been deprecated - looking at the deprecation notice mostly due to cross-platform and security issues.
There was a problem hiding this comment.
We can fix it in a follow-up.
a1e0829 to
10a0fe3
Compare
openshift-ci
Bot
added
the
needs-rebase
|
Addressed review comments from @rszwajko: Fixed in this push:
Tracked as follow-ups:
|
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>
10a0fe3 to
8518e7b
Compare
openshift-ci
Bot
removed
the
needs-rebase
rszwajko
left a comment
rszwajko
left a comment
There was a problem hiding this comment.
Looks good! the remaining points can be fixed in follow-ups.
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkladnit, rszwajko 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 |
|
@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. |
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