NETOBSERV-2653: TLS Tracking automation#1528
Conversation
|
@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 "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. |
|
[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 |
📝 WalkthroughWalkthroughAdds TLSTracking FlowCollector fixture and wiring, refactors Cypress column selectors and tab navigation, implements a selectAndVerifyColumns helper, inlines test server/client fixture, updates many tests to use selectors/helpers, and adds TLS dashboard and TLSTracking end-to-end specs. ChangesTLS Tracking Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web/cypress/views/netflow-page.ts (1)
369-369: 💤 Low valueInconsistent tab selector pattern.
Line 369 still uses
li.tableTabButtonCSS selector while the other tab interactions (lines 372, 378) use#tabs-containerwith text matching. For consistency, consider updating this to match the new pattern or retaining the CSS selector approach throughout.🤖 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 `@web/cypress/views/netflow-page.ts` at line 369, The assertion uses a raw CSS selector cy.get('li.tableTabButton') which is inconsistent with other tab interactions that use the `#tabs-container` text-based pattern; update the check to use the same pattern as the other assertions (the `#tabs-container` + contains/text match used in the nearby checks) so the test consistently targets tabs the same way, or if you prefer CSS selectors, convert the other tab checks to use li.tableTabButton; locate the call to cy.get('li.tableTabButton') and replace it with the matching `#tabs-container-based` selector and text assertion used in the surrounding lines.web/cypress/integration-tests/tls_tracking.cy.ts (2)
31-35: 💤 Low valueDuplicate Loki query wait.
Two consecutive
waitForLokiQuery()calls (lines 31 and 34) are redundant. The second wait should be sufficient after panel selection.♻️ Simplify waits
cy.get(overviewSelectors.panelsModal).contains('Select all').click(); cy.get(overviewSelectors.panelsModal).contains('Save').click(); -netflowPage.waitForLokiQuery() -cy.checkPanelsNum(8); - netflowPage.waitForLokiQuery() +cy.checkPanelsNum(8); cy.checkPanel(overviewSelectors.allTLSTrackingPanels)🤖 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 `@web/cypress/integration-tests/tls_tracking.cy.ts` around lines 31 - 35, There are two consecutive netflowPage.waitForLokiQuery() calls; remove the first redundant call and keep a single wait before asserting panels so the test waits once for Loki results after panel interactions—update the snippet that uses netflowPage.waitForLokiQuery(), cy.checkPanelsNum(8), and cy.checkPanel(overviewSelectors.allTLSTrackingPanels) to call waitForLokiQuery() only once (retain the second occurrence that precedes cy.checkPanel).
77-90: ⚡ Quick winInconsistent selector usage - prefer colSelectors.
Lines 77-90 use hardcoded
[data-test-td-column-id="..."]selectors, but the PR refactored column selectors to usecolSelectorsconstants (e.g.,colSelectors.tlsVersion). Using constants improves maintainability and resilience.♻️ Use colSelectors for table columns
If
colSelectorsincludes table column selectors (e.g.,colSelectors.tlsVersionData), refactor to:-cy.get('[data-test-td-column-id="TLSVersion"]') +cy.get(colSelectors.tlsVersionData) // or appropriate constant .should('have.length.greaterThan', 0) .each((td) => { expect(td).to.contain('TLS 1.3') }) -cy.get('[data-test-td-column-id="TLSTypes"]').each((td) => { +cy.get(colSelectors.tlsTypesData).each((td) => { expect(td).to.contain('ServerHello') }) -cy.get('[data-test-td-column-id="TLSGroup"]').each((td) => { +cy.get(colSelectors.tlsGroupData).each((td) => { expect(td.text().trim()).to.not.be.empty }) -cy.get('[data-test-td-column-id="TLSCipherSuite"]').each((td) => { +cy.get(colSelectors.tlsCipherSuiteData).each((td) => { expect(td.text().trim()).to.not.be.empty })🤖 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 `@web/cypress/integration-tests/tls_tracking.cy.ts` around lines 77 - 90, Replace the hardcoded data-test selectors with the shared colSelectors constants: use colSelectors.tlsVersion (or colSelectors.tlsVersionData if naming uses *Data) instead of '[data-test-td-column-id="TLSVersion"]', colSelectors.tlsTypes for TLSTypes, colSelectors.tlsGroup for TLSGroup, and colSelectors.tlsCipherSuite for TLSCipherSuite; update the chained assertions and .each callbacks to call cy.get(colSelectors.<name>) so the .should('have.length.greaterThan', 0) and the expect checks operate on the constant-based selectors.
🤖 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 `@web/cypress/integration-tests/tls_dashboards.cy.ts`:
- Around line 30-32: After scrolling with
cy.get('#content-scrollable').scrollTo('bottom'), add an explicit wait for panel
load before calling cy.checkDashboards(TLSPanels): wait for a stable indicator
such as visibility/existence of the dashboard panel elements (eg. a data-test or
panel CSS selector) or for the network/resource that drives lazy-load to finish,
using a Cypress command with an appropriate timeout; this ensures
cy.checkDashboards(TLSPanels) runs only after panels are rendered and prevents
flakiness.
In `@web/cypress/views/netflow-page.ts`:
- Line 274: The current manageTLSTrackingPanelsList uses hardcoded English panel
title strings which will break in localized UIs; change it to a list of stable
test identifiers (e.g., "tls-usage-donut", "tls-usage-version", etc.), add
corresponding data-test-panel-id attributes to the panel components in the app
(e.g., data-test-panel-id="tls-usage-donut"), and update the tests that
currently call .contains(...) (the selector usage referencing
manageTLSTrackingPanelsList) to select by attribute (e.g.,
cy.get('[data-test-panel-id="..."]')) so the tests are resilient to
translations.
- Line 363: Tests now use text-matching like
cy.get('#tabs-container').contains('Overview') which breaks i18n; update the tab
clicks in checkNetflowTraffic() (and the other two occurrences) to use stable,
non-localized selectors: prefer data-test-id attributes on the tab elements
(e.g., overview-tab, traffic-flows-tab, topology-tab) and change the selectors
to target those data-test-id attributes, or if data-test-id isn't yet available
fall back to the original li.*TabButton class selector to avoid text matching;
ensure each replacement uses cy.get(...) with the attribute/class selector and
retains the existing click({ force: true })/wait logic.
- Around line 207-210: The selector constant tlsGroup is incorrect; update the
export to use the correct symbol and selector by renaming tlsGroup to tlsCurve
and changing its value from '#TLSGroup' to '#TLSCurve' so the exported constant
tlsCurve = '#TLSCurve' matches the app config and the tests (e.g.,
tls_tracking.cy.ts) that reference TLSCurve. Ensure any imports/usages of
tlsGroup elsewhere are updated to use tlsCurve.
---
Nitpick comments:
In `@web/cypress/integration-tests/tls_tracking.cy.ts`:
- Around line 31-35: There are two consecutive netflowPage.waitForLokiQuery()
calls; remove the first redundant call and keep a single wait before asserting
panels so the test waits once for Loki results after panel interactions—update
the snippet that uses netflowPage.waitForLokiQuery(), cy.checkPanelsNum(8), and
cy.checkPanel(overviewSelectors.allTLSTrackingPanels) to call waitForLokiQuery()
only once (retain the second occurrence that precedes cy.checkPanel).
- Around line 77-90: Replace the hardcoded data-test selectors with the shared
colSelectors constants: use colSelectors.tlsVersion (or
colSelectors.tlsVersionData if naming uses *Data) instead of
'[data-test-td-column-id="TLSVersion"]', colSelectors.tlsTypes for TLSTypes,
colSelectors.tlsGroup for TLSGroup, and colSelectors.tlsCipherSuite for
TLSCipherSuite; update the chained assertions and .each callbacks to call
cy.get(colSelectors.<name>) so the .should('have.length.greaterThan', 0) and the
expect checks operate on the constant-based selectors.
In `@web/cypress/views/netflow-page.ts`:
- Line 369: The assertion uses a raw CSS selector cy.get('li.tableTabButton')
which is inconsistent with other tab interactions that use the `#tabs-container`
text-based pattern; update the check to use the same pattern as the other
assertions (the `#tabs-container` + contains/text match used in the nearby checks)
so the test consistently targets tabs the same way, or if you prefer CSS
selectors, convert the other tab checks to use li.tableTabButton; locate the
call to cy.get('li.tableTabButton') and replace it with the matching
`#tabs-container-based` selector and text assertion used in the surrounding lines.
🪄 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: 2af2e419-f6a9-4059-8f2e-b6cab5b245a7
📒 Files selected for processing (8)
web/cypress/fixtures/flowcollector/fc_TLSTracking.yamlweb/cypress/integration-tests/dns_tracking.cy.tsweb/cypress/integration-tests/netflow_cluster_admin_group.cy.tsweb/cypress/integration-tests/netflow_external_subnet.cy.tsweb/cypress/integration-tests/tls_dashboards.cy.tsweb/cypress/integration-tests/tls_tracking.cy.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
ccfda34 to
7a9c70b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
web/cypress/integration-tests/quickFilters.cy.ts (1)
31-39: ⚡ Quick winFixed 10-second wait may cause flake or unnecessary delays.
Line 35 introduces a fixed
cy.wait(10000)before checking pod readiness. Since lines 38-39 use deterministicoc wait --for=condition=Readycommands with timeouts, the fixed delay is likely unnecessary. The readiness checks will poll until the pods are ready or time out. Consider removing the fixed wait to reduce test duration and eliminate an arbitrary delay.🤖 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 `@web/cypress/integration-tests/quickFilters.cy.ts` around lines 31 - 39, Remove the fixed delay by deleting the cy.wait(10000) call and rely on the deterministic readiness checks already present (the cy.adminCLI('oc wait --for=condition=Ready pod -l app=nginx -n test-server-56222 --timeout=120s') and cy.adminCLI('oc wait --for=condition=Ready pod -n test-client-56222 client --timeout=120s') calls); ensure those oc wait commands remain immediately after creating the pods (the cy.adminCLI('oc apply -f cypress/fixtures/test-server-client.yaml') block) so the test polls for readiness instead of sleeping.web/cypress/fixtures/test-server-client.yaml (1)
78-78: 💤 Low valueService FQDN hardcoded in client command.
The client pod's curl loop directly references
nginx-service.test-server-56222.svc:80. This tight coupling means the fixture cannot be reused with different namespace names without manual editing.🤖 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 `@web/cypress/fixtures/test-server-client.yaml` at line 78, The curl invocation "curl nginx-service.test-server-56222.svc:80/data/100K" hardcodes the service FQDN; change it to use a namespace variable so the fixture is reusable (e.g. set an env var like TEST_SERVER_NAMESPACE via env or downward API and replace the command with "curl nginx-service.${TEST_SERVER_NAMESPACE}.svc:80/data/100K" or use "$(POD_NAMESPACE)" if available), and ensure the pod spec defines that env var (TEST_SERVER_NAMESPACE or POD_NAMESPACE) so the command resolves at runtime.web/cypress/views/netobserv.ts (3)
143-145: 💤 Low valueRedundant loading-box wait + magic
cy.wait(3000).
Operator.visitFlowcollector()on line 143 already assertsdiv.loading-box__loadedexists (line 136). Re-asserting on line 144 and then sleeping 3s on line 145 is duplicated work and the only fixed-time wait left in this flow — a recipe for flake when the cluster is slow and wasted time when it's fast. Consider deleting the re-assert and replacing thecy.waitwith the actual readiness signal (#yaml-createis already asserted on line 146 with its own timeout).As per coding guidelines: "Verify E2E test stability, proper waits, and selector resilience".
🤖 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 `@web/cypress/views/netobserv.ts` around lines 143 - 145, Remove the redundant check and fixed wait: in the block that calls Operator.visitFlowcollector(), delete the duplicate assertion for 'div.loading-box__loaded' and the magic cy.wait(3000); rely on the existing readiness assertion for '#yaml-create' (already present after visitFlowcollector) so the flow uses that explicit readiness signal instead of a fixed sleep to improve stability.
219-226: 💤 Low valueReadiness pairing looks correct — one watch-out.
The
tr 'cluster'row +oc waitonapp=netobserv-pluginpair nicely. Note thatcy.contains('tr', 'cluster')matches any row whose text contains "cluster"; if the list ever shows other rows whose names embedclusterit could match the wrong row. Scoping to the name cell (e.g.cy.contains('td a', /^cluster$/).parents('tr')) would harden it.As per coding guidelines: "Verify E2E test stability, proper waits, and selector resilience".
🤖 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 `@web/cypress/views/netobserv.ts` around lines 219 - 226, The selector cy.contains('tr', 'cluster') is too loose and can match rows whose text merely contains "cluster"; change it to scope to the exact name cell by using a strict selector like cy.contains('td a', /^cluster$/).parents('tr') and then call .within(...) to assert the status via cy.byTestID('status-text', { timeout: 60000 }).should('contain.text', 'Ready'); leave the accompanying cy.adminCLI(`oc wait --for=condition=Ready pod -l app=netobserv-plugin -n ${project} --timeout=180s`) as-is to keep the oc wait pairing.
99-101: 💤 Low valueSubstring match can latch onto the wrong CSV.
stdout.split('\n').find(line => line.includes('netobserv-operator') || line.includes('network-observability-operator'))will also match any CSV whose name happens to embed those substrings (e.g. a leftover/dev CSV, a*-bundletest artifact). With multiple matches you silently grab the first one. Anchoring to a^name.vprefix — or filtering withoc get csv -l operators.coreos.com/netobserv-operator.openshift-netobserv-operator— would be more precise.Also applies to: 130-132
🤖 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 `@web/cypress/views/netobserv.ts` around lines 99 - 101, The current csvName extraction uses a loose substring match and can pick the wrong CSV; update the logic that sets csvName (and the similar block at the second occurrence around lines 130-132) to match the CSV name more precisely—either run oc get csv with the operator label (e.g., operators.coreos.com/netobserv-operator.openshift-netobserv-operator) and parse that single result, or filter stdout lines with a regex anchored to the CSV naming convention (e.g., /^netobserv-operator\.v\d+/ or /^network-observability-operator\.v\d+/) so you only pick the CSV whose name begins with the operator name plus a .v version prefix instead of using includes().
🤖 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 `@web/cypress/fixtures/test-server-client.yaml`:
- Line 5: The fixture uses hardcoded namespace names 'test-server-56222' and
'test-client-56222' which can cause collisions in parallel runs; change the
static values in the YAML to parameterized or dynamically-generated names (e.g.,
replace 'test-server-56222' and 'test-client-56222' with templating placeholders
like {{server_namespace}} and {{client_namespace}} or use your test harness to
generate unique names at runtime), update all other occurrences of those
literals in the same file (the other noted spots) to use the same placeholders,
and ensure the test setup/teardown code injects or computes unique values for
server_namespace and client_namespace for each test run.
In `@web/cypress/views/netobserv.ts`:
- Around line 103-112: The current check in the csv path (using csvName, stdout
and cy.adminCLI) throws immediately if the CSV exists but statusResult.stdout is
not "Succeeded", which flakes on transient phases; instead, change the logic in
the NetObserv CSV handling (where cy.adminCLI(... -o jsonpath='{.status.phase}')
returns statusResult) to treat non-terminal phases as "not installed yet" and
poll/retry until the CSV reaches "Succeeded" or a reasonable timeout, e.g.
replace the immediate throw in the statusResult.then callback with a
waiter/retry loop (using Cypress retries or a custom poll with cy.wait() and
repeated cy.adminCLI calls) and only fail if the phase becomes a terminal
failure or the timeout expires.
- Line 97: Three promise callbacks in web/cypress/views/netobserv.ts use an
untyped parameter `(result: any)`/`(statusResult: any)`; change these parameter
types to `Cypress.Exec` so callers can safely access `stdout`/`stderr`. Locate
the three `.then((result: any) => {` / `.then((statusResult: any) => {` handlers
(used with `adminCLI`/`cy.exec()` calls) and update their signatures to
`.then((result: Cypress.Exec) => {` or `.then((statusResult: Cypress.Exec) => {`
respectively, keeping the same variable names and body logic. Ensure all three
call sites (the ones at the current result, the later statusResult, and the one
around line 128) are updated consistently.
---
Nitpick comments:
In `@web/cypress/fixtures/test-server-client.yaml`:
- Line 78: The curl invocation "curl
nginx-service.test-server-56222.svc:80/data/100K" hardcodes the service FQDN;
change it to use a namespace variable so the fixture is reusable (e.g. set an
env var like TEST_SERVER_NAMESPACE via env or downward API and replace the
command with "curl nginx-service.${TEST_SERVER_NAMESPACE}.svc:80/data/100K" or
use "$(POD_NAMESPACE)" if available), and ensure the pod spec defines that env
var (TEST_SERVER_NAMESPACE or POD_NAMESPACE) so the command resolves at runtime.
In `@web/cypress/integration-tests/quickFilters.cy.ts`:
- Around line 31-39: Remove the fixed delay by deleting the cy.wait(10000) call
and rely on the deterministic readiness checks already present (the
cy.adminCLI('oc wait --for=condition=Ready pod -l app=nginx -n test-server-56222
--timeout=120s') and cy.adminCLI('oc wait --for=condition=Ready pod -n
test-client-56222 client --timeout=120s') calls); ensure those oc wait commands
remain immediately after creating the pods (the cy.adminCLI('oc apply -f
cypress/fixtures/test-server-client.yaml') block) so the test polls for
readiness instead of sleeping.
In `@web/cypress/views/netobserv.ts`:
- Around line 143-145: Remove the redundant check and fixed wait: in the block
that calls Operator.visitFlowcollector(), delete the duplicate assertion for
'div.loading-box__loaded' and the magic cy.wait(3000); rely on the existing
readiness assertion for '#yaml-create' (already present after
visitFlowcollector) so the flow uses that explicit readiness signal instead of a
fixed sleep to improve stability.
- Around line 219-226: The selector cy.contains('tr', 'cluster') is too loose
and can match rows whose text merely contains "cluster"; change it to scope to
the exact name cell by using a strict selector like cy.contains('td a',
/^cluster$/).parents('tr') and then call .within(...) to assert the status via
cy.byTestID('status-text', { timeout: 60000 }).should('contain.text', 'Ready');
leave the accompanying cy.adminCLI(`oc wait --for=condition=Ready pod -l
app=netobserv-plugin -n ${project} --timeout=180s`) as-is to keep the oc wait
pairing.
- Around line 99-101: The current csvName extraction uses a loose substring
match and can pick the wrong CSV; update the logic that sets csvName (and the
similar block at the second occurrence around lines 130-132) to match the CSV
name more precisely—either run oc get csv with the operator label (e.g.,
operators.coreos.com/netobserv-operator.openshift-netobserv-operator) and parse
that single result, or filter stdout lines with a regex anchored to the CSV
naming convention (e.g., /^netobserv-operator\.v\d+/ or
/^network-observability-operator\.v\d+/) so you only pick the CSV whose name
begins with the operator name plus a .v version prefix instead of using
includes().
🪄 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: 2afa091f-294d-41d9-9493-a82bc9a37ae5
📒 Files selected for processing (13)
web/cypress/fixtures/flowcollector/fc_TLSTracking.yamlweb/cypress/fixtures/test-server-client.yamlweb/cypress/integration-tests/dns_tracking.cy.tsweb/cypress/integration-tests/netflow_cluster_admin_group.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/quickFilters.cy.tsweb/cypress/integration-tests/table_queryopts.cy.tsweb/cypress/integration-tests/tls_dashboards.cy.tsweb/cypress/integration-tests/tls_tracking.cy.tsweb/cypress/views/netflow-page.tsweb/cypress/views/netobserv.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- web/cypress/integration-tests/netflow_cluster_admin_group.cy.ts
- web/cypress/fixtures/flowcollector/fc_TLSTracking.yaml
- web/cypress/integration-tests/tls_dashboards.cy.ts
- web/cypress/integration-tests/dns_tracking.cy.ts
- web/cypress/integration-tests/netflow_external_subnet.cy.ts
- web/cypress/integration-tests/tls_tracking.cy.ts
- web/cypress/views/netflow-page.ts
Description
Dependencies
Dependent on PR #1530
Checklist
Initial jenkins run: noo-frontend-tests#76 Fixed status_test after this. TLS test verifying panels is the only test failing since the changes havent been merged to PF6 branch yet
Got a clean run noo-frontend-tests#78 with above TLS PR image patched
Summary by CodeRabbit