NETOBSERV-2653: Patternflyv6 updates#1426
Open
Amoghrd wants to merge 9 commits intonetobserv:mainfrom
Open
Conversation
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2653 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 "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughRefactors Cypress tests and view helpers across the web Cypress suite: replaces brittle positional/selectors with text/test-id based selectors, simplifies topology scope calls, consolidates view selectors, and updates several assertions (including field labels and a Flow RTT value). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Amoghrd
changed the title
openshift-ci
bot
added
do-not-merge/work-in-progress
needs-rebase
and removed
needs-rebase
labels
Amoghrd
force-pushed
the
netobserv-2653-v6-updates
branch
5 times, most recently
from
3910400 to
b5702ad
Compare
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2653 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 "4.22.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. |
Amoghrd
changed the title
Member
Author
|
Have fixed the failing tests(network_health, netflow_table and netflow_export) from ginkgo job. Will rerun them on monday to check if they are passing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1426 +/- ##
=======================================
Coverage 52.48% 52.48%
=======================================
Files 229 229
Lines 12246 12246
Branches 1531 1531
=======================================
Hits 6427 6427
Misses 5224 5224
Partials 595 595
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Amoghrd
force-pushed
the
netobserv-2653-v6-updates
branch
from
b5702ad to
fcad18b
Compare
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2653 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 "4.22.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. |
Amoghrd
force-pushed
the
netobserv-2653-v6-updates
branch
from
fcad18b to
30fb22f
Compare
![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: 6
🧹 Nitpick comments (3)
web/cypress/views/yaml-editor.ts (1)
99-104: Narrow the click scope to avoid flaky cross-item matches.Using
.parents('li')+.find('button')can target a different item’s button in nested lists. Prefer nearest ancestor scoping.As per coding guidelines, "`web/cypress/**/*.ts`: Verify E2E test stability, proper waits, and selector resilience".Proposed change
export const clickFieldDetailsButton = (fieldName: string) => { cy.contains('h5', `${fieldName}`) - .parents('li') - .find('button') - .contains('View details') - .click(); + .closest('li') + .within(() => { + cy.contains('button', 'View details').click(); + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/yaml-editor.ts` around lines 99 - 104, The selector in clickFieldDetailsButton is too broad — using .parents('li').find('button') can match buttons from other nested items; change to scope to the nearest list item and then click the button within that scope. Update clickFieldDetailsButton to use cy.contains('h5', fieldName).closest('li') (or .closest('li').within(() => cy.contains('button', 'View details').click())) so the lookup is limited to the nearest ancestor li and the button lookup uses the 'button' context.web/cypress/views/operator-hub-page.ts (1)
47-47: Scope the status selector to operator tiles.Line 47 uses a page-wide
[role="status"], which can match unrelated status elements and introduce flaky assertions. Scope it to tile containers.Proposed change
getAllTileLabels: () => { - return cy.get('[role="status"]') + return cy.get('.co-catalog-tile').find('[role="status"]') },As per coding guidelines,
web/cypress/**/*.tsmust "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/views/operator-hub-page.ts` at line 47, The selector currently returns a page-wide '[role="status"]' which can match unrelated elements; update the call that returns this selector so it scopes the status to operator tile containers (for example replace with a descendant selector like '[data-testid="operator-tile"] [role="status"]' or use cy.get('[data-testid="operator-tile"]').find('[role="status"]') / '.operator-tile [role="status"]' depending on the DOM) to ensure the status element is fetched only from operator tiles and avoid flaky assertions.web/cypress/views/netobserv.ts (1)
226-227: Scope the finalSubmitclick to the wizard footer.Line 227 uses a page-wide
contains('button', 'Submit'), which can click the wrong button if another submit action is present. Scope it to the form footer to reduce flakiness.Proposed tweak
- cy.contains('button', 'Submit').should('exist').click() + cy.get('footer').contains('button[type="submit"]', 'Submit').should('be.visible').click()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/views/netobserv.ts` around lines 226 - 227, The page-wide cy.contains('button', 'Submit') call in the "Consumption tab - final submit" step is too broad; scope the lookup to the wizard/footer region to avoid clicking the wrong submit button by first selecting the wizard footer container (for example the footer element or a data-testid like wizard-footer) and then calling contains on that container for the 'Submit' button, keeping the existing should('exist') and click flow to preserve assertions.
🤖 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: Replace the flaky fixed sleep and ambiguous ls parsing:
remove the cy.wait(3000) and instead wait for the download file to appear by
polling the downloads directory and selecting the newest file; replace the raw
cy.exec("ls cypress/downloads") + cy.wrap(response.stdout) usage with a
deterministic listing (e.g., cy.exec('ls -t cypress/downloads | head -n1') or a
Node task that returns the newest file) and trim newlines before using the
filename, then move/rename and cy.readFile('cypress/downloads/export_table.csv')
only after confirming the file exists; update the code around cy.exec, cy.wrap,
and cy.readFile to use this deterministic approach so tests are stable and
handle multiple files.
In `@web/cypress/integration-tests/netflow_table.cy.ts`:
- Around line 151-153: The test currently re-queries the entire column inside
the each callback (cy.get('[data-test-td-column-id=SrcPort]').each((td) => {
cy.get('[data-test-td-column-id=SrcPort]').should(...)})), which validates the
whole column instead of the current cell; update the each callbacks to use
cy.wrap(td).should('not.contain.text', 'loki (3100)') so each cell is asserted
individually, and apply the same change to the other similar block mentioned
(the second occurrence around lines 159-161).
In `@web/cypress/integration-tests/quickFilters.cy.ts`:
- Around line 52-53: The test is querying any checkbox inside
`#quick-filters-dropdown` which is too broad and brittle; update both occurrences
(the checks at the lines referencing `#quick-filters-dropdown`) to scope the
checkbox to the "Test NS" filter label instead of selecting all
checkboxes—locate the label text "Test NS" (e.g., with cy.contains or within)
and then find the associated input[type="checkbox"] from that label/container
before asserting/clicking, and ensure any necessary waits or assertions for the
dropdown/label visibility are added to stabilize the E2E step.
In `@web/cypress/integration-tests/table_queryopts.cy.ts`:
- Around line 126-129: The assertion currently re-queries all Dscp cells inside
the .each loop, losing scope; instead, inside the each callback use the td
element directly (wrap the td with cy.wrap(td)) to assert its attributes and
text so each row is validated individually: reference the existing
cy.get('[data-test-td-column-id=Dscp]') .each((td) => ...) loop and replace the
global cy.get(...) text check with cy.wrap(td).should('contain.text',
'Standard') and assert the attribute on cy.wrap(td) (e.g.,
should('have.attr','data-test-td-value').and('contain','0')) so each td is
tested in scope.
In `@web/cypress/views/netflow-page.ts`:
- Around line 145-146: The selector confirmDel uses Playwright-only syntax
'#delete-modal button:has-text("Delete")' which will break in Cypress; replace
it with a proper test-hook based selector (e.g. use an existing data-test-id on
the modal and the confirm button) and update confirmDel to point to that test
hook (reference symbol: confirmDel) instead of using :has-text(), or
alternatively ensure tests use cy.contains(...) in the test code rather than
this Playwright selector; also keep the existing del constant
([data-test-id=delete-resource-button]) unchanged.
In `@web/cypress/views/network-health.ts`:
- Around line 14-25: The tests fail because clickOnAlert and verifyAlert use
non-existent data-test attributes; update both functions (clickOnAlert and
verifyAlert in web/cypress/views/network-health.ts) to use the id-based hooks
emitted by the component (replace `[data-test="health-card-${name}"]` with the
component's id selector, e.g. `#health-card-${name}` or the exact id attribute
used in web/src/components/health/health-card.tsx), keep the existing { timeout,
force } options and the sidePanel checks unchanged, and ensure the same selector
change is applied to both the initial click and the closing click so the test
targets the real DOM nodes.
---
Nitpick comments:
In `@web/cypress/views/netobserv.ts`:
- Around line 226-227: The page-wide cy.contains('button', 'Submit') call in the
"Consumption tab - final submit" step is too broad; scope the lookup to the
wizard/footer region to avoid clicking the wrong submit button by first
selecting the wizard footer container (for example the footer element or a
data-testid like wizard-footer) and then calling contains on that container for
the 'Submit' button, keeping the existing should('exist') and click flow to
preserve assertions.
In `@web/cypress/views/operator-hub-page.ts`:
- Line 47: The selector currently returns a page-wide '[role="status"]' which
can match unrelated elements; update the call that returns this selector so it
scopes the status to operator tile containers (for example replace with a
descendant selector like '[data-testid="operator-tile"] [role="status"]' or use
cy.get('[data-testid="operator-tile"]').find('[role="status"]') /
'.operator-tile [role="status"]' depending on the DOM) to ensure the status
element is fetched only from operator tiles and avoid flaky assertions.
In `@web/cypress/views/yaml-editor.ts`:
- Around line 99-104: The selector in clickFieldDetailsButton is too broad —
using .parents('li').find('button') can match buttons from other nested items;
change to scope to the nearest list item and then click the button within that
scope. Update clickFieldDetailsButton to use cy.contains('h5',
fieldName).closest('li') (or .closest('li').within(() => cy.contains('button',
'View details').click())) so the lookup is limited to the nearest ancestor li
and the button lookup uses the 'button' context.
🪄 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: ef81c7ae-ffe5-4194-b81a-ad35272f388f
📒 Files selected for processing (32)
web/cypress/e2e/table/fields.spec.tsweb/cypress/integration-tests/client_performance.cy.tsweb/cypress/integration-tests/dns_dashboards.cy.tsweb/cypress/integration-tests/dns_tracking.cy.tsweb/cypress/integration-tests/flowRTT.cy.tsweb/cypress/integration-tests/flowRTT_dashboards.cy.tsweb/cypress/integration-tests/gateway_topology_logo.cy.tsweb/cypress/integration-tests/ingress_dashboard.cy.tsweb/cypress/integration-tests/netflow_conversations.cy.tsweb/cypress/integration-tests/netflow_export.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/netflow_table.cy.tsweb/cypress/integration-tests/netflow_zone_multiCluster.cy.tsweb/cypress/integration-tests/netobserv_udn.cy.tsweb/cypress/integration-tests/network_health.cy.tsweb/cypress/integration-tests/overview_page.cy.tsweb/cypress/integration-tests/packet_drop.cy.tsweb/cypress/integration-tests/packet_drop_dashboards.cy.tsweb/cypress/integration-tests/quickFilters.cy.tsweb/cypress/integration-tests/table_queryopts.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/dashboards-page.tsweb/cypress/views/list-page.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.tsweb/cypress/views/network-health.tsweb/cypress/views/operator-hub-page.tsweb/cypress/views/pages.tsweb/cypress/views/yaml-editor.ts
Comment on lines
+47
to
+53
| cy.wait(3000) | ||
| // get the CSV file name | ||
| cy.exec("ls cypress/downloads").then((response) => { | ||
| // rename CSV file to export_table.csv | ||
| cy.wrap(response.stdout).should('not.be.empty') | ||
| cy.exec(`mv cypress/downloads/${response.stdout} cypress/downloads/export_table.csv`) | ||
| cy.readFile('cypress/downloads/export_table.csv') |
Contributor
There was a problem hiding this comment.
Replace fixed sleep and ambiguous ls parsing in download flow.
Line 47 (cy.wait(3000)) is timing-based and flaky. Lines 49-53 treat full ls stdout as one filename, which can fail when multiple files exist or include trailing newlines.
Proposed fix
- // wait for download to complete
- cy.wait(3000)
- // get the CSV file name
- cy.exec("ls cypress/downloads").then((response) => {
- // rename CSV file to export_table.csv
- cy.wrap(response.stdout).should('not.be.empty')
- cy.exec(`mv cypress/downloads/${response.stdout} cypress/downloads/export_table.csv`)
- cy.readFile('cypress/downloads/export_table.csv')
- })
+ // wait up to 20s for a CSV and pick the newest file deterministically
+ cy.exec(`bash -lc 'for i in {1..20}; do f=$(ls -1t cypress/downloads/*.csv 2>/dev/null | head -n1); [ -n "$f" ] && echo "$f" && exit 0; sleep 1; done; exit 1'`)
+ .then(({ stdout }) => {
+ const downloaded = stdout.trim()
+ cy.wrap(downloaded).should('not.be.empty')
+ cy.exec(`mv "${downloaded}" cypress/downloads/export_table.csv`)
+ cy.readFile('cypress/downloads/export_table.csv')
+ })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/netflow_export.cy.ts` around lines 47 - 53,
Replace the flaky fixed sleep and ambiguous ls parsing: remove the cy.wait(3000)
and instead wait for the download file to appear by polling the downloads
directory and selecting the newest file; replace the raw cy.exec("ls
cypress/downloads") + cy.wrap(response.stdout) usage with a deterministic
listing (e.g., cy.exec('ls -t cypress/downloads | head -n1') or a Node task that
returns the newest file) and trim newlines before using the filename, then
move/rename and cy.readFile('cypress/downloads/export_table.csv') only after
confirming the file exists; update the code around cy.exec, cy.wrap, and
cy.readFile to use this deterministic approach so tests are stable and handle
multiple files.
web/cypress/views/network-health.ts
Outdated
Comment on lines
+14
to
+25
| cy.get(`[data-test="health-card-${name}"]`, { timeout: 60000 }).eq(0).should('exist').click() | ||
| }, | ||
| verifyAlert: (name: string, mode: string = "alert", alertText?: string) => { | ||
| // click force since node cards are covered | ||
| cy.get(`label[for^="health-card-selectable-${name}"]`).eq(0).should('be.visible').click({ force: true }).then(() => { | ||
| cy.get(networkHealthSelectors.sidePanel).should('be.visible') | ||
| cy.contains(mode).should('exist') | ||
| if (alertText) { | ||
| cy.contains(alertText).should('exist') | ||
|
|
||
| } | ||
| cy.get(`label[for^="health-card-selectable-${name}"]`).eq(0).click() | ||
| cy.get(networkHealthSelectors.sidePanel).should('not.exist') | ||
| }) | ||
| cy.get(`[data-test="health-card-${name}"]`, { timeout: 60000 }).eq(0).should('exist').click({ force: true }) | ||
| cy.get(networkHealthSelectors.sidePanel, { timeout: 10000 }).should('be.visible') | ||
| cy.contains(mode).should('exist') | ||
| if (alertText) { | ||
| cy.contains(alertText).should('exist') | ||
| } | ||
| cy.get(`[data-test="health-card-${name}"]`).eq(0).click({ force: true }) | ||
| cy.get(networkHealthSelectors.sidePanel).should('not.exist') |
Contributor
There was a problem hiding this comment.
These health-card selectors do not exist on the rendered component.
web/src/components/health/health-card.tsx does not render data-test="health-card-..."; it exposes id-based hooks instead. Both clickOnAlert() and verifyAlert() will timeout here until the test switches back to an existing hook or the component adds the missing data-test attribute.
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/views/network-health.ts` around lines 14 - 25, The tests fail
because clickOnAlert and verifyAlert use non-existent data-test attributes;
update both functions (clickOnAlert and verifyAlert in
web/cypress/views/network-health.ts) to use the id-based hooks emitted by the
component (replace `[data-test="health-card-${name}"]` with the component's id
selector, e.g. `#health-card-${name}` or the exact id attribute used in
web/src/components/health/health-card.tsx), keep the existing { timeout, force }
options and the sidePanel checks unchanged, and ensure the same selector change
is applied to both the initial click and the closing click so the test targets
the real DOM nodes.
30fb22f to
ca59955
Compare
Collaborator
|
@Amoghrd: This pull request references NETOBSERV-2653 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 "4.22.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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/cypress/views/dashboards-page.ts (1)
39-45: Tighten toggle selectors to avoid ambiguous button matchesLine 39-45 now match any descendant
buttoninside each panel. That can click the wrong control if additional buttons are added to card actions/header. Prefer a more specific toggle target (e.g., dedicated test id or a unique button attribute).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/views/dashboards-page.ts` around lines 39 - 45, The current selectors (flowStatsToggle, ebpfStatsToggle, operatorStatsToggle, resourceStatsToggle, top10PerRouteToggle, top10PerNamespaceToggle, top10PerShardToggle) target any descendant "button" inside the panel which is ambiguous; update each selector to a more specific target (for example target a dedicated toggle button attribute or a direct child with a unique class/test-id such as the actual toggle button element like button[data-test-id="panel-*-toggle"] or use a direct-child selector) so clicks hit the intended toggle only; change the selector strings to reference the unique toggle element or attribute for each exported constant and ensure they remain resilient to added action/header buttons.web/cypress/views/list-page.ts (1)
62-65: Avoid positional targeting in DataView filter toggle.Line 62 uses
.first()onbutton, which is DOM-order dependent and can become flaky after PatternFly/layout updates. Prefer a semantic target (role/label/text) scoped to the same DataView filter container.Suggested refactor
- cy.get('[data-ouia-component-id="DataViewFilters"]').within(() => - cy.get('button').first().click(), - ); - cy.contains('li', 'Name').click(); + cy.get('[data-ouia-component-id="DataViewFilters"]').within(() => { + cy.contains('button', /^Name$/).click(); + }); + cy.contains('[role="option"], li', /^Name$/).click();As per coding guidelines, "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/views/list-page.ts` around lines 62 - 65, Replace the brittle positional selector cy.get('button').first().click() with a semantic, scoped selector targeting the DataView filter toggle (e.g., select by role/aria-label/text within the same DataView filter container) and scope the subsequent interactions (the cy.contains('li','Name').click() and cy.get('[aria-label="Name filter"]')...) to that container (using .within() or .find()) so the toggle and Name filter are located reliably without relying on DOM order; also ensure any necessary waits/assertions after opening the filter are present.
🤖 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/views/dashboards-page.ts`:
- Line 49: The selector assigned to graphBody ('[role="region"]') is too
generic; change the graphBody constant in web/cypress/views/dashboards-page.ts
to target a stable, specific hook such as a data-test/data-cy attribute (e.g.,
'[data-cy="graph-container"]' or '[data-test="graph-container"]'), update any
tests that reference graphBody to use the new value, and if the application
markup lacks that attribute add the corresponding data-cy/data-test to the graph
container component so assertions are scoped narrowly and more reliable.
---
Nitpick comments:
In `@web/cypress/views/dashboards-page.ts`:
- Around line 39-45: The current selectors (flowStatsToggle, ebpfStatsToggle,
operatorStatsToggle, resourceStatsToggle, top10PerRouteToggle,
top10PerNamespaceToggle, top10PerShardToggle) target any descendant "button"
inside the panel which is ambiguous; update each selector to a more specific
target (for example target a dedicated toggle button attribute or a direct child
with a unique class/test-id such as the actual toggle button element like
button[data-test-id="panel-*-toggle"] or use a direct-child selector) so clicks
hit the intended toggle only; change the selector strings to reference the
unique toggle element or attribute for each exported constant and ensure they
remain resilient to added action/header buttons.
In `@web/cypress/views/list-page.ts`:
- Around line 62-65: Replace the brittle positional selector
cy.get('button').first().click() with a semantic, scoped selector targeting the
DataView filter toggle (e.g., select by role/aria-label/text within the same
DataView filter container) and scope the subsequent interactions (the
cy.contains('li','Name').click() and cy.get('[aria-label="Name filter"]')...) to
that container (using .within() or .find()) so the toggle and Name filter are
located reliably without relying on DOM order; also ensure any necessary
waits/assertions after opening the filter are present.
🪄 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: 8e2a9a10-e205-422e-bd3e-2c215b5606b3
📒 Files selected for processing (29)
web/cypress/integration-tests/client_performance.cy.tsweb/cypress/integration-tests/dns_dashboards.cy.tsweb/cypress/integration-tests/dns_tracking.cy.tsweb/cypress/integration-tests/flowRTT.cy.tsweb/cypress/integration-tests/flowRTT_dashboards.cy.tsweb/cypress/integration-tests/gateway_topology_logo.cy.tsweb/cypress/integration-tests/netflow_conversations.cy.tsweb/cypress/integration-tests/netflow_export.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/netflow_table.cy.tsweb/cypress/integration-tests/netflow_zone_multiCluster.cy.tsweb/cypress/integration-tests/netobserv_udn.cy.tsweb/cypress/integration-tests/network_health.cy.tsweb/cypress/integration-tests/overview_page.cy.tsweb/cypress/integration-tests/packet_drop.cy.tsweb/cypress/integration-tests/packet_drop_dashboards.cy.tsweb/cypress/integration-tests/quickFilters.cy.tsweb/cypress/integration-tests/table_queryopts.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/dashboards-page.tsweb/cypress/views/list-page.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.tsweb/cypress/views/network-health.tsweb/cypress/views/operator-hub-page.tsweb/cypress/views/pages.ts
✅ Files skipped from review due to trivial changes (11)
- web/cypress/integration-tests/dns_tracking.cy.ts
- web/cypress/integration-tests/netflow_conversations.cy.ts
- web/cypress/integration-tests/flowRTT.cy.ts
- web/cypress/integration-tests/netflow_external_subnet.cy.ts
- web/cypress/integration-tests/topology_view.cy.ts
- web/cypress/integration-tests/overview_page.cy.ts
- web/cypress/integration-tests/gateway_topology_logo.cy.ts
- web/cypress/integration-tests/packet_drop.cy.ts
- web/cypress/integration-tests/dns_dashboards.cy.ts
- web/cypress/integration-tests/netflow_zone_multiCluster.cy.ts
- web/cypress/integration-tests/netflow_table.cy.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- web/cypress/integration-tests/packet_drop_dashboards.cy.ts
- web/cypress/support/commands.ts
- web/cypress/views/operator-hub-page.ts
- web/cypress/integration-tests/quickFilters.cy.ts
- web/cypress/integration-tests/netobserv_udn.cy.ts
- web/cypress/integration-tests/topology_edges_labels.cy.ts
- web/cypress/integration-tests/topology_groups.cy.ts
- web/cypress/integration-tests/netflow_export.cy.ts
- web/cypress/views/netobserv.ts
- web/cypress/integration-tests/flowRTT_dashboards.cy.ts
- web/cypress/integration-tests/network_health.cy.ts
- web/cypress/integration-tests/client_performance.cy.ts
- web/cypress/integration-tests/table_queryopts.cy.ts
- web/cypress/views/network-health.ts
web/cypress/views/dashboards-page.ts
Outdated
|
|
||
| export const graphSelector = { | ||
| graphBody: '.pf-v6-c-card__body > div > div' | ||
| graphBody: '[role="region"]' |
Contributor
There was a problem hiding this comment.
graphBody is too generic for stable assertions
Line 49 ([role="region"]) is broad and may capture unrelated regions, making dashboard checks flaky. Use a graph-container selector with tighter scope (prefer a stable data-test hook).
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/views/dashboards-page.ts` at line 49, The selector assigned to
graphBody ('[role="region"]') is too generic; change the graphBody constant in
web/cypress/views/dashboards-page.ts to target a stable, specific hook such as a
data-test/data-cy attribute (e.g., '[data-cy="graph-container"]' or
'[data-test="graph-container"]'), update any tests that reference graphBody to
use the new value, and if the application markup lacks that attribute add the
corresponding data-cy/data-test to the graph container component so assertions
are scoped narrowly and more reliable.
…reliability Address major and nitpick issues identified in PR netobserv#1426 code review: Major fixes: - netflow_export.cy.ts: Replace fixed cy.wait(3000) with deterministic file polling using exec timeout - netflow_table.cy.ts: Fix .each() callbacks to use cy.wrap(td) instead of re-querying entire column (2 instances) - quickFilters.cy.ts: Scope checkbox selector to "Test NS" label instead of all checkboxes (2 instances) - table_queryopts.cy.ts: Fix Dscp cell assertion to use cy.wrap(td) in .each() loop - netflow-page.ts: Replace Playwright-only :has-text() syntax with proper Cypress data-test selector Nitpick fixes: - yaml-editor.ts: Replace .parents('li').find('button') with .closest('li').within() for better scoping - operator-hub-page.ts: Scope [role="status"] selector to .co-catalog-tile elements - netobserv.ts: Scope Submit button click to footer region These changes improve test reliability and follow Cypress best practices for selector specificity. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Scope [role="region"] selector to be more specific by requiring it to be within a [data-test-id] element. This prevents capturing unrelated regions on the page and makes dashboard assertions more stable. Addresses CodeRabbit review comment about overly generic selector. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add 120s timeout to the status-text check when waiting for flowcollector to reach Ready state. This prevents flaky test failures when flowcollector takes longer to reconcile after deployment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@Amoghrd: The following test 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. |
Member
|
Seems like you have some issues fixed in https://github.com/netobserv/netobserv-web-console/pull/1365/changes#diff-bbb920770f884a6314eb2ee333b15fd3cc85ad22e10ce7431b6d090e5fc9be76 Let's wait for PF6 PR to be merged to polish it 😉 |
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.
Description
Dependencies
#1365
Checklist
Jenkins run: noo-frontend-tests#48
Summary by CodeRabbit