NETOBSERV-2689: Improve Cypress tests#1460
Open
Amoghrd wants to merge 2 commits intonetobserv:mainfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates topology helpers into a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 1. Analyzed 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 |
Amoghrd
changed the title
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2689 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. |
Member
Author
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/cypress/integration-tests/quickFilters.cy.ts (1)
64-64:⚠️ Potential issue | 🟡 MinorKeep the uncheck selector scoped to the dropdown.
cy.contains("Test NS").parent()is still broader than necessary and can drift if the page ever renders anotherTest NSlabel. Reuse the scoped dropdown lookup here.♻️ Suggested fix
- cy.contains("Test NS").parent().find('input[type="checkbox"]').should('exist').click() + cy.get('#quick-filters-dropdown') + .contains('label', "Test NS") + .find('input[type="checkbox"]') + .should('exist') + .click()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/quickFilters.cy.ts` at line 64, The selector cy.contains("Test NS").parent().find('input[type="checkbox"]') is too broad and can match other "Test NS" labels; instead reuse the already-scoped dropdown element used earlier in the test (the scoped dropdown lookup variable) and call .find('input[type="checkbox"]').should('exist').click() on that scoped element so the checkbox lookup stays confined to the dropdown.
🧹 Nitpick comments (1)
web/cypress/views/netflow-page.ts (1)
82-105: Deduplicate topology setup logic to avoid drift.
setupTopologyViewWithNamespaceFilter()andtopologyPage.setupWithNamespaceFilter()now carry the same flow. Keep one source of truth and delegate.Proposed refactor
export function setupTopologyViewWithNamespaceFilter(namespace?: string) { - cy.clearLocalStorage() - netflowPage.visit() - cy.get('#tabs-container').contains('Topology').click() - - if (Cypress.$('[data-surface=true][transform="translate(0, 0) scale(1)"]').length > 0) { - cy.get('[data-test="filters"] > [data-test="clear-all-filters-button"]').should('exist').click() - } - cy.get('#drawer').should('not.be.empty') - - // Add filter for namespace if provided - if (namespace) { - cy.get(filterSelectors.filterInput).type("src_namespace=" + namespace + '{enter}') - cy.get('#src_namespace-0-toggle').should('contain.text', `${namespace}`) - } - - cy.byTestID("show-view-options-button").should('exist').click().then(() => { - cy.contains('Display options').should('exist').click() - cy.byTestID('layout-dropdown').click() - cy.byTestID('Grid').click() - }) - cy.byTestID(topologySelectors.metricsFunctionDrop).should('exist').click().get('#sum').click() - cy.contains('Display options').should('exist').click() + topologyPage.setupWithNamespaceFilter(namespace) }Also applies to: 157-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/netflow-page.ts` around lines 82 - 105, There are duplicate flows between setupTopologyViewWithNamespaceFilter() and topologyPage.setupWithNamespaceFilter(); keep a single source of truth by removing the duplicated implementation and delegating: choose one implementation to be canonical (e.g., topologyPage.setupWithNamespaceFilter) and update the other (e.g., setupTopologyViewWithNamespaceFilter) to simply call that canonical function with the same parameters; ensure the canonical method preserves the existing steps (clearing local storage, visiting page, switching to Topology tab, clearing filters if present, asserting drawer, applying optional namespace filter, opening Display options, selecting Grid layout, setting metrics via topologySelectors.metricsFunctionDrop and selecting '#sum', and final Display options assertion) and reuse existing selectors like filterSelectors.filterInput, '#src_namespace-0-toggle', topologySelectors.metricsFunctionDrop to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/integration-tests/netflow_export.cy.ts`:
- Around line 47-53: The test currently runs cy.exec("ls cypress/downloads")
once and moves an unquoted filename, causing flakes when the CSV isn't yet
present or the name contains spaces; update the block that builds files/csvFile
to poll/retry until a single CSV appears (e.g., loop using Cypress.Promise or a
small retry helper that calls cy.exec("ls cypress/downloads", { timeout: ... })
with a short delay and aborts on timeout) and then quote paths in the shell move
command (use mv "cypress/downloads/${csvFile}"
"cypress/downloads/export_table.csv"); keep the subsequent
cy.readFile('cypress/downloads/export_table.csv', { timeout: ... }) to assert
the file contents.
In `@web/cypress/integration-tests/quickFilters.cy.ts`:
- Around line 47-49: Replace the fixed cy.wait(10000) + cy.reload() with a
readiness check that waits for a concrete signal from the app/plugin before
reloading; e.g. use cy.intercept to watch the plugin startup API/network call
(or cy.get/cy.contains to wait for a DOM element or text that appears when the
plugin is ready) and then call cy.reload() in the .then() of that
intercept/wait. Update both occurrences that currently call cy.wait(10000) (the
block using cy.wait(...).then(() => cy.reload())) so each waits on the concrete
readiness condition (intercept alias or element assertion) instead of a fixed
timeout.
In `@web/cypress/support/commands.ts`:
- Around line 341-355: The current cy.exec calls use JSON.stringify(...) to
interpolate kubeconfig, loginUsername, loginPassword, and hostapi which does not
safely escape shell metacharacters; replace those usages with a proper
shell-escaping helper (e.g., escapeShellArg) and apply it to kubeconfig,
loginUsername, loginPassword and hostapi before building the oc commands used in
the two cy.exec calls so arguments are wrapped/escaped safely (handle single
quotes inside values), or alternatively switch to invoking a Node child_process
spawn variant that accepts an argv array; update the two cy.exec invocations
(the oc whoami and the oc login) to use the escaped values (or spawn with args)
to eliminate shell-injection risk.
In `@web/cypress/views/netflow-page.ts`:
- Around line 87-89: Replace the synchronous DOM probe using
Cypress.$('[data-surface=true][transform="translate(0, 0) scale(1)"]').length
with a retryable Cypress command: use
cy.get('[data-surface=true][transform="translate(0, 0) scale(1)"]', { timeout:
<reasonable-ms> }).should('exist').then(() => cy.get('[data-test="filters"] >
[data-test="clear-all-filters-button"]').should('exist').click()) so the clear
action waits for the topology surface to render; apply the same change to the
second occurrence (previously at lines 162–164). Also remove duplication between
setupWithNamespaceFilter and setupTopologyViewWithNamespaceFilter by extracting
their shared steps into a single helper (e.g., setupNamespaceFilter helper) and
call it from both places to keep behavior consistent.
In `@web/cypress/views/operator-hub-page.ts`:
- Around line 112-117: Replace the one-time snapshot body.find(...) with a
retryable Cypress query: use cy.get('input[data-test="Select a
Namespace-radio-input"]', { timeout: 10000 }).then(($els) => { if ($els.length)
cy.wrap($els.first()).click(); }); This keeps the optional-click behavior but
retries until the radio appears (or the timeout elapses) to avoid the flaky
path; update the block that currently uses cy.get('body').then(...) accordingly.
---
Outside diff comments:
In `@web/cypress/integration-tests/quickFilters.cy.ts`:
- Line 64: The selector cy.contains("Test
NS").parent().find('input[type="checkbox"]') is too broad and can match other
"Test NS" labels; instead reuse the already-scoped dropdown element used earlier
in the test (the scoped dropdown lookup variable) and call
.find('input[type="checkbox"]').should('exist').click() on that scoped element
so the checkbox lookup stays confined to the dropdown.
---
Nitpick comments:
In `@web/cypress/views/netflow-page.ts`:
- Around line 82-105: There are duplicate flows between
setupTopologyViewWithNamespaceFilter() and
topologyPage.setupWithNamespaceFilter(); keep a single source of truth by
removing the duplicated implementation and delegating: choose one implementation
to be canonical (e.g., topologyPage.setupWithNamespaceFilter) and update the
other (e.g., setupTopologyViewWithNamespaceFilter) to simply call that canonical
function with the same parameters; ensure the canonical method preserves the
existing steps (clearing local storage, visiting page, switching to Topology
tab, clearing filters if present, asserting drawer, applying optional namespace
filter, opening Display options, selecting Grid layout, setting metrics via
topologySelectors.metricsFunctionDrop and selecting '#sum', and final Display
options assertion) and reuse existing selectors like
filterSelectors.filterInput, '#src_namespace-0-toggle',
topologySelectors.metricsFunctionDrop to avoid drift.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deb5e6bf-3646-45e2-b506-36fd2a8c7e3d
📒 Files selected for processing (25)
web/cypress/fixtures/gateway-api.yamlweb/cypress/integration-tests/client_performance.cy.tsweb/cypress/integration-tests/gateway_topology_logo.cy.tsweb/cypress/integration-tests/netflow_export.cy.tsweb/cypress/integration-tests/netflow_zone_multiCluster.cy.tsweb/cypress/integration-tests/netobserv_udn.cy.tsweb/cypress/integration-tests/quickFilters.cy.tsweb/cypress/integration-tests/topology_edges_labels.cy.tsweb/cypress/integration-tests/topology_groups.cy.tsweb/cypress/integration-tests/topology_view.cy.tsweb/cypress/support/commands.tsweb/cypress/views/catalogs.tsweb/cypress/views/dashboards-page.tsweb/cypress/views/list-page.tsweb/cypress/views/nav.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv-logo.tsweb/cypress/views/netobserv.tsweb/cypress/views/network-health.tsweb/cypress/views/operator-hub-page.tsweb/cypress/views/pages.tsweb/cypress/views/pods.tsweb/cypress/views/tour.tsweb/cypress/views/utils.tsweb/cypress/views/yaml-editor.ts
💤 Files with no reviewable changes (9)
- web/cypress/views/tour.ts
- web/cypress/views/dashboards-page.ts
- web/cypress/views/catalogs.ts
- web/cypress/views/nav.ts
- web/cypress/views/pages.ts
- web/cypress/views/utils.ts
- web/cypress/views/pods.ts
- web/cypress/views/list-page.ts
- web/cypress/views/yaml-editor.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1460 +/- ##
=======================================
Coverage 52.42% 52.42%
=======================================
Files 233 233
Lines 12452 12452
Branches 1564 1564
=======================================
Hits 6528 6528
Misses 5310 5310
Partials 614 614
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/cypress/integration-tests/network_health.cy.ts (1)
28-28: Clear the filter input before typing to avoid sticky-state flakes.Line 28 types directly into the field; if a prior value persists, this appends and can intermittently fail the alert-name checks.
Suggested stabilization
- cy.byTestID('name-filter-input').type('DNSNxDomain_PerDst' + '{enter}') + cy.byTestID('name-filter-input') + .should('be.visible') + .clear() + .type('DNSNxDomain_PerDst{enter}')As per coding guidelines,
web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/network_health.cy.ts` at line 28, Clear the filter input before typing to avoid sticky-state flakes: update the cy.byTestID('name-filter-input').type(...) usage to first call cy.byTestID('name-filter-input').clear() (or .clear({force:true}) if necessary) and then .type('DNSNxDomain_PerDst{enter}'), and ensure any prior assertions/waits that depend on the filter use cy.get/cy.byTestID for the resulting list to stabilize the check.web/cypress/integration-tests/topology_edges_labels.cy.ts (1)
28-28: Use stable selectors instead of visible text for “Display options”.
cy.contains('Display options')is brittle to copy/localization changes. Prefer a dedicated test id / page-object selector here for resilient E2E tests.As per coding guidelines "web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience".
Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/topology_edges_labels.cy.ts` at line 28, Replace the brittle cy.contains('Display options') calls with a stable test selector (e.g., data-cy or data-testid)—update the DOM to include a dedicated attribute like data-cy="display-options" and change the Cypress calls that reference the visible text (the cy.contains usages for "Display options" at both occurrences) to use cy.get('[data-cy="display-options"]') with any necessary waits/assertions; ensure the new selector is used consistently in topology_edges_labels.cy.ts for both places mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/support/commands.ts`:
- Around line 366-386: There are duplicate registrations and a duplicate type
declaration for the Cypress custom command "cliLogin": remove the second
Cypress.Commands.add("cliLogin", ...) block (the one that duplicates the
implementation starting at the later declaration) and delete the duplicated
TypeScript declaration for the cliLogin command (the redundant type signature
that mirrors the original). Keep the first/primary
Cypress.Commands.add("cliLogin", ...) implementation and its single type
declaration, ensuring no other references to the removed duplicates remain.
---
Nitpick comments:
In `@web/cypress/integration-tests/network_health.cy.ts`:
- Line 28: Clear the filter input before typing to avoid sticky-state flakes:
update the cy.byTestID('name-filter-input').type(...) usage to first call
cy.byTestID('name-filter-input').clear() (or .clear({force:true}) if necessary)
and then .type('DNSNxDomain_PerDst{enter}'), and ensure any prior
assertions/waits that depend on the filter use cy.get/cy.byTestID for the
resulting list to stabilize the check.
In `@web/cypress/integration-tests/topology_edges_labels.cy.ts`:
- Line 28: Replace the brittle cy.contains('Display options') calls with a
stable test selector (e.g., data-cy or data-testid)—update the DOM to include a
dedicated attribute like data-cy="display-options" and change the Cypress calls
that reference the visible text (the cy.contains usages for "Display options" at
both occurrences) to use cy.get('[data-cy="display-options"]') with any
necessary waits/assertions; ensure the new selector is used consistently in
topology_edges_labels.cy.ts for both places mentioned.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a0cb190-7de7-4a1c-9155-8b839635e99d
📒 Files selected for processing (6)
web/cypress/integration-tests/netflow_export.cy.tsweb/cypress/integration-tests/network_health.cy.tsweb/cypress/integration-tests/topology_edges_labels.cy.tsweb/cypress/support/commands.tsweb/cypress/views/netobserv.tsweb/cypress/views/network-health.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/cypress/views/netobserv.ts
- web/cypress/views/network-health.ts
- web/cypress/integration-tests/netflow_export.cy.ts
memodi
reviewed
May 7, 2026
Member
|
/lgtm |
Member
|
@Amoghrd - could you give a run on jenkins please? if the run is from PR description is from jenkins, ignore this |
Member
Author
|
The run pasted was on terminal. Not sure why that would change when run on jenkins🤔 |
Member
it rules out "works on my system" (only) situation and uncover issues that we would typically run in CI where environment is built afresh especially for major changes such as this. |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR applies multiple improvements to enhance Cypress test stability, security, and maintainability based on CodeRabbit review suggestions.
Bug Fixes
netflow-page.ts(missing closing quote in attribute selector)gateway-api.yamlfor proper Gateway routing{ force: true }toclearAllFilters()to bypass Cypress actionability checks and ensure filters are cleared reliablyTest Reliability Improvements
netflow_export.cy.tswith robust file filtering and proper timeout handlingtopology_edges_labels.cy.tsto fix race condition by movingselectScopeGroupinto individual tests where intercept is registeredlist-page.tswith scoped menu lookupsquickFilters.cy.tswith scoped dropdown selectionsnetobserv-logo.tsto use retryable selectorsnetobserv.tsvisitFlowcollectorto catch missing hrefs earlynetwork-health.tsto prevent element detachment issuessetupWithNamespaceFiltersincevisit()already clears filtersSecurity Enhancements
JSON.stringify()incommands.tscliLoginfor credentials and pathsnetobserv.tsto handle paths with spaceslog: falseflagTest Plan
Note: The 2 test failures in full suite run (client_performance topology + dns_dashboards) pass when run individually, indicating environment/timing issues that occur only during consecutive test execution, not actual code problems.
🤖 Generated with Claude Code
Changes based on PR #1459
Test Results (Full Suite):
Failing tests passed on individual rerun ✅
Summary by CodeRabbit
Tests
Chores