[NETOBSERV-2376] FlowCollector Status Page UI Test#1439
[NETOBSERV-2376] FlowCollector Status Page UI Test#1439kapjain-rh wants to merge 4 commits intonetobserv:mainfrom
Conversation
|
[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 Cypress integration tests and view helpers for FlowCollector status page, covering ready and error states. Introduces support for a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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: 2
🧹 Nitpick comments (7)
web/cypress/views/flowcollector-status.ts (2)
8-9: PatternFly version pinned in selectors.
pf-v5-c-alerthardcodes PatternFly v5. When the app upgrades to v6, every test relying on these selectors will silently break. Consider a version-agnostic helper or stabledata-test/roleselectors if the app exposes them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/flowcollector-status.ts` around lines 8 - 9, Selectors configWarningAlert and configErrorAlert use the hardcoded class "pf-v5-c-alert" which pins PatternFly version; replace these with version-agnostic selectors (e.g., remove the "pf-v5-" prefix or match on generic classes like "pf-c-alert" or use stable attributes) or, preferably, switch to stable test-specific attributes or roles (data-test or role="alert") exposed by the app so tests won't break on PF upgrades; update references to configWarningAlert and configErrorAlert accordingly wherever they are imported.
14-14: Missing leading slash incy.visitpath.
cy.visit('k8s/cluster/...')resolves relative to the current URL when nobaseUrlis set, and even with a baseUrl, a leading slash is idiomatic and matchesflowCollectorStatusPathdefined inweb/src/utils/url.ts(/k8s/cluster/...). Consider importing that constant to keep the path single-sourced.- cy.visit('k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status') + cy.visit('/k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/flowcollector-status.ts` at line 14, The path passed to cy.visit is missing a leading slash and should use the canonical constant to avoid divergence; update the call to cy.visit to use '/k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status' (i.e., add the leading '/') and preferably import and use the flowCollectorStatusPath constant from web/src/utils/url.ts instead of the hardcoded string so the test uses the single-sourced URL; change the cy.visit invocation in flowcollector-status.ts to reference flowCollectorStatusPath.web/cypress/views/search.ts (2)
10-13: Quote attribute values in selector.
[data-filter-text=${machineResource}]uses unquoted attribute values. It happens to work for these alphanumeric strings, but it's fragile—any future value with special characters will silently break. Quote for resilience.- cy.get(`[data-filter-text=${machineResource}]`).should('not.exist'); + cy.get(`[data-filter-text="${machineResource}"]`).should('not.exist');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/search.ts` around lines 10 - 13, The selector uses unquoted attribute values in cy.get(`[data-filter-text=${machineResource}]`), which is fragile; update the loop that iterates over machineResources (the machineResources array and the forEach callback) to quote the attribute value in the selector (e.g., use `[data-filter-text="${machineResource}"]`) so values with special characters won't break the selector.
3-6: Escape Cypress special characters in typed input.
cy.type()interprets{,}, and other sequences as special commands. Ifresource_typeever contains such characters, typing will misbehave. Also the template literal on line 4 is redundant.♻️ Suggested tweak
- chooseResourceType: (resource_type) => { - cy.get('input[placeholder="Resources"]').clear().type(`${resource_type}`); + chooseResourceType: (resource_type: string) => { + cy.get('input[placeholder="Resources"]').clear().type(resource_type, { parseSpecialCharSequences: false }); cy.get(`label[id$="~${resource_type}"]`).click(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/search.ts` around lines 3 - 6, The chooseResourceType helper types resource_type into the input using cy.type which treats { and } as special sequences; change the call to cy.get('input[placeholder="Resources"]').clear().type(resource_type, { parseSpecialCharSequences: false }) to disable special-sequence parsing and pass resource_type directly (remove the redundant template literal), and keep the existing label click using the current cy.get(`label[id$="~${resource_type}"]`).click() selector.web/cypress/integration-tests/flowcollector_status.cy.ts (3)
21-63: Single test asserts many unrelated concerns.This
itblock verifies title, tooltip, table headers, component rows, button state, warning alert, and four condition rows. On failure you get one signal for many distinct features, making triage harder. Consider splitting into focused tests (page header/tooltip, component table, conditions, warning alert) — they can share thevisit()viabeforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 21 - 63, The test currently bundles many unrelated assertions in one it block; split this single test into multiple focused tests that each verify one concern: 1) header and tooltip — use flowcollectorStatusPage.visit() in a beforeEach and assert the title, the 'button[aria-label="FlowCollector status"]' tooltip; 2) component table headers and rows — assert 'Component statuses', th headers and presence of eBPF Agent/Flowlogs Pipeline/Console Plugin/Loki/Monitoring; 3) open network traffic button state — assert pluginSelectors.openNetworkTraffic is present and not aria-disabled; 4) configuration warning alert — assert flowcollectorStatusSelectors.configIssueRow attrs and flowcollectorStatusSelectors.configWarningAlert title; 5) conditions rows — assert flowcollectorStatusSelectors.readyRow, agentReadyRow, pluginReadyRow, monitoringReadyRow; keep flowcollectorStatusPage.visit() shared in beforeEach and move related cy.get/cy.contains calls into their respective it blocks referencing the same selector symbols.
65-89: Two tests are near-duplicates; extract a helper.The Network Traffic and Network Health status-indicator tests differ only in the initial URL and the container selector. Extract a helper to reduce maintenance cost and keep assertions in lockstep.
♻️ Example refactor
const verifyStatusIndicator = (url: string, containerSel: string, timeout = 30000) => { cy.visit(url) cy.get(containerSel, { timeout: 60000 }).should('exist') cy.get(flowcollectorStatusSelectors.statusIndicator).should('exist') cy.get(flowcollectorStatusSelectors.statusIndicator + ' span').first().trigger('mouseenter', { force: true }) cy.get('.pf-v5-c-tooltip__content', { timeout: 10000 }).should('contain.text', 'FlowCollector is ready') cy.get(flowcollectorStatusSelectors.statusIndicator).click() cy.contains('Network Observability FlowCollector status', { timeout }).should('exist') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 65 - 89, Extract the duplicated test logic into a helper function (e.g., verifyStatusIndicator(url: string, containerSel: string, timeout = 30000)) that visits the given url, asserts the page container exists (use the longer timeout used in Traffic test where appropriate), checks flowcollectorStatusSelectors.statusIndicator exists, triggers mouseenter on flowcollectorStatusSelectors.statusIndicator + ' span', asserts the tooltip '.pf-v5-c-tooltip__content' contains 'FlowCollector is ready', clicks the status indicator, and finally asserts 'Network Observability FlowCollector status' exists with the provided timeout; then replace both test bodies to call verifyStatusIndicator('/netflow-traffic', '#overview-container', 60000) and verifyStatusIndicator('/network-health', '#content-scrollable', 30000) respectively so the assertions remain identical and timeouts preserved.
27-28: Flaky tooltip pattern: trigger mouseenter onspanthen assert tooltip.
trigger('mouseenter', { force: true })onspan(repeated on lines 71, 84) can race with Floating UI's positioning. Cypress's.trigger()does not retry on the command itself, only on the preceding query. If the tooltip library debounces, this may intermittently fail. Consider.realHover()(cypress-real-events) or hovering the button/indicator element directly, and add an explicit assertion that the tooltip appeared before asserting its text.Also note the tooltip text "FlowCollector is ready" is sourced from the operator — linked-repo context shows condition messages originate in
netobserv-operator, so the test is tightly coupled to that string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 27 - 28, The tooltip hover is flaky because the test triggers mouseenter on the inner span (cy.get('button[aria-label="FlowCollector status"] span') / repeated occurrences) which can race with Floating UI; change the hover to target the button/indicator element itself (the button with aria-label "FlowCollector status") and use a reliable hover method (e.g., cypress-real-events .realHover() if available) or a retryable Cypress command, then add an explicit wait/assertion that the tooltip element ('.pf-v5-c-tooltip__content') is present/visible before asserting its text; also consider loosening the exact text dependency since the string originates from netobserv-operator.
🤖 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/flowcollector_status.cy.ts`:
- Around line 108-111: The after hook can be skipped if before fails, leaking
the cluster-admin binding; update teardown to be resilient by moving role
bind/unbind to the outermost wrapper or implementing a cy.task with try/finally
in support code, and at minimum make the existing cleanup idempotent: wrap
Operator.deleteFlowCollector() in a safe guard (check existence or catch and log
errors) and make the cy.adminCLI remove-cluster-role-from-user call tolerant of
missing bindings (check bound users first or catch failures) so cleanup never
throws and therefore won't cascade-fail; reference Operator.install,
createFlowcollector, Operator.deleteFlowCollector and cy.adminCLI when making
these changes.
In `@web/cypress/views/search.ts`:
- Around line 18-23: In searchMethodValues replace the locale-dependent call by
using method = method.toLowerCase() instead of method.toLocaleLowerCase() so the
DOM selector construction (used in li[data-test="${method}-filter"]) is
locale-independent; update the assignment in the searchMethodValues function
accordingly.
---
Nitpick comments:
In `@web/cypress/integration-tests/flowcollector_status.cy.ts`:
- Around line 21-63: The test currently bundles many unrelated assertions in one
it block; split this single test into multiple focused tests that each verify
one concern: 1) header and tooltip — use flowcollectorStatusPage.visit() in a
beforeEach and assert the title, the 'button[aria-label="FlowCollector status"]'
tooltip; 2) component table headers and rows — assert 'Component statuses', th
headers and presence of eBPF Agent/Flowlogs Pipeline/Console
Plugin/Loki/Monitoring; 3) open network traffic button state — assert
pluginSelectors.openNetworkTraffic is present and not aria-disabled; 4)
configuration warning alert — assert flowcollectorStatusSelectors.configIssueRow
attrs and flowcollectorStatusSelectors.configWarningAlert title; 5) conditions
rows — assert flowcollectorStatusSelectors.readyRow, agentReadyRow,
pluginReadyRow, monitoringReadyRow; keep flowcollectorStatusPage.visit() shared
in beforeEach and move related cy.get/cy.contains calls into their respective it
blocks referencing the same selector symbols.
- Around line 65-89: Extract the duplicated test logic into a helper function
(e.g., verifyStatusIndicator(url: string, containerSel: string, timeout =
30000)) that visits the given url, asserts the page container exists (use the
longer timeout used in Traffic test where appropriate), checks
flowcollectorStatusSelectors.statusIndicator exists, triggers mouseenter on
flowcollectorStatusSelectors.statusIndicator + ' span', asserts the tooltip
'.pf-v5-c-tooltip__content' contains 'FlowCollector is ready', clicks the status
indicator, and finally asserts 'Network Observability FlowCollector status'
exists with the provided timeout; then replace both test bodies to call
verifyStatusIndicator('/netflow-traffic', '#overview-container', 60000) and
verifyStatusIndicator('/network-health', '#content-scrollable', 30000)
respectively so the assertions remain identical and timeouts preserved.
- Around line 27-28: The tooltip hover is flaky because the test triggers
mouseenter on the inner span (cy.get('button[aria-label="FlowCollector status"]
span') / repeated occurrences) which can race with Floating UI; change the hover
to target the button/indicator element itself (the button with aria-label
"FlowCollector status") and use a reliable hover method (e.g.,
cypress-real-events .realHover() if available) or a retryable Cypress command,
then add an explicit wait/assertion that the tooltip element
('.pf-v5-c-tooltip__content') is present/visible before asserting its text; also
consider loosening the exact text dependency since the string originates from
netobserv-operator.
In `@web/cypress/views/flowcollector-status.ts`:
- Around line 8-9: Selectors configWarningAlert and configErrorAlert use the
hardcoded class "pf-v5-c-alert" which pins PatternFly version; replace these
with version-agnostic selectors (e.g., remove the "pf-v5-" prefix or match on
generic classes like "pf-c-alert" or use stable attributes) or, preferably,
switch to stable test-specific attributes or roles (data-test or role="alert")
exposed by the app so tests won't break on PF upgrades; update references to
configWarningAlert and configErrorAlert accordingly wherever they are imported.
- Line 14: The path passed to cy.visit is missing a leading slash and should use
the canonical constant to avoid divergence; update the call to cy.visit to use
'/k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status' (i.e., add the
leading '/') and preferably import and use the flowCollectorStatusPath constant
from web/src/utils/url.ts instead of the hardcoded string so the test uses the
single-sourced URL; change the cy.visit invocation in flowcollector-status.ts to
reference flowCollectorStatusPath.
In `@web/cypress/views/search.ts`:
- Around line 10-13: The selector uses unquoted attribute values in
cy.get(`[data-filter-text=${machineResource}]`), which is fragile; update the
loop that iterates over machineResources (the machineResources array and the
forEach callback) to quote the attribute value in the selector (e.g., use
`[data-filter-text="${machineResource}"]`) so values with special characters
won't break the selector.
- Around line 3-6: The chooseResourceType helper types resource_type into the
input using cy.type which treats { and } as special sequences; change the call
to cy.get('input[placeholder="Resources"]').clear().type(resource_type, {
parseSpecialCharSequences: false }) to disable special-sequence parsing and pass
resource_type directly (remove the redundant template literal), and keep the
existing label click using the current
cy.get(`label[id$="~${resource_type}"]`).click() selector.
🪄 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: 2f0cab04-5bcd-4b1b-8fde-2d6ea4d6fda8
📒 Files selected for processing (3)
web/cypress/integration-tests/flowcollector_status.cy.tsweb/cypress/views/flowcollector-status.tsweb/cypress/views/search.ts
| after("after all tests are done", function () { | ||
| Operator.deleteFlowCollector() | ||
| cy.adminCLI(`oc adm policy remove-cluster-role-from-user cluster-admin ${Cypress.env('LOGIN_USERNAME')}`) | ||
| }) |
There was a problem hiding this comment.
after hook won't run if before fails partway.
If Operator.install() or createFlowcollector() throws, the cluster-admin role binding added at line 9 leaks to subsequent runs. Consider moving the role-add/remove to the outermost wrapper or using cy.task with try/finally semantics in a support file. At minimum, guard deletion so it doesn't cascade-fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 108 -
111, The after hook can be skipped if before fails, leaking the cluster-admin
binding; update teardown to be resilient by moving role bind/unbind to the
outermost wrapper or implementing a cy.task with try/finally in support code,
and at minimum make the existing cleanup idempotent: wrap
Operator.deleteFlowCollector() in a safe guard (check existence or catch and log
errors) and make the cy.adminCLI remove-cluster-role-from-user call tolerant of
missing bindings (check bound users first or catch failures) so cleanup never
throws and therefore won't cascade-fail; reference Operator.install,
createFlowcollector, Operator.deleteFlowCollector and cy.adminCLI when making
these changes.
| searchMethodValues: (method, value) => { | ||
| method = method.toLocaleLowerCase(); | ||
| cy.get('button[id="search-filter-toggle"]').click(); | ||
| cy.get(`li[data-test="${method}-filter"] button[role="option"]`).click(); | ||
| cy.get('input[id="search-filter-input"]').clear().type(`${value}`); | ||
| }, |
There was a problem hiding this comment.
Prefer toLowerCase() over toLocaleLowerCase() for selector construction.
toLocaleLowerCase() depends on the runtime locale (e.g., Turkish locale maps I → ı), which can produce a selector like li[data-test="ı-filter"] and silently fail. Since this string is used to construct a DOM selector, use locale-independent toLowerCase().
- method = method.toLocaleLowerCase();
+ method = method.toLowerCase();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| searchMethodValues: (method, value) => { | |
| method = method.toLocaleLowerCase(); | |
| cy.get('button[id="search-filter-toggle"]').click(); | |
| cy.get(`li[data-test="${method}-filter"] button[role="option"]`).click(); | |
| cy.get('input[id="search-filter-input"]').clear().type(`${value}`); | |
| }, | |
| searchMethodValues: (method, value) => { | |
| method = method.toLowerCase(); | |
| cy.get('button[id="search-filter-toggle"]').click(); | |
| cy.get(`li[data-test="${method}-filter"] button[role="option"]`).click(); | |
| cy.get('input[id="search-filter-input"]').clear().type(`${value}`); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/views/search.ts` around lines 18 - 23, In searchMethodValues
replace the locale-dependent call by using method = method.toLowerCase() instead
of method.toLocaleLowerCase() so the DOM selector construction (used in
li[data-test="${method}-filter"]) is locale-independent; update the assignment
in the searchMethodValues function accordingly.
|
|
||
| // Verify status icon tooltip shows error | ||
| cy.get('button[aria-label="FlowCollector status"] span').first().trigger('mouseenter', { force: true }) | ||
| cy.get('.pf-v5-c-tooltip__content', { timeout: 10000 }).should('contain.text', 'FlowCollector has errors') |
There was a problem hiding this comment.
Since you are in the status view here you should see Loki component with an error message + color un pipeline view.
It would be interesting to check the error message at least.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1439 +/- ##
=======================================
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:
|
|
Main will now point to pf-6 once PR netobserv-web-console#1365 is merged. So please test on 4.22 and make changes based on that. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/cypress/integration-tests/flowcollector_status.cy.ts (2)
126-127: ReuseflowcollectorStatusPage.visit()instead of hardcoding the URL.The helper already encapsulates this exact path plus a 30s
readyRowexistence wait. Inlining the URL here creates drift risk if the CRD version changes.♻️ Suggested change
- cy.visit('k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status') - cy.get(flowcollectorStatusSelectors.readyRow, { timeout: 120000 }).should('exist') + flowcollectorStatusPage.visit() + cy.get(flowcollectorStatusSelectors.readyRow, { timeout: 120000 }) .should('have.attr', 'data-test-status', 'False')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 126 - 127, Replace the hardcoded cy.visit URL and explicit readyRow wait with the existing helper: call flowcollectorStatusPage.visit() (which navigates to 'k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status' and already waits up to 30s for flowcollectorStatusSelectors.readyRow). Remove the inlined cy.visit(...) and the cy.get(flowcollectorStatusSelectors.readyRow, { timeout: 120000 }).should('exist') so the test relies on flowcollectorStatusPage.visit() to handle navigation and the readiness assertion.
26-29: Fragilespan spanselector for tooltip trigger.
.find('span span')(Lines 27, 68, 80, 159) depends on internal PatternFly DOM nesting and is exactly the kind of selector the PR reviewer (Amoghrd) asked to avoid, especially given the upcoming pf-6 migration in#1365/#1426 which will almost certainly reshuffle inner spans. Prefer hovering the labeled button itself (button[aria-label="FlowCollector status"]) or the#flowcollector-status-indicatorroot, or add a stabledata-testhook on the tooltip trigger.♻️ Suggested change
- cy.get('button[aria-label="FlowCollector status"]').should('exist') - .find('span span').trigger('mouseenter', { force: true }) + cy.get('button[aria-label="FlowCollector status"]').should('exist') + .trigger('mouseenter', { force: true })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/integration-tests/flowcollector_status.cy.ts` around lines 26 - 29, Replace the fragile nested selector usage in the Cypress test by triggering hover on the stable element instead of `.find('span span')`: use `cy.get('button[aria-label="FlowCollector status"]')` (or `#flowcollector-status-indicator` or a new `data-test` hook) and call `.trigger('mouseenter', { force: true })` on that element before asserting against `flowcollectorStatusSelectors.statusTooltip`; update all occurrences (the calls using `.find('span span')`) to use the stable selector and keep the existing timeout/assertion logic.
🤖 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/flowcollector_status.cy.ts`:
- Around line 113-122: The before hook uses
cy.deployFlowcollectorFromFixture(...) which applies a FlowCollector in-place
and can leave a previously Ready FC intact; update this setup to either call
Operator.deleteFlowCollector() first to remove any existing FlowCollector before
applying the fixture, or reuse Operator.createFlowcollector('LokiWithoutStack')
(after addressing the Ready assertion in netobserv.ts) so it deletes, creates
and waits for console refresh like the success path; ensure the chosen change
adds the same console-refresh wait used by Operator.createFlowcollector to avoid
observing stale Ready=True state.
In `@web/cypress/views/netobserv.ts`:
- Around line 166-168: The "LokiWithoutStack" switch branch in the flowcollector
deploy path (case "LokiWithoutStack") is incompatible with the post-deploy
assertions in Operator.createFlowcollector (which expects Ready status and a
running loki pod); either remove this branch from the generic
createFlowcollector routing or make the post-deploy assertions conditional on
parameters (e.g., skip Ready/loki checks when parameters === "LokiWithoutStack"
|| parameters === "LokiDisabled"). Locate the case "LokiWithoutStack" and either
delete it and leave its usage to direct cy.deployFlowcollectorFromFixture calls,
or add a guard around the status checks in Operator.createFlowcollector to
bypass the cy.byTestID('status-text').should('contain.text','Ready') and the
loki pod Running check when those parameter values are present.
---
Nitpick comments:
In `@web/cypress/integration-tests/flowcollector_status.cy.ts`:
- Around line 126-127: Replace the hardcoded cy.visit URL and explicit readyRow
wait with the existing helper: call flowcollectorStatusPage.visit() (which
navigates to 'k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status' and
already waits up to 30s for flowcollectorStatusSelectors.readyRow). Remove the
inlined cy.visit(...) and the cy.get(flowcollectorStatusSelectors.readyRow, {
timeout: 120000 }).should('exist') so the test relies on
flowcollectorStatusPage.visit() to handle navigation and the readiness
assertion.
- Around line 26-29: Replace the fragile nested selector usage in the Cypress
test by triggering hover on the stable element instead of `.find('span span')`:
use `cy.get('button[aria-label="FlowCollector status"]')` (or
`#flowcollector-status-indicator` or a new `data-test` hook) and call
`.trigger('mouseenter', { force: true })` on that element before asserting
against `flowcollectorStatusSelectors.statusTooltip`; update all occurrences
(the calls using `.find('span span')`) to use the stable selector and keep the
existing timeout/assertion logic.
🪄 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: 8ae841f4-d97b-4044-936f-0aa11cf302d8
📒 Files selected for processing (5)
web/cypress/fixtures/flowcollector/fc_lokiWithoutStack.yamlweb/cypress/integration-tests/flowcollector_status.cy.tsweb/cypress/views/flowcollector-status.tsweb/cypress/views/netobserv.tsweb/cypress/views/search.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/fixtures/flowcollector/fc_lokiWithoutStack.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- web/cypress/views/search.ts
- web/cypress/views/flowcollector-status.ts
| before('setup', function () { | ||
| cy.adminCLI(`oc adm policy add-cluster-role-to-user cluster-admin ${Cypress.env('LOGIN_USERNAME')}`) | ||
| cy.uiLogin(Cypress.env('LOGIN_IDP'), Cypress.env('LOGIN_USERNAME'), Cypress.env('LOGIN_PASSWORD')) | ||
|
|
||
| Operator.install() | ||
| cy.checkStorageClass(this) | ||
|
|
||
| // Deploy FlowCollector with Loki enabled pointing to a non-existent LokiStack | ||
| cy.deployFlowcollectorFromFixture('./cypress/fixtures/flowcollector/fc_lokiWithoutStack.yaml') | ||
| }) |
There was a problem hiding this comment.
Error-scenario before hook skips the "FlowCollector already exists" guard.
Unlike the success scenario which uses Operator.createFlowcollector() (which deletes any pre-existing FC and waits for console refresh), this block oc applys directly. If the previous describe's after failed for any reason (see the outstanding concern about after hooks not running when before throws), the old Ready FlowCollector will be patched in-place rather than replaced, and the 120s wait at Line 127 may observe stale Ready=True status before flipping. Suggest calling Operator.deleteFlowCollector() first, or reusing Operator.createFlowcollector('LokiWithoutStack') (after fixing the Ready assertion flagged in netobserv.ts), plus a console-refresh wait similar to createFlowcollector.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 113 -
122, The before hook uses cy.deployFlowcollectorFromFixture(...) which applies a
FlowCollector in-place and can leave a previously Ready FC intact; update this
setup to either call Operator.deleteFlowCollector() first to remove any existing
FlowCollector before applying the fixture, or reuse
Operator.createFlowcollector('LokiWithoutStack') (after addressing the Ready
assertion in netobserv.ts) so it deletes, creates and waits for console refresh
like the success path; ensure the chosen change adds the same console-refresh
wait used by Operator.createFlowcollector to avoid observing stale Ready=True
state.
|
Please Review |
| | 'DNSTracking' | ||
| | 'UDNMapping' | ||
| | 'LokiDisabled' | ||
| | 'LokiWithoutStack' |
There was a problem hiding this comment.
The naming feels off to me.
Something like WithoutLokiStack would be better?🤔
| cy.checkStorageClass(this) | ||
|
|
||
| // Deploy FlowCollector with Loki enabled pointing to a non-existent LokiStack | ||
| cy.deployFlowcollectorFromFixture('./cypress/fixtures/flowcollector/fc_lokiWithoutStack.yaml') |
There was a problem hiding this comment.
Here you can use DeployFlowcollector func instead
There was a problem hiding this comment.
are you talking about deployFlowcollectorFromUI?
if yes do you think it is imp here?
There was a problem hiding this comment.
The func you have used to deploy flowcollector in beforeEach block, can use the same here.
You have added switch-case condition for this scenario and are not using that
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/cypress/integration-tests/flowcollector_status.cy.ts (1)
112-121:⚠️ Potential issue | 🟡 MinorReset the FlowCollector before applying the error fixture.
deployFlowcollectorFromFixture()applies in-place. If the previous cleanup fails, this scenario can start from a stale Ready FlowCollector and observe transient/stale status. Delete the existing FlowCollector first or use the same create helper pattern as the ready scenario.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/flowcollector_status.cy.ts` around lines 112 - 121, The test's setup uses deployFlowcollectorFromFixture() which applies in-place and can leave a stale Ready FlowCollector if prior cleanup failed; update the before('setup') block to explicitly remove any existing FlowCollector before applying the error fixture (or mirror the ready scenario by using the create helper used there instead of deployFlowcollectorFromFixture), e.g., call the FlowCollector delete helper or kubectl/oc delete for the FlowCollector resource prior to invoking deployFlowcollectorFromFixture(), and ensure any necessary wait/check (e.g., wait for deletion completion or absence) is performed so the test starts from a clean state.
🧹 Nitpick comments (1)
web/cypress/views/flowcollector-status.ts (1)
3-3: Prefer the existing stable ID over localized ARIA text.
statusIndicatoralready targets the same button. KeepingstatusButtontied to"FlowCollector status"can break on copy or locale changes.Proposed fix
export namespace flowcollectorStatusSelectors { export const statusIndicator = '#flowcollector-status-indicator' - export const statusButton = 'button[aria-label="FlowCollector status"]' + export const statusButton = statusIndicatorAs 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/flowcollector-status.ts` at line 3, The selector statusButton uses a localized aria-label which is brittle; replace it to use the existing stable selector used by statusIndicator instead (make statusButton reference the same constant/selector as statusIndicator or use the stable data-test/id used there) so both selectors target the same resilient element and avoid locale/copy breakage; update any tests referencing statusButton to use the new stable selector.
🤖 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/flowcollector_status.cy.ts`:
- Around line 25-28: The test currently hovers using fragile PatternFly
internals via .find('span span'); instead, update the test to hover a stable
element such as flowcollectorStatusSelectors.statusButton (or a new
data-test/data-testid attribute added to the actual tooltip trigger) and
remove/find usages of '.find("span span")' — invoke .trigger('mouseenter', {
force: true }) on the stable selector and keep asserting
flowcollectorStatusSelectors.statusTooltip contains 'FlowCollector is ready';
apply the same change to the other occurrences mentioned (lines 66-69, 78-81,
155-158).
- Around line 143-147: Replace the brittle exact-message assertion on
flowcollectorStatusSelectors.lokiStackRow with a stable check: keep the
existence and data-test-status='True' assertions, remove the full
environment-specific text match, and instead assert a stable operator-provided
reason (e.g., the data-test-reason attribute or a shorter contains like
'Loki'/'LokiStack') and ensure the test waits for the row to appear (reuse the
existing cy.get(...).should('exist') pattern) so the assertion is resilient
across clusters where the CRD exists but the stack is missing.
---
Duplicate comments:
In `@web/cypress/integration-tests/flowcollector_status.cy.ts`:
- Around line 112-121: The test's setup uses deployFlowcollectorFromFixture()
which applies in-place and can leave a stale Ready FlowCollector if prior
cleanup failed; update the before('setup') block to explicitly remove any
existing FlowCollector before applying the error fixture (or mirror the ready
scenario by using the create helper used there instead of
deployFlowcollectorFromFixture), e.g., call the FlowCollector delete helper or
kubectl/oc delete for the FlowCollector resource prior to invoking
deployFlowcollectorFromFixture(), and ensure any necessary wait/check (e.g.,
wait for deletion completion or absence) is performed so the test starts from a
clean state.
---
Nitpick comments:
In `@web/cypress/views/flowcollector-status.ts`:
- Line 3: The selector statusButton uses a localized aria-label which is
brittle; replace it to use the existing stable selector used by statusIndicator
instead (make statusButton reference the same constant/selector as
statusIndicator or use the stable data-test/id used there) so both selectors
target the same resilient element and avoid locale/copy breakage; update any
tests referencing statusButton to use the new stable selector.
🪄 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: 20ad9e38-3412-47c1-b815-4d24fc37c36c
📒 Files selected for processing (3)
web/cypress/fixtures/flowcollector/fc_lokiWithoutStack.yamlweb/cypress/integration-tests/flowcollector_status.cy.tsweb/cypress/views/flowcollector-status.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/fixtures/flowcollector/fc_lokiWithoutStack.yaml
| cy.get(flowcollectorStatusSelectors.statusButton).should('exist') | ||
| .find('span span').trigger('mouseenter', { force: true }) | ||
| cy.get(flowcollectorStatusSelectors.statusTooltip, { timeout: 10000 }) | ||
| .should('contain.text', 'FlowCollector is ready') |
There was a problem hiding this comment.
Avoid hovering through PatternFly internals.
.find('span span') depends on the current PF DOM shape and is likely to break during PF6 migration. Add/use a stable data-test or ID on the tooltip trigger, or trigger hover on the stable button if that is the actual tooltip target.
As per coding guidelines, web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience.
Also applies to: 66-69, 78-81, 155-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 25 -
28, The test currently hovers using fragile PatternFly internals via .find('span
span'); instead, update the test to hover a stable element such as
flowcollectorStatusSelectors.statusButton (or a new data-test/data-testid
attribute added to the actual tooltip trigger) and remove/find usages of
'.find("span span")' — invoke .trigger('mouseenter', { force: true }) on the
stable selector and keep asserting flowcollectorStatusSelectors.statusTooltip
contains 'FlowCollector is ready'; apply the same change to the other
occurrences mentioned (lines 66-69, 78-81, 155-158).
| // Verify WaitingLokiStack condition shows LokiStack not found error | ||
| cy.get(flowcollectorStatusSelectors.lokiStackRow).should('exist') | ||
| .should('have.attr', 'data-test-status', 'True') | ||
| cy.get(flowcollectorStatusSelectors.lokiStackRow) | ||
| .should('contain.text', 'Loki is configured in LokiStack mode, but LokiStack API is missing') |
There was a problem hiding this comment.
Do not assert the environment-specific LokiStack API message.
This exact text only holds when the LokiStack API is absent. On clusters where the CRD exists but the loki stack is missing, the condition can still be valid while the message differs. Prefer the stable reason from the operator and only loosely assert the row context.
Proposed adjustment
// Verify WaitingLokiStack condition shows LokiStack not found error
cy.get(flowcollectorStatusSelectors.lokiStackRow).should('exist')
.should('have.attr', 'data-test-status', 'True')
+ .should('have.attr', 'data-test-reason', 'CantFetchLokiStack')
cy.get(flowcollectorStatusSelectors.lokiStackRow)
- .should('contain.text', 'Loki is configured in LokiStack mode, but LokiStack API is missing')
+ .should('contain.text', 'LokiStack')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/flowcollector_status.cy.ts` around lines 143 -
147, Replace the brittle exact-message assertion on
flowcollectorStatusSelectors.lokiStackRow with a stable check: keep the
existence and data-test-status='True' assertions, remove the full
environment-specific text match, and instead assert a stable operator-provided
reason (e.g., the data-test-reason attribute or a shorter contains like
'Loki'/'LokiStack') and ensure the test waits for the row to appear (reuse the
existing cy.get(...).should('exist') pattern) so the assertion is resilient
across clusters where the CRD exists but the stack is missing.
| cy.clearLocalStorage() | ||
| }) | ||
|
|
||
| it("(OCP-88744, kapjain, Network_Observability) Verify FlowCollector status page components and conditions", { tags: ['@netobserv-critical'] }, function () { |
There was a problem hiding this comment.
I feel this whole it block can be moved to different tests.
For example you can move the Verify status indicator on Network Traffic page to the netflow_traffic page test
The Verify FlowCollector status page components and conditions can be moved to netobserv.ts to have that check for every test/can be moved to staticPlugin test since we go to statusPage if I remember correctly.
I am suggesting to move this whole describe block so that we have flowcollector deployment and deletion only once in each test file.
There was a problem hiding this comment.
I think they are all related to is is on to be in the same file.
And it make any issue we if we do twice as the test are filtered based on file or tag.
IMo if test are related and testing one feature it is ok to be in one file.
| @@ -0,0 +1,20 @@ | |||
| export namespace flowcollectorStatusSelectors { | |||
There was a problem hiding this comment.
these can be moved to netfow-page.ts instead of a new file
There was a problem hiding this comment.
I think if Page is diff it should have a diff view
| cy.get(`[data-filter-text=${machineResource}]`).should('not.exist'); | ||
| }); | ||
| }, | ||
| clearAllFilters: () => { |
There was a problem hiding this comment.
I just copied the search view from openshfit-test-private repo and this function is for search view.
We have to think about how we will manage views that is not related to netobserv
|
Could you also first rebase your branch with main to get the latest changes? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/cypress/views/flowcollector-status.ts (1)
5-12: Optional: prefer#X-rowover[id=X-row].Idiomatic id selector is shorter and marginally faster. Attribute form also breaks if an id ever contains special chars (not the case today).
♻️ Proposed tweak
- export const readyRow = '[id=Ready-row]' - export const agentReadyRow = '[id=WaitingEBPFAgents-row]' - export const pluginReadyRow = '[id=WaitingWebConsole-row]' - export const monitoringReadyRow = '[id=WaitingMonitoring-row]' - export const configIssueRow = '[id=ConfigurationIssue-row]' - export const flpMonolithRow = '[id=WaitingFLPMonolith-row]' - export const lokiStackRow = '[id=WaitingLokiStack-row]' - export const flpParentRow = '[id=WaitingFLPParent-row]' + export const readyRow = '#Ready-row' + export const agentReadyRow = '#WaitingEBPFAgents-row' + export const pluginReadyRow = '#WaitingWebConsole-row' + export const monitoringReadyRow = '#WaitingMonitoring-row' + export const configIssueRow = '#ConfigurationIssue-row' + export const flpMonolithRow = '#WaitingFLPMonolith-row' + export const lokiStackRow = '#WaitingLokiStack-row' + export const flpParentRow = '#WaitingFLPParent-row'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/flowcollector-status.ts` around lines 5 - 12, The id selectors use attribute syntax (e.g., readyRow = '[id=Ready-row]') which is verbose and fragile; update each exported constant (readyRow, agentReadyRow, pluginReadyRow, monitoringReadyRow, configIssueRow, flpMonolithRow, lokiStackRow, flpParentRow) to use the idiomatic ID selector syntax (e.g., '#Ready-row') so selectors are shorter and more robust against special chars.web/cypress/integration-tests/flowcollector_status.cy.ts (1)
123-131: ReuseflowcollectorStatusPage.visit()for consistency.The success spec navigates via the helper (line 21); the error spec inlines the same URL. Reusing the helper keeps the navigation contract in one place and already waits for
Ready-rowto exist, after which yourshould('have.attr', 'data-test-status', 'False')with a longer timeout still has Cypress retry until the status flips.♻️ Proposed tweak
- // Visit status page and wait for Ready condition to show False (error state) - cy.visit('k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status') - cy.get(flowcollectorStatusSelectors.readyRow, { timeout: 120000 }).should('exist') - .should('have.attr', 'data-test-status', 'False') + // Visit status page and wait for Ready condition to show False (error state) + flowcollectorStatusPage.visit() + cy.get(flowcollectorStatusSelectors.readyRow, { timeout: 120000 }) + .should('have.attr', 'data-test-status', 'False')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 123 - 131, Replace the inline navigation with the shared helper: call flowcollectorStatusPage.visit() instead of cy.visit('k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status'); rely on the helper’s built-in wait for flowcollectorStatusSelectors.readyRow to exist and then assert that flowcollectorStatusSelectors.readyRow has data-test-status 'False' and that data-test-reason is neither 'Pending' nor 'Valid' (remove the redundant explicit existence wait/timeout since the helper covers it).
🤖 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/flowcollector_status.cy.ts`:
- Around line 139-140: The test in flowcollector_status.cy.ts uses a global
contains lookup for the cell text "Flowlogs Pipeline" which can match elsewhere;
scope the selector to the component statuses table or add a stable test id on
the row in resource-status.tsx so the spec targets the correct row. Concretely,
either update the E2E test to find the table element for component statuses
first and then search for the td containing "Flowlogs Pipeline" within that
table, or add a deterministic data-test/data-id on the row rendered by
resource-status.tsx (e.g. per-component row id keyed by component name) and
change the assertion in flowcollector_status.cy.ts to select that data-test/id
before asserting the presence of "loki-gateway-ca-bundle".
---
Nitpick comments:
In `@web/cypress/integration-tests/flowcollector_status.cy.ts`:
- Around line 123-131: Replace the inline navigation with the shared helper:
call flowcollectorStatusPage.visit() instead of
cy.visit('k8s/cluster/flows.netobserv.io~v1beta2~FlowCollector/status'); rely on
the helper’s built-in wait for flowcollectorStatusSelectors.readyRow to exist
and then assert that flowcollectorStatusSelectors.readyRow has data-test-status
'False' and that data-test-reason is neither 'Pending' nor 'Valid' (remove the
redundant explicit existence wait/timeout since the helper covers it).
In `@web/cypress/views/flowcollector-status.ts`:
- Around line 5-12: The id selectors use attribute syntax (e.g., readyRow =
'[id=Ready-row]') which is verbose and fragile; update each exported constant
(readyRow, agentReadyRow, pluginReadyRow, monitoringReadyRow, configIssueRow,
flpMonolithRow, lokiStackRow, flpParentRow) to use the idiomatic ID selector
syntax (e.g., '#Ready-row') so selectors are shorter and more robust against
special chars.
🪄 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: 5f1a9edb-6dea-4de9-a81f-98c4201824c3
📒 Files selected for processing (5)
web/cypress/fixtures/flowcollector/fc_lokiWithoutLokiStack.yamlweb/cypress/integration-tests/flowcollector_status.cy.tsweb/cypress/views/flowcollector-status.tsweb/cypress/views/netobserv.tsweb/cypress/views/search.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/fixtures/flowcollector/fc_lokiWithoutLokiStack.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- web/cypress/views/search.ts
| cy.contains('td', 'Flowlogs Pipeline').parent('tr') | ||
| .should('contain.text', 'loki-gateway-ca-bundle') |
There was a problem hiding this comment.
Fragile row lookup — anchor to the component table.
cy.contains('td', 'Flowlogs Pipeline').parent('tr') matches the first <td> containing that text anywhere on the page. If the string also appears in a Details/message column elsewhere (common in error states), the test silently asserts against the wrong row. Anchor the lookup to the component statuses table, or add a stable data-test/id on the row in resource-status.tsx.
♻️ Suggested tightening
- cy.contains('td', 'Flowlogs Pipeline').parent('tr')
- .should('contain.text', 'loki-gateway-ca-bundle')
+ cy.contains('Component statuses')
+ .parents('section, [role="region"], .pf-v6-c-card, .pf-c-card').first()
+ .contains('td', /^Flowlogs Pipeline$/).parent('tr')
+ .should('contain.text', 'loki-gateway-ca-bundle')As per coding guidelines, web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.contains('td', 'Flowlogs Pipeline').parent('tr') | |
| .should('contain.text', 'loki-gateway-ca-bundle') | |
| cy.contains('Component statuses') | |
| .parents('section, [role="region"], .pf-v6-c-card, .pf-c-card').first() | |
| .contains('td', /^Flowlogs Pipeline$/).parent('tr') | |
| .should('contain.text', 'loki-gateway-ca-bundle') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/integration-tests/flowcollector_status.cy.ts` around lines 139 -
140, The test in flowcollector_status.cy.ts uses a global contains lookup for
the cell text "Flowlogs Pipeline" which can match elsewhere; scope the selector
to the component statuses table or add a stable test id on the row in
resource-status.tsx so the spec targets the correct row. Concretely, either
update the E2E test to find the table element for component statuses first and
then search for the td containing "Flowlogs Pipeline" within that table, or add
a deterministic data-test/data-id on the row rendered by resource-status.tsx
(e.g. per-component row id keyed by component name) and change the assertion in
flowcollector_status.cy.ts to select that data-test/id before asserting the
presence of "loki-gateway-ca-bundle".
3 participants
Description
npx cypress open --env grep="88744"
(Run Finished)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ flowcollector_status.cy.ts 01:17 5 5 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! 01:17 5 5 - - -
Dependencies
n/a
Checklist
Summary by CodeRabbit