NETOBSERV-2653 & NETOBSERV-2689: TLS updates and PF pre-migration refactor changes#1395
NETOBSERV-2653 & NETOBSERV-2689: TLS updates and PF pre-migration refactor changes#1395memodi merged 3 commits intonetobserv:mainfrom
Conversation
Amoghrd
changed the title
|
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:
📝 WalkthroughWalkthroughTightened regex assertions in two dashboard tests, made the DSCP column test explicitly interact with the columns modal (checks Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
|
@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: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/cypress/integration-tests/table_queryopts.cy.ts (1)
109-115: Re-assert refresh state after reload to avoid flaky DSCP checks.
stopAutoRefresh()is called beforecy.reload(). If reload restores default refresh, the table may update while validating DSCP values. Re-apply or assert refresh-off after Line 114.Proposed adjustment
it("(OCP-68125, aramesha, Network_Observability) should verify DSCP column", function () { netflowPage.stopAutoRefresh() cy.openColumnsModal().then(col => { cy.get(colSelectors.columnsModal).should('be.visible') cy.get('#Dscp').check() }) cy.reload() + netflowPage.stopAutoRefresh() cy.byTestID('table-composable').should('exist').within(() => { cy.get(colSelectors.dscp).should('exist') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/table_queryopts.cy.ts` around lines 109 - 115, After reloading the page, ensure the auto-refresh remains disabled before interacting with the DSCP checkbox: after cy.reload() call either re-invoke netflowPage.stopAutoRefresh() or assert the refresh state is off (e.g., via a visible UI indicator or a method like netflowPage.isAutoRefreshEnabled() returning false), then reopen the columns modal (using cy.openColumnsModal()) and proceed to check '#Dscp' (refer to colSelectors.columnsModal and the existing stopAutoRefresh() call); this prevents the table from refreshing during the DSCP assertion and avoids flakiness.
🤖 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/integration-tests/table_queryopts.cy.ts`:
- Around line 109-115: After reloading the page, ensure the auto-refresh remains
disabled before interacting with the DSCP checkbox: after cy.reload() call
either re-invoke netflowPage.stopAutoRefresh() or assert the refresh state is
off (e.g., via a visible UI indicator or a method like
netflowPage.isAutoRefreshEnabled() returning false), then reopen the columns
modal (using cy.openColumnsModal()) and proceed to check '#Dscp' (refer to
colSelectors.columnsModal and the existing stopAutoRefresh() call); this
prevents the table from refreshing during the DSCP assertion and avoids
flakiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff7e7648-9c93-4b60-86c7-84257f93a9d5
📒 Files selected for processing (1)
web/cypress/integration-tests/table_queryopts.cy.ts
Amoghrd
changed the title
|
TLS PRs merged |
|
[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 |
|
@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: 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/cypress/views/netobserv.ts (1)
145-145: Selector change looks correct but relies on fragile:nth-child(2).Waiting for
loading-box__loadedbefore checking gridcell existence is the right approach—ensures the DOM is in a stable loaded state. However,:nth-child(2)is brittle; any DOM restructuring will break this.Consider extracting this selector into a constant (like the
pluginSelectorspattern used elsewhere) to make it easier to update if the DOM changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/netobserv.ts` at line 145, Extract the fragile selector 'div.loading-box__loaded:nth-child(2)' into a named constant (following the pluginSelectors pattern used elsewhere) and replace the inline string in the cy.get call with that constant; while extracting, prefer a more robust selector (e.g., a class-only '.loading-box__loaded' combined with a stable data-test attribute or a deterministic index like .eq(1) if necessary) so the test doesn't rely on DOM child ordering — update the constant name (e.g., LOADING_BOX_LOADED_SECOND) and change the cy.get('div.loading-box__loaded:nth-child(2)') usage to cy.get(LOADING_BOX_LOADED_SECOND).web/cypress/views/dashboards-page.ts (1)
59-67: Replace fixed delay with condition-based retry.The hard-coded
cy.wait(3000)on line 63 slows tests without guaranteeing the metrics data loads. Instead, rely on Cypress retries with the existing timeouts to wait until the condition is met (no empty-state class).Proposed refactor
- // Give dashboards additional time to load metrics data - cy.wait(3000) - - // Check that the graph body doesn't have empty state class - cy.byTestID(names[i]).find(graphSelector.graphBody, { timeout: 120000 }) - .should('not.have.class', 'pf-v6-c-empty-state') + // Check graph body once data is rendered (retry-driven, no fixed sleep) + cy.byTestID(names[i], { timeout: 120000 }) + .find(graphSelector.graphBody, { timeout: 120000 }) + .should('not.have.class', 'pf-v6-c-empty-state')🤖 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 59 - 67, Remove the hard-coded cy.wait(3000) and rely on Cypress' retry/timeouts to wait for metrics to load; keep the existing existence check cy.byTestID(names[i], { timeout: 120000 }).should('exist') and then use cy.byTestID(names[i]).find(graphSelector.graphBody, { timeout: 120000 }).should('not.have.class', 'pf-v6-c-empty-state') (or chain the find off the initial cy.byTestID call) so the test retries until the graph body no longer has the empty-state class instead of using a fixed delay.
🤖 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/dashboards-page.ts`:
- Around line 59-67: Remove the hard-coded cy.wait(3000) and rely on Cypress'
retry/timeouts to wait for metrics to load; keep the existing existence check
cy.byTestID(names[i], { timeout: 120000 }).should('exist') and then use
cy.byTestID(names[i]).find(graphSelector.graphBody, { timeout: 120000
}).should('not.have.class', 'pf-v6-c-empty-state') (or chain the find off the
initial cy.byTestID call) so the test retries until the graph body no longer has
the empty-state class instead of using a fixed delay.
In `@web/cypress/views/netobserv.ts`:
- Line 145: Extract the fragile selector 'div.loading-box__loaded:nth-child(2)'
into a named constant (following the pluginSelectors pattern used elsewhere) and
replace the inline string in the cy.get call with that constant; while
extracting, prefer a more robust selector (e.g., a class-only
'.loading-box__loaded' combined with a stable data-test attribute or a
deterministic index like .eq(1) if necessary) so the test doesn't rely on DOM
child ordering — update the constant name (e.g., LOADING_BOX_LOADED_SECOND) and
change the cy.get('div.loading-box__loaded:nth-child(2)') usage to
cy.get(LOADING_BOX_LOADED_SECOND).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f1ecb33-5648-49dc-84f3-ecb30e0a69a6
📒 Files selected for processing (6)
web/cypress/integration-tests/dns_dashboards.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/table_queryopts.cy.tsweb/cypress/views/dashboards-page.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/cypress/integration-tests/dns_dashboards.cy.ts
- web/cypress/integration-tests/table_queryopts.cy.ts
|
@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: 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. |
|
/retest |
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/table_queryopts.cy.ts`:
- Around line 109-113: The test opens the columns modal with openColumnsModal(),
checks the '#Dscp' checkbox, then reloads without persisting the selection so
the change can be lost; modify the test to explicitly persist the selection by
interacting with the modal's save/confirm control (the modal referenced by
colSelectors.columnsModal) after checking '#Dscp' before calling cy.reload(),
ensuring the checked state survives the reload.
- Around line 108-114: Reintroduce the "refresh-off" guard around the DSCP
column test to avoid racey assertions: call the existing auto-refresh disable
helper before invoking cy.openColumnsModal() (so table auto-updates won't mutate
state while checking colSelectors.columnsModal and toggling '#Dscp'), perform
the check and cy.reload(), then re-enable auto-refresh after the reload using
the corresponding enable helper; ensure the disable/enable calls bracket the
sequence to keep the test deterministic.
🪄 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: e0d3172c-5135-4db5-ac89-4f3a214c7e94
📒 Files selected for processing (4)
web/cypress/integration-tests/dns_dashboards.cy.tsweb/cypress/integration-tests/flowRTT_dashboards.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/table_queryopts.cy.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/integration-tests/netflow_external_subnet.cy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/cypress/integration-tests/dns_dashboards.cy.ts
- web/cypress/integration-tests/flowRTT_dashboards.cy.ts
|
/retest |
Amoghrd
force-pushed
the
netobserv-2653
branch
3 times, most recently
from
2f4faa6 to
38bdf61
Compare
|
/hold |
Amoghrd
force-pushed
the
netobserv-2653
branch
2 times, most recently
from
551abd2 to
d34d7e2
Compare
|
/unhold |
|
All tests are now passing noo-frontend-tests#44 |
|
@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 "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. |
|
/retest |
1 similar comment
|
/retest |
|
@Amoghrd: The following tests 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 |
|
merging manually since plugin-cypress is failing. |
4 participants
Description
Dependencies
#1398
Checklist
Jenkins run: noo-frontend-tests#36
Another run: noo-frontend-tests#41
Summary by CodeRabbit