NETOBSERV-2683 fix dropdowns state change#1378
Conversation
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:cddf49a
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=cddf49a make set-plugin-image |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1378 +/- ##
==========================================
+ Coverage 51.29% 51.34% +0.04%
==========================================
Files 225 225
Lines 12117 12107 -10
Branches 1516 1516
==========================================
Hits 6216 6216
+ Misses 5319 5309 -10
Partials 582 582
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@jpinsonneau is this PR dependent on #1365 ? or could it be tested standalone? |
|
/ok-to-test |
That's fully independant 😉 |
|
@jpinsonneau in recent run, the fields.spec.ts is failing for FlowRTT: any ideas how this got flaky? |
|
@jpinsonneau I ran bunch of regression tests that uses dropdown, could you include this commit in this PR? |
|
[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 |
github-actions
Bot
removed
the
ok-to-test
Sure, done in 8abdb71 |
|
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:
📝 WalkthroughWalkthroughReplaced blur-based delayed closing on multiple PatternFly dropdowns by wiring Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/netflow-page.ts`:
- Around line 55-57: The test clicks the refresh toggle twice around selecting
OFF_KEY which can re-open the dropdown (due to setOpen(!isOpen)); remove the
redundant cy.wrap($btn).click({ force: true }) after
cy.get('[data-test="OFF_KEY"]').should('exist').click({ force: true }) so the
sequence is: open toggle, click OFF_KEY, and do not click the toggle
again—update the test in netflow-page.ts to drop the third click referencing
$btn and keep only the initial toggle click and the OFF_KEY click.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d66497cd-460f-46b5-aa4f-2a6ef43c4189
📒 Files selected for processing (11)
web/cypress/views/netflow-page.tsweb/src/components/dropdowns/group-dropdown.tsxweb/src/components/dropdowns/layout-dropdown.tsxweb/src/components/dropdowns/match-dropdown.tsxweb/src/components/dropdowns/metric-type-dropdown.tsxweb/src/components/dropdowns/refresh-dropdown.tsxweb/src/components/dropdowns/scope-dropdown.tsxweb/src/components/dropdowns/time-range-dropdown.tsxweb/src/components/dropdowns/truncate-dropdown.tsxweb/src/components/toolbar/filters/compare-filter.tsxweb/src/components/toolbar/links-overflow.tsx
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:3855820
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=3855820 make set-plugin-image |
github-actions
Bot
removed
the
ok-to-test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/cypress/e2e/table/fields.spec.ts (1)
100-101: Replace repeated magic row indices with named constants.Line 101, Line 121, Line 137, and Line 148 hardcode row numbers in multiple tests. Centralizing them will make fixture-order updates safer and reduce churn.
♻️ Proposed refactor
describe('netflow-table', () => { + const ROW_WITH_PKT_DROP_FLOWRTT_ZONES = 6; + const ROW_WITH_NETWORK_EVENTS = 7; + const updatedData = r; updatedData.data.result.forEach(r => { @@ it('display pktDrop', () => { // select seventh row (has pktDrop data) - cy.get('#netflow-table-row-6').click(); + cy.get(`#netflow-table-row-${ROW_WITH_PKT_DROP_FLOWRTT_ZONES}`).click(); @@ it('display flowRTT', () => { // select seventh row (has flowRTT = 4047000ns = 4.05ms) - cy.get('#netflow-table-row-6').click(); + cy.get(`#netflow-table-row-${ROW_WITH_PKT_DROP_FLOWRTT_ZONES}`).click(); @@ it('display zones', () => { // select seventh row (has zone data: eu-west-1 and us-east-2) - cy.get('#netflow-table-row-6').click(); + cy.get(`#netflow-table-row-${ROW_WITH_PKT_DROP_FLOWRTT_ZONES}`).click(); @@ it('display networkEvents', () => { // select eighth row (has NetworkEvents data) - cy.get('#netflow-table-row-7').click(); + cy.get(`#netflow-table-row-${ROW_WITH_NETWORK_EVENTS}`).click();Also applies to: 120-121, 136-137, 147-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/table/fields.spec.ts` around lines 100 - 101, Replace hardcoded numeric row indices used in selectors with named constants: define constants near the top of the spec (for example const PKT_DROP_ROW = 6) and replace every occurrence of selectors like '#netflow-table-row-6' (and other '#netflow-table-row-N' uses found later) with template strings that interpolate the constant, e.g. cy.get(`#netflow-table-row-${PKT_DROP_ROW}`).click(); do this for each distinct row index used across the tests so the indices are centralized and easy to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/cypress/e2e/table/fields.spec.ts`:
- Around line 100-101: Replace hardcoded numeric row indices used in selectors
with named constants: define constants near the top of the spec (for example
const PKT_DROP_ROW = 6) and replace every occurrence of selectors like
'#netflow-table-row-6' (and other '#netflow-table-row-N' uses found later) with
template strings that interpolate the constant, e.g.
cy.get(`#netflow-table-row-${PKT_DROP_ROW}`).click(); do this for each distinct
row index used across the tests so the indices are centralized and easy to
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcab0b52-4cd2-4faf-972f-50ca17518474
📒 Files selected for processing (1)
web/cypress/e2e/table/fields.spec.ts
|
Rebase without changes |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/cypress/views/netflow-page.ts (1)
42-49: Consider applying the same visibility wait pattern here for consistency.
setAutoRefreshclicks[data-test="15s"]immediately after opening the dropdown without waiting for menu visibility. This could exhibit similar flakiness.Suggested fix
setAutoRefresh: () => { cy.byTestID(genSelectors.refreshDrop).should('exist').then($btn => { // only set refresh if it's OFF if ($btn.text() == "Refresh off") { cy.wrap($btn).click({ force: true }) + // Wait for dropdown menu to be rendered and visible + cy.get('.pf-v5-c-menu').should('be.visible') - cy.get('[data-test="15s"]').should('exist').click() + cy.get('[data-test="15s"]') + .should('be.visible') + .click() } }) },🤖 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 42 - 49, setAutoRefresh currently opens the refresh dropdown and immediately clicks the '[data-test="15s"]' item which can be flaky; update the function (identifier: setAutoRefresh) to wait for the dropdown menu to be visible before clicking the 15s option by asserting visibility on the menu or the '[data-test="15s"]' element (use the same visibility wait pattern used elsewhere with genSelectors.refreshDrop and then call click on '[data-test="15s"]' only after a should('be.visible')/should('exist') confirmation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/cypress/views/netflow-page.ts`:
- Around line 42-49: setAutoRefresh currently opens the refresh dropdown and
immediately clicks the '[data-test="15s"]' item which can be flaky; update the
function (identifier: setAutoRefresh) to wait for the dropdown menu to be
visible before clicking the 15s option by asserting visibility on the menu or
the '[data-test="15s"]' element (use the same visibility wait pattern used
elsewhere with genSelectors.refreshDrop and then call click on
'[data-test="15s"]' only after a should('be.visible')/should('exist')
confirmation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbf6c412-f85d-464d-89a2-6a2209214c6d
📒 Files selected for processing (12)
web/cypress/e2e/table/fields.spec.tsweb/cypress/views/netflow-page.tsweb/src/components/dropdowns/group-dropdown.tsxweb/src/components/dropdowns/layout-dropdown.tsxweb/src/components/dropdowns/match-dropdown.tsxweb/src/components/dropdowns/metric-type-dropdown.tsxweb/src/components/dropdowns/refresh-dropdown.tsxweb/src/components/dropdowns/scope-dropdown.tsxweb/src/components/dropdowns/time-range-dropdown.tsxweb/src/components/dropdowns/truncate-dropdown.tsxweb/src/components/toolbar/filters/compare-filter.tsxweb/src/components/toolbar/links-overflow.tsx
✅ Files skipped from review due to trivial changes (3)
- web/src/components/dropdowns/layout-dropdown.tsx
- web/src/components/dropdowns/refresh-dropdown.tsx
- web/src/components/toolbar/links-overflow.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- web/src/components/toolbar/filters/compare-filter.tsx
- web/src/components/dropdowns/truncate-dropdown.tsx
- web/src/components/dropdowns/metric-type-dropdown.tsx
- web/src/components/dropdowns/match-dropdown.tsx
- web/src/components/dropdowns/time-range-dropdown.tsx
- web/src/components/dropdowns/scope-dropdown.tsx
- web/cypress/e2e/table/fields.spec.ts
|
/retest |
|
looks like the the |
We can try if you think it's related but that's not supposed to take that much ! Do we have access to the log mentionned |
unfortunately no, other than what we see: testing it here openshift/release#77487 |
|
/retest |
|
Rebased without changes |
|
@jpinsonneau images building fine now, but overview/fields/table tests are still failing here. |
Co-authored-by: Mehul Modi <memodi@redhat.com>
|
Rebased and fixed cypress tests @memodi it seems some issues are coming from others PRs. Let's fix everything here and merge 👍 |
|
thanks @jpinsonneau , I see it's passing now, re-running one more time to ensure it's stable. |
|
@jpinsonneau: 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. |
|
/lgtm |
|
/label qe-approved |
3 participants
Description
Replaced the
onBlurtrick byonOpenChange. That should fix cypress flakiness.Dependencies
n/a
Checklist
Summary by CodeRabbit
Refactor
Tests