Skip to content

OBSINTA-1290: fix incident detection test selectors#1022

Open
DavidRajnoha wants to merge 4 commits into
openshift:mainfrom
DavidRajnoha:fix/incidents-tab-selector-case-insensitive
Open

OBSINTA-1290: fix incident detection test selectors#1022
DavidRajnoha wants to merge 4 commits into
openshift:mainfrom
DavidRajnoha:fix/incidents-tab-selector-case-insensitive

Conversation

@DavidRajnoha

@DavidRajnoha DavidRajnoha commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make the Incidents tab selector case-insensitive ([data-test-id="horizontal-link-incidents" i]) — the Console SDK has shipped both "incidents" and "Incidents" across versions
  • Replace cy.byTestID lookups for detail table cells (alert rule, namespace, severity, state, start, end) with data-label attribute selectors — the data-test IDs weren't being rendered on
    these cells, while data-label is a built-in PatternFly structural attribute
  • Fix getIncidentIds to extract IDs from data-test attributes instead of button text content, which was picking up the description suffix (e.g. "1 component") and corrupting the ID used
    for filter selection
  • Use container existence check instead of visibility assertion for the filter dropdown (renders with 0 height)
  • Add scrollIntoView() and .first() across tests to fix viewport clipping and multi-element ambiguity failures

Summary by CodeRabbit

Summary

  • Tests
    • Improved reliability of Incidents page navigation and expanded-details verification, including more precise table targeting.
    • Updated incident ID extraction to use stable option attributes.
    • Adjusted alert search behavior in tests so filters clear only when the alert isn’t found.
    • Enhanced regression assertions by scrolling relevant UI elements into view and tightening chip/element targeting.
    • Reordered Incidents page test setup steps for more consistent filtering outcomes.

@openshift-ci
openshift-ci Bot requested review from etmurasaki and jgbernalp July 1, 2026 09:18
@openshift-ci

openshift-ci Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DavidRajnoha
Once this PR has been reviewed and has the lgtm label, please assign jan--f for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: f46ff0ff-d714-4347-abab-7355a73d01ad

📥 Commits

Reviewing files that changed from the base of the PR and between c965eaa and 94a4b27.

📒 Files selected for processing (1)
  • web/cypress/views/incidents-page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/cypress/views/incidents-page.ts

Walkthrough

Cypress incidents coverage now uses case-insensitive tab detection, row-based detail selectors, data-attribute incident ID extraction, reordered setup, conditional cleanup, and viewport-aware table and chip assertions.

Changes

Incidents Cypress coverage

Layer / File(s) Summary
Incident page selectors and filtering behavior
web/cypress/views/incidents-page.ts
The Incidents tab selector is case-insensitive; detail cells use row and data-label selectors; incident IDs come from option data-test attributes; filter cleanup is conditional on search results.
Walkthrough filter setup
web/cypress/e2e/incidents/01.incidents.cy.ts
The days filter is set before fixture loading, and filters are cleared afterward.
Filtering regression assertions
web/cypress/e2e/incidents/regression/*.cy.ts
Table and chip assertions scroll targets into view and select the first Incident ID chip where needed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: tremes, jgbernalp, etmurasaki, sthундат

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: updating incident-related test selectors and interactions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo test titles were added or changed; the modified Cypress titles are static literals, and dynamic incident IDs stay in test bodies.
Test Structure And Quality ✅ Passed Changed files are Cypress specs and helpers, not Ginkgo tests, so the Ginkgo-specific quality check is not applicable.
Microshift Test Compatibility ✅ Passed Only Cypress UI specs changed; no new Ginkgo It/Describe/Context tests or MicroShift-unsupported OpenShift API usage were added.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS: The PR only touches Cypress browser tests/selectors; no new Ginkgo e2e specs or multi-node/topology assumptions were added.
Topology-Aware Scheduling Compatibility ✅ Passed Only Cypress test/helper files changed; no deployment manifests, operator code, or controllers, and no scheduling/topology logic is present.
Ote Binary Stdout Contract ✅ Passed The PR only changes Cypress/view helpers; no process-level stdout writes (main/TestMain/BeforeSuite/top-level init) were added in touched files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo tests were added; the touched Cypress files contain no IPv4 literals, IP parsing, or public-host connectivity.
No-Weak-Crypto ✅ Passed Only Cypress selector/test-flow changes; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were found.
Container-Privileges ✅ Passed Only a Cypress TypeScript page-object file changed; no container/K8s manifests or privilege settings were introduced.
No-Sensitive-Data-In-Logs ✅ Passed Found only Cypress status/debug logs; none print secrets, PII, internal hosts, or customer data.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

DavidRajnoha and others added 3 commits July 15, 2026 12:41
The console changed the data-test-id from "horizontal-link-incidents"
to "horizontal-link-Incidents". Use the CSS `i` flag so the selector
matches both casings.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The detail table component never applied the data-test IDs defined in
data-test.ts. Switch to data-label attribute selectors matching the
actual dataLabel props rendered by IncidentsDetailsRowTable, consistent
with the regression test helpers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract incident IDs from data-test attributes instead of text content
to avoid description text pollution. Use container existence check instead
of visibility assertion for 0-height dropdown. Add scrollIntoView and
.first() to fix viewport and multi-element ambiguity issues.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@DavidRajnoha
DavidRajnoha force-pushed the fix/incidents-tab-selector-case-insensitive branch from fd2f1fd to 0a520a2 Compare July 15, 2026 10:41

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
web/cypress/views/incidents-page.ts (2)

282-287: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Fixed cy.wait(250) calls around setDays interaction.

Two arbitrary sleeps bracket the toggle click. The .should('be.visible') assertion already retries until the dropdown renders, so the trailing cy.wait(250) is redundant, and the leading one doesn't guarantee the toggle is ready — Cypress's built-in actionability retries already handle that. Fixed waits add flakiness risk without a real synchronization guarantee, and slow down every test calling setDays.

♻️ Proposed fix
   setDays: (value: '1 day' | '3 days' | '7 days' | '15 days') => {
     cy.log('incidentsPage.setDays');
-    cy.wait(250);
     incidentsPage.elements.daysSelectToggle().scrollIntoView().click({ force: true });
     incidentsPage.elements.daysSelectList().should('be.visible');
-    cy.wait(250);
     const dayKey = value.replace(' ', '-');
🤖 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/incidents-page.ts` around lines 282 - 287, Remove both
arbitrary cy.wait(250) calls from the setDays interaction. Keep the existing
scrollIntoView().click({ force: true }) action and
daysSelectList().should('be.visible') assertion so Cypress handles actionability
and dropdown readiness through its built-in retries.

186-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent side-effect: scrollIntoView() only on the alert-rule cell getter.

incidentsDetailsAlertRuleCell scrolls into view as part of the selector, but the sibling incidentsDetailsAlertRuleLink and the other cell getters (Namespace/Severity/State/Start/End) don't. Baking an action (scroll) into a query getter is inconsistent with the rest of the elements object and could produce different assertion behavior depending on which sibling selector a caller picks.

Consider moving the scroll to the call site (e.g., in the test, before .should('be.visible')) or applying it uniformly across all detail cell getters.

🤖 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/incidents-page.ts` around lines 186 - 229, Remove the
embedded scrollIntoView side effect from incidentsDetailsAlertRuleCell so all
incidents detail getters, including incidentsDetailsAlertRuleLink and the
Namespace/Severity/State/Start/End cell getters, remain selector-only. Move
scrolling to the relevant test call sites before visibility assertions,
preserving the existing visibility behavior without making getter selection
alter Cypress actions.
🤖 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/e2e/incidents/01.incidents.cy.ts`:
- Around line 96-102: Add an explicit incidentsPage.goTo() call at the start of
the “6. Admin perspective - Incidents page - Bar click selection walkthrough”
test before loading fixtures or applying filters, so it always begins on the
Incidents page independently of prior test state.

---

Nitpick comments:
In `@web/cypress/views/incidents-page.ts`:
- Around line 282-287: Remove both arbitrary cy.wait(250) calls from the setDays
interaction. Keep the existing scrollIntoView().click({ force: true }) action
and daysSelectList().should('be.visible') assertion so Cypress handles
actionability and dropdown readiness through its built-in retries.
- Around line 186-229: Remove the embedded scrollIntoView side effect from
incidentsDetailsAlertRuleCell so all incidents detail getters, including
incidentsDetailsAlertRuleLink and the Namespace/Severity/State/Start/End cell
getters, remain selector-only. Move scrolling to the relevant test call sites
before visibility assertions, preserving the existing visibility behavior
without making getter selection alter Cypress actions.
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: f83b6335-29a9-4837-b77e-b0c1c6c48e53

📥 Commits

Reviewing files that changed from the base of the PR and between fd2f1fd and 0a520a2.

📒 Files selected for processing (4)
  • web/cypress/e2e/incidents/01.incidents.cy.ts
  • web/cypress/e2e/incidents/regression/01.reg_filtering.cy.ts
  • web/cypress/e2e/incidents/regression/04.reg_redux_effects.cy.ts
  • web/cypress/views/incidents-page.ts

Comment thread web/cypress/e2e/incidents/01.incidents.cy.ts
@DavidRajnoha DavidRajnoha changed the title fix: make Incidents tab selector case-insensitive OBSINTA-1290: fix tests by making incidents tab selector case-insensitive Jul 15, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 15, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 15, 2026

Copy link
Copy Markdown

@DavidRajnoha: This pull request references OBSINTA-1290 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 task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

  • The OpenShift console changed the data-test-id for the Incidents tab from horizontal-link-incidents to horizontal-link-Incidents
  • This caused waitForIncidentsTab to time out in the before all hook, failing all Incidents tests
  • Uses the CSS attribute selector i flag for case-insensitive matching, supporting both casings

Test plan

  • Run incidents E2E suite against a cluster with the updated console
  • Verify the Incidents tab is found during warmUpForPlugin

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
  • Improved reliability of the Incidents page tab selection so it works consistently across different text casing.
  • Tests
  • Strengthened Incidents filtering and incident details verification by locating the correct expanded-row content more precisely and scrolling relevant UI elements into view before assertions.
  • Adjusted Incidents regression setup order and refined chip-visibility checks to be more consistent when filters are applied or removed.

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.

@DavidRajnoha DavidRajnoha changed the title OBSINTA-1290: fix tests by making incidents tab selector case-insensitive OBSINTA-1290: fix incident detection test selectors Jul 15, 2026
@DavidRajnoha

Copy link
Copy Markdown
Contributor Author

/cc @etmurasaki @tremes @sthundat

@openshift-ci
openshift-ci Bot requested review from sthundat and tremes July 15, 2026 11:16
searchForAlertInIncidentByFilter unconditionally cleared all filters
after checking each incident, including the one where the alert was
found. This deselected the incident before callers could inspect its
alert details, causing 80s timeouts on incidents-details-table.

Only clear filters when moving to the next incident (alert not found).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@DavidRajnoha
DavidRajnoha force-pushed the fix/incidents-tab-selector-case-insensitive branch from c965eaa to 94a4b27 Compare July 21, 2026 13:05
@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@DavidRajnoha: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants