OU-1040: Tests Absolute start dates and Matching start/end/dates#910
Open
DavidRajnoha wants to merge 1 commit intoopenshift:mainfrom
Open
OU-1040: Tests Absolute start dates and Matching start/end/dates#910DavidRajnoha wants to merge 1 commit intoopenshift:mainfrom
DavidRajnoha wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@DavidRajnoha: This pull request references OU-1040 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 bug 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DavidRajnoha 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 |
WalkthroughUpdates incident detection E2E test suite and fixtures to add a 15‑day data‑loading scenario, refactors incidents page helpers (visibility/tooltip collection), aligns Prometheus mock timestamps to 5‑minute boundaries, adjusts Cypress support, and removes obsolete test documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 7
🧹 Nitpick comments (1)
web/cypress/support/e2e.js (1)
6-13: Gate the command-log CSS behind a suite flag.This support file runs for every Cypress spec, so hiding all
request/xhrentries here makes unrelated failures much harder to debug. Prefer an env flag or an incidents-only hook so the noise reduction stays local to these suites.Possible adjustment
// Hide fetch/XHR request entries from the Cypress command log to reduce noise -const app = window.top; -if (app && !app.document.head.querySelector('[data-hide-command-log-request]')) { - const style = app.document.createElement('style'); - style.innerHTML = '.command-name-request, .command-name-xhr { display: none }'; - style.setAttribute('data-hide-command-log-request', ''); - app.document.head.appendChild(style); +if (Cypress.env('HIDE_REQUEST_COMMAND_LOG') === true) { + const app = window.top; + if (app && !app.document.head.querySelector('[data-hide-command-log-request]')) { + const style = app.document.createElement('style'); + style.innerHTML = '.command-name-request, .command-name-xhr { display: none }'; + style.setAttribute('data-hide-command-log-request', ''); + app.document.head.appendChild(style); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/e2e.js` around lines 6 - 13, Wrap the existing DOM-injection that hides request/xhr entries in a conditional driven by an opt-in suite flag (e.g. Cypress.env('hideCommandLogRequests')) instead of running unconditionally: check Cypress.env('hideCommandLogRequests') at the top, and only execute the block that creates the style element (the code referencing app, style, and the data-hide-command-log-request attribute and style.innerHTML) when that env flag is truthy; ensure the flag name is documented in the README or example CI config so affected suites can enable it when desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/incident_detection/tests/3.api_calls_data_loading_flows.md`:
- Around line 87-119: Update section 3.4 to mark the scenario as AUTOMATED by
referencing the new Cypress test suite and only keep truly manual steps as
manual; specifically, change "Automation Status: NOT AUTOMATED" to "Automation
Status: AUTOMATED" and add a line citing the test file
`web/cypress/e2e/incidents/regression/03.reg_15day_data_loading.cy.ts` (or test
suite/fixture name) as the automated coverage, then remove or annotate any steps
that are now covered by that e2e test so the checklist only contains remaining
manual verification items.
In `@web/cypress/e2e/incidents/regression/02.reg_ui_tooltip_boundary_times.cy.ts`:
- Around line 377-424: The test is indexing alerts by position which is brittle;
update the block using incidentsPage.getSelectedIncidentAlerts() to first read a
stable identifier (e.g., severity via getSeverityCell() or name via
getNameCell()) for each alert, build a small map keyed by that identifier, then
read start/end cells from the mapped alert elements and assign to
tableInfoStart/tableInfoEnd, tableWarningStart/tableWarningEnd,
tableCriticalStart/tableCriticalEnd accordingly; ensure you still assert
alerts.length === 3 and use the identifier values to pick the correct alert
element rather than alerts[0/1/2].
- Around line 50-55: The intercept for test data is being registered after
navigation which can race with network requests; move the
cy.mockIncidentFixture('incident-scenarios/19-15-day-data-loading.yaml') call
before incidentsPage.goTo() in the beforeEach so the fixture/intercepts are set
up prior to triggering initial incidents queries (i.e., swap the order of
cy.mockIncidentFixture and incidentsPage.goTo in the beforeEach).
In `@web/cypress/e2e/incidents/regression/03.reg_15day_data_loading.cy.ts`:
- Around line 82-99: collectAllStartDates() currently expands and reads fixed
row index 0 (calls like incidentsPage.expandRow(0),
incidentsDetailsStartCell(0), incidentsDetailsFiringStartCell(0)) so it ignores
the passed incidentId; update the helper to locate and operate on the row for
the given incidentId instead of index 0 — e.g. use or add a selector helper that
finds the incident row by incidentId (or call an existing
incidentsPage.findRowByIncidentId / incidentsPage.rowForIncidentId) then call
expandRow on that row and use incident-specific cell getters (replace
incidentsDetailsStartCell(0) and incidentsDetailsFiringStartCell(0) with their
incident-row-aware equivalents) so the function reads the correct table cells
for the requested incident.
- Around line 255-264: The tooltip start date assertion is too loose: replace
the loose expect(d).to.not.be.empty in the hover test that uses
incidentsPage.hoverOverIncidentBarById('ESCALATING-14d-monitoring-severity',
warningIndex) and incidentsPage.getTooltipStartDate() with a strict check that
the returned value equals one of the two acceptable documented values (the
warning segment start or the incident/info segment start), i.e. assert that d
=== warningSegment.start OR d === infoSegment.start (or use a
predicate/satisfy-style assertion) so only those two documented outputs pass.
- Around line 51-56: The test currently calls incidentsPage.goTo() before
registering the mock which causes a race; remove the explicit
incidentsPage.goTo() call in the beforeEach and instead call
cy.mockIncidentFixture('incident-scenarios/19-15-day-data-loading.yaml') first
so the mock interceptor is registered and let cy.mockIncidentFixture() perform
navigation (it already calls goTo() internally).
In `@web/cypress/support/incidents_prometheus_query_mocks/mock-generators.ts`:
- Around line 15-31: The code currently snaps start/end to an absolute 5-minute
grid (alignedStart/alignedEnd) which can produce timestamps outside the
requested range; change it to use the caller-provided startTime as the grid
origin: set the first timestamp to startTime (or startTime when duration <
fiveMinutes) and then loop currentTime = startTime; while (currentTime <=
endTime) { timestamps.push(currentTime); currentTime += fiveMinutes; } remove
Math.ceil/Math.floor alignment and reference fiveMinutes, startTime, endTime,
timestamps, currentTime when making the change.
---
Nitpick comments:
In `@web/cypress/support/e2e.js`:
- Around line 6-13: Wrap the existing DOM-injection that hides request/xhr
entries in a conditional driven by an opt-in suite flag (e.g.
Cypress.env('hideCommandLogRequests')) instead of running unconditionally: check
Cypress.env('hideCommandLogRequests') at the top, and only execute the block
that creates the style element (the code referencing app, style, and the
data-hide-command-log-request attribute and style.innerHTML) when that env flag
is truthy; ensure the flag name is documented in the README or example CI config
so affected suites can enable it when desired.
🪄 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: 555ed8fe-6cad-46ab-bce4-895a6a2e538e
📒 Files selected for processing (11)
docs/incident_detection/tests/3.api_calls_data_loading_flows.mdweb/cypress/e2e/incidents/regression/02.reg_ui_tooltip_boundary_times.cy.tsweb/cypress/e2e/incidents/regression/03.reg_15day_data_loading.cy.tsweb/cypress/fixtures/coo/monitoring-ui-plugin.yamlweb/cypress/fixtures/incident-scenarios/19-15-day-data-loading.yamlweb/cypress/fixtures/incident-scenarios/21-multi-severity-boundary-times.yamlweb/cypress/support/e2e.jsweb/cypress/support/incidents_prometheus_query_mocks/mock-generators.tsweb/cypress/support/incidents_prometheus_query_mocks/prometheus-mocks.tsweb/cypress/views/incidents-page.tsweb/src/components/data-test.ts
💤 Files with no reviewable changes (1)
- web/cypress/fixtures/incident-scenarios/21-multi-severity-boundary-times.yaml
…d 15-day data loading Add regression tests for mixed severity interval boundary times (OU-1205, OU-1221) and 15-day data loading with time filter consistency. Extend the incidents page object with tooltip date extraction helpers, visible segment counting, and incident-by-ID hover/select methods. Rename 05.reg_15day to 03.reg_15day to reflect correct test ordering. Made-with: Cursor
DavidRajnoha
force-pushed
the
test/OU-1040-incidents-mocking-min-timestamp
branch
from
15a00ac to
29ae97e
Compare
Contributor
Author
|
/test e2e-incidents |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/e2e/incidents/regression/02.reg_ui_tooltip_boundary_times.cy.ts`:
- Around line 50-60: The beforeEach setup currently mocks the fixture but
doesn't navigate to the incidents page, which makes tests brittle; update the
beforeEach block to call incidentsPage.goTo() immediately after
cy.mockIncidentFixture('incident-scenarios/19-15-day-data-loading.yaml') so each
test explicitly navigates to the incidents page (match the pattern used in
02.incidents-mocking-example.cy.ts) and retains the existing setup functions
(incidentsPage.clearAllFilters(), incidentsPage.setDays('15 days'), etc.).
In `@web/cypress/views/incidents-page.ts`:
- Around line 648-667: hoverOverAlertBar returns incidentsPage.waitForTooltip()
which waits for the global .incidents__tooltip and can pass if an unrelated
incident tooltip is already visible; change the hover flow to wait specifically
for the alert-chart tooltip used by collectAlertTooltip (e.g., call a new or
existing helper like incidentsPage.waitForAlertTooltip() or
incidentsPage.waitForTooltip('.alerts__tooltip') after triggering the hover),
ensuring the hoverOverAlertBar function (and the call site using
collectAlertTooltip) awaits the alert-specific tooltip update rather than any
visible tooltip.
🪄 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: 59629f06-4936-4813-9306-fcaf5669c760
📒 Files selected for processing (11)
docs/incident_detection/tests/3.api_calls_data_loading_flows.mdweb/cypress/e2e/incidents/regression/02.reg_ui_tooltip_boundary_times.cy.tsweb/cypress/e2e/incidents/regression/03.reg_15day_data_loading.cy.tsweb/cypress/fixtures/coo/monitoring-ui-plugin.yamlweb/cypress/fixtures/incident-scenarios/19-15-day-data-loading.yamlweb/cypress/fixtures/incident-scenarios/21-multi-severity-boundary-times.yamlweb/cypress/support/e2e.jsweb/cypress/support/incidents_prometheus_query_mocks/mock-generators.tsweb/cypress/support/incidents_prometheus_query_mocks/prometheus-mocks.tsweb/cypress/views/incidents-page.tsweb/src/components/data-test.ts
💤 Files with no reviewable changes (1)
- web/cypress/fixtures/incident-scenarios/21-multi-severity-boundary-times.yaml
✅ Files skipped from review due to trivial changes (3)
- web/cypress/fixtures/coo/monitoring-ui-plugin.yaml
- web/src/components/data-test.ts
- web/cypress/support/incidents_prometheus_query_mocks/prometheus-mocks.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/cypress/support/e2e.js
- web/cypress/fixtures/incident-scenarios/19-15-day-data-loading.yaml
- web/cypress/e2e/incidents/regression/03.reg_15day_data_loading.cy.ts
- docs/incident_detection/tests/3.api_calls_data_loading_flows.md
Contributor
|
@DavidRajnoha: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit