OBSINTA-1006: Add UI performance benchmarks for Incidents page#873
OBSINTA-1006: Add UI performance benchmarks for Incidents page#873DavidRajnoha wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@DavidRajnoha: This pull request references OBSINTA-1006 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 "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
needs-rebase
|
/test e2e-incidents |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Cypress performance test specs and fixtures for the Incidents page, a shelved endurance test source in docs, a small Cypress config change, and refactors incidents-page Cypress view helpers (signature and selector changes). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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: 1
🧹 Nitpick comments (4)
web/cypress/views/incidents-page.ts (1)
364-378: Consider using the centralized element selector for the alerts table.Both
selectIncidentByIdanddeselectIncidentByIduse a hardcoded selector string[data-test="incidents-alerts-table"]rather than the centralizedincidentsTableelement defined at line 116. This could lead to maintenance issues if the data-test ID changes.♻️ Suggested refactor to use centralized selector
.click({ force: true, log }) .then(() => { - return cy.get(`[data-test="incidents-alerts-table"]`, { log }) + return incidentsPage.elements.incidentsTable() .scrollIntoView({ log }) .should('exist'); });Note: If the data-test IDs are intentionally different between
incidentsTableandincidents-alerts-table, disregard this suggestion.Also applies to: 387-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/incidents-page.ts` around lines 364 - 378, Replace the hardcoded selector string `[data-test="incidents-alerts-table"]` in selectIncidentById and deselectIncidentById with the centralized incidentsTable element defined earlier (use the incidentsTable selector/variable from line 116) so both functions reference the single source of truth; update the cy.get(...) call chains in selectIncidentById and deselectIncidentById to use incidentsTable (including the scrollIntoView and should('exist') steps) to avoid duplicate hardcoded selectors.web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts (1)
148-148: Same suggestion as walkthrough test: document the 10-second waits.Multiple
cy.wait(10000)calls appear without explanation. A brief comment would clarify their purpose.Also applies to: 200-200, 214-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts` at line 148, Three occurrences of cy.wait(10000) in 01.performance_benchmark.cy.ts are undocumented; add brief comments explaining why each 10s hard wait is required (e.g., waiting for background indexing, external service readiness, or UI stabilization) so reviewers understand the intention. Locate the cy.wait(10000) calls (around the current assertions/steps at the positions shown) and prepend a one-line comment describing the specific reason and whether it can be replaced by a conditional check (like cy.intercept/wait for a selector) in future.web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts (2)
171-171: Consider adding a comment explaining the 10-second wait.The
cy.wait(10000)at the start of test 7.2 lacks explanation. If this is to allow teardown/GC from the previous test, a brief comment would help future maintainers understand the intent.📝 Suggested documentation
it('7.2 Walkthrough: Table row expansion with 100 and 500 alerts', () => { + // Allow browser/Cypress to stabilize after filter/time-range test before loading new fixtures cy.wait(10000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts` at line 171, Add a brief inline comment above the cy.wait(10000) in 02.performance_walkthrough.cy.ts explaining why a 10s static wait is required for test "7.2" (e.g., to allow previous-test teardown/GC or system warmup), reference the exact statement `cy.wait(10000)`, and optionally add a TODO suggesting replacing the hard wait with an explicit condition or stable selector check in the future.
42-63: Consider extracting shared benchmark utilities.The
markStartandrecordBenchmarkhelper functions are duplicated between this file and01.performance_benchmark.cy.ts. Consider extracting them to a shared utility file to reduce duplication.Also applies to: 52-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts` around lines 42 - 63, Extract the duplicated benchmarking helpers into a shared utility: move benchmarkResults array plus the functions markStart(label) and recordBenchmark(label, thresholdMs) into a new module (exporting the array and both functions), update both spec files to import { benchmarkResults, markStart, recordBenchmark } from that module, and ensure recordBenchmark still pushes into the exported benchmarkResults and uses cy.window, cy.log, cy.task and the same expect assertion; keep function names exactly markStart and recordBenchmark so existing calls require minimal edits.
🤖 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/performance/01.performance_benchmark.cy.ts`:
- Around line 199-211: Tests 6.3 and 6.4 call markStart before
cy.mockIncidentFixture, which causes the fixture load time to be included in the
benchmark and is inconsistent with benchmarkIncidentsChart (which marks start
after mocking); move the markStart call so it runs after
cy.mockIncidentFixture('incident-scenarios/22-benchmark-20-incidents.yaml') (and
similarly for the 40-incidents test) and before
incidentsPage.clearAllFilters()/elements assertions so the timing excludes
fixture loading and matches benchmarkIncidentsChart's pattern.
---
Nitpick comments:
In `@web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts`:
- Line 148: Three occurrences of cy.wait(10000) in
01.performance_benchmark.cy.ts are undocumented; add brief comments explaining
why each 10s hard wait is required (e.g., waiting for background indexing,
external service readiness, or UI stabilization) so reviewers understand the
intention. Locate the cy.wait(10000) calls (around the current assertions/steps
at the positions shown) and prepend a one-line comment describing the specific
reason and whether it can be replaced by a conditional check (like
cy.intercept/wait for a selector) in future.
In `@web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts`:
- Line 171: Add a brief inline comment above the cy.wait(10000) in
02.performance_walkthrough.cy.ts explaining why a 10s static wait is required
for test "7.2" (e.g., to allow previous-test teardown/GC or system warmup),
reference the exact statement `cy.wait(10000)`, and optionally add a TODO
suggesting replacing the hard wait with an explicit condition or stable selector
check in the future.
- Around line 42-63: Extract the duplicated benchmarking helpers into a shared
utility: move benchmarkResults array plus the functions markStart(label) and
recordBenchmark(label, thresholdMs) into a new module (exporting the array and
both functions), update both spec files to import { benchmarkResults, markStart,
recordBenchmark } from that module, and ensure recordBenchmark still pushes into
the exported benchmarkResults and uses cy.window, cy.log, cy.task and the same
expect assertion; keep function names exactly markStart and recordBenchmark so
existing calls require minimal edits.
In `@web/cypress/views/incidents-page.ts`:
- Around line 364-378: Replace the hardcoded selector string
`[data-test="incidents-alerts-table"]` in selectIncidentById and
deselectIncidentById with the centralized incidentsTable element defined earlier
(use the incidentsTable selector/variable from line 116) so both functions
reference the single source of truth; update the cy.get(...) call chains in
selectIncidentById and deselectIncidentById to use incidentsTable (including the
scrollIntoView and should('exist') steps) to avoid duplicate hardcoded
selectors.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 289c9c99-bf13-4161-9c8e-d4e95a9c1919
📒 Files selected for processing (8)
docs/incident_detection/tests/performance/03.endurance_test_source.mddocs/incident_detection/tests/performance/overview.mdweb/cypress.config.tsweb/cypress/e2e/incidents/performance/01.performance_benchmark.cy.tsweb/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.tsweb/cypress/fixtures/incident-scenarios/22-benchmark-20-incidents.yamlweb/cypress/fixtures/incident-scenarios/23-benchmark-mixed-size-incidents.yamlweb/cypress/views/incidents-page.ts
DavidRajnoha
force-pushed
the
test/OBSINTA-1006-UI-load-tests
branch
from
7e5f573 to
a341bf9
Compare
openshift-ci
bot
removed
the
needs-rebase
|
/test e2e-incidents |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
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/views/incidents-page.ts`:
- Around line 415-420: The selector currently uses substring matching in the
cy.get call that targets `[data-test="incidents-chart-bars"]
g[data-test*="${incidentId}"]`, which can match unintended IDs; update the
selector to target the exact bar suffix (the `data-test` follows
`incidents-chart-bar-{group_id}`) by using an ends-with selector or full exact
match for the `g[data-test]` part (e.g., change `g[data-test*="${incidentId}"]`
to `g[data-test$="-${incidentId}"]` or
`g[data-test="incidents-chart-bar-${incidentId}"]`) so the correct incident bar
is selected without relying on `.first()`; keep the rest of the chain (.first(),
.click()) as needed.
- Around line 438-443: The selector is using a substring match
([data-test*="${incidentId}"]) which can match unintended elements; update the
selector to use an ends-with match like `[data-test$="-${incidentId}"]` (same
approach used in selectIncidentById) for the query targeting
`[data-test="incidents-chart-bars"] g ... path[role="presentation"]`, then keep
the chained .first().click({ force: true, log }) behavior unchanged so the test
consistently targets the correct incident chart bar.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 848fc785-59e6-4286-a7ce-f5cf7499fb6c
📒 Files selected for processing (8)
docs/incident_detection/tests/performance/03.endurance_test_source.mddocs/incident_detection/tests/performance/overview.mdweb/cypress.config.tsweb/cypress/e2e/incidents/performance/01.performance_benchmark.cy.tsweb/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.tsweb/cypress/fixtures/incident-scenarios/22-benchmark-20-incidents.yamlweb/cypress/fixtures/incident-scenarios/23-benchmark-mixed-size-incidents.yamlweb/cypress/views/incidents-page.ts
✅ Files skipped from review due to trivial changes (4)
- web/cypress.config.ts
- web/cypress/fixtures/incident-scenarios/23-benchmark-mixed-size-incidents.yaml
- docs/incident_detection/tests/performance/03.endurance_test_source.md
- docs/incident_detection/tests/performance/overview.md
🚧 Files skipped from review as they are similar to previous changes (3)
- web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts
- web/cypress/fixtures/incident-scenarios/22-benchmark-20-incidents.yaml
- web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts
DavidRajnoha
force-pushed
the
test/OBSINTA-1006-UI-load-tests
branch
from
a341bf9 to
a48a1b9
Compare
|
/test e2e-incidents |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/cypress/views/incidents-page.ts (1)
206-209: Avoid hard sleeps in this shared helper.These two
cy.wait(250)calls add a fixed 500 ms to everysetDays()call and make the new performance specs measure helper delays instead of UI readiness. Prefer waiting on the dropdown state only.Suggested change
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.daysSelectToggle().should('be.visible').scrollIntoView().click({ force: true }); incidentsPage.elements.daysSelectList().should('be.visible'); - cy.wait(250); const dayKey = value.replace(' ', '-'); cy.byTestID(`${DataTestIDs.IncidentsPage.DaysSelectOption}-${dayKey}`) .should('be.visible') .click();As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/incidents-page.ts` around lines 206 - 209, The two fixed sleeps in the setDays helper should be removed and replaced with UI-driven waits: remove both cy.wait(250) calls and rely on incidentsPage.elements.daysSelectToggle().scrollIntoView().click({ force: true }) followed immediately by incidentsPage.elements.daysSelectList().should('be.visible') (and, after selecting an option, assert the list is hidden or the selection is applied as needed). Update the setDays helper to wait only on incidentsPage.elements.daysSelectList() visibility (and subsequent expected state) instead of using fixed delays so tests wait for UI readiness rather than arbitrary time.
🤖 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/views/incidents-page.ts`:
- Around line 412-417: Change the default logging behavior for the incident
selectors to avoid Cypress logging overhead in perf tests: update
selectIncidentById and deselectIncidentById so their options default log to
false (const log = options?.log ?? false) instead of true; this preserves opt-in
logging for individual calls while allowing existing performance tests to call
the helpers without incurring log overhead (alternatively, if you prefer not to
change defaults, update the perf specs to call selectIncidentById(id, { log:
false }) and deselectIncidentById(id, { log: false })).
---
Nitpick comments:
In `@web/cypress/views/incidents-page.ts`:
- Around line 206-209: The two fixed sleeps in the setDays helper should be
removed and replaced with UI-driven waits: remove both cy.wait(250) calls and
rely on incidentsPage.elements.daysSelectToggle().scrollIntoView().click({
force: true }) followed immediately by
incidentsPage.elements.daysSelectList().should('be.visible') (and, after
selecting an option, assert the list is hidden or the selection is applied as
needed). Update the setDays helper to wait only on
incidentsPage.elements.daysSelectList() visibility (and subsequent expected
state) instead of using fixed delays so tests wait for UI readiness rather than
arbitrary time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61ab497a-0d5e-4f69-8e19-988829258895
📒 Files selected for processing (4)
docs/incident_detection/tests/performance/03.endurance_test_source.mddocs/incident_detection/tests/performance/overview.mdweb/cypress/e2e/incidents/performance/01.performance_benchmark.cy.tsweb/cypress/views/incidents-page.ts
✅ Files skipped from review due to trivial changes (3)
- web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts
- docs/incident_detection/tests/performance/overview.md
- docs/incident_detection/tests/performance/03.endurance_test_source.md
| selectIncidentById: (incidentId: string, options?: { log?: boolean }) => { | ||
| const log = options?.log ?? true; | ||
| if (log) cy.log(`incidentsPage.selectIncidentById: ${incidentId}`); | ||
| return cy.get( | ||
| `[data-test="incidents-chart-bars"] g[data-test$="-${incidentId}"] path[role="presentation"]`, | ||
| { log }, |
There was a problem hiding this comment.
The new log option does not help the benchmark path with a true default.
selectIncidentById() and deselectIncidentById() now accept { log?: boolean }, but both default to true. The current performance callers still invoke selectIncidentById(incidentId) without options (web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts:164-169, web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts:177-181), so the benchmark path keeps full Cypress command logging overhead. Either flip the default to false here or update the perf specs to pass { log: false }.
Suggested change
selectIncidentById: (incidentId: string, options?: { log?: boolean }) => {
- const log = options?.log ?? true;
+ const log = options?.log ?? false;
if (log) cy.log(`incidentsPage.selectIncidentById: ${incidentId}`); deselectIncidentById: (incidentId: string, options?: { log?: boolean }) => {
- const log = options?.log ?? true;
+ const log = options?.log ?? false;
if (log) cy.log(`incidentsPage.deselectIncidentById: ${incidentId}`);As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 435-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/cypress/views/incidents-page.ts` around lines 412 - 417, Change the
default logging behavior for the incident selectors to avoid Cypress logging
overhead in perf tests: update selectIncidentById and deselectIncidentById so
their options default log to false (const log = options?.log ?? false) instead
of true; this preserves opt-in logging for individual calls while allowing
existing performance tests to call the helpers without incurring log overhead
(alternatively, if you prefer not to change defaults, update the perf specs to
call selectIncidentById(id, { log: false }) and deselectIncidentById(id, { log:
false })).
|
/test e2e-incidents |
2 similar comments
|
/test e2e-incidents |
|
/test e2e-incidents |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/performance/01.performance_benchmark.cy.ts`:
- Around line 73-79: The PASS/FAIL check treats exact-threshold runs as FAIL;
update the comparison to allow equality by changing the status logic to use
elapsedMs <= thresholdMs (reference: status, elapsedMs, thresholdMs) and change
the assertion to use a "at most" check (e.g., expect(elapsedMs, `${label} should
complete within ${thresholdMs}ms`).to.be.at.most(thresholdMs)) so runs equal to
the threshold pass while preserving cy.log/msg behavior.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f035924c-d507-429e-9c5d-48aa85372531
📒 Files selected for processing (2)
web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.tsweb/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts
DavidRajnoha
force-pushed
the
test/OBSINTA-1006-UI-load-tests
branch
2 times, most recently
from
dbaf6dc to
b96e6a1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/cypress/views/incidents-page.ts (1)
412-417:⚠️ Potential issue | 🟠 MajorMake logging opt-in on the ID select/deselect hot path.
Line 413 and Line 436 still default
logtotrue, so these helpers keep full Cypress command-log overhead unless every benchmark caller passes{ log: false }. Either flip the default here or verify that all performance specs opt out explicitly.Suggested change
selectIncidentById: (incidentId: string, options?: { log?: boolean }) => { - const log = options?.log ?? true; + const log = options?.log ?? false; if (log) cy.log(`incidentsPage.selectIncidentById: ${incidentId}`);deselectIncidentById: (incidentId: string, options?: { log?: boolean }) => { - const log = options?.log ?? true; + const log = options?.log ?? false; if (log) cy.log(`incidentsPage.deselectIncidentById: ${incidentId}`);Run this to inspect the perf call sites:
#!/bin/bash set -euo pipefail if [ -d web/cypress/e2e/incidents/performance ]; then rg -n -C2 '\bselectIncidentById\s*\(|\bdeselectIncidentById\s*\(' web/cypress/e2e/incidents/performance fiExpected: benchmark/endurance call sites either pass
{ log: false }or these helpers default tofalse.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 435-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/incidents-page.ts` around lines 412 - 417, The selectIncidentById and deselectIncidentById helpers default the Cypress { log } option to true which forces heavy command logging on the hot path; change the default to false (e.g., const log = options?.log ?? false) so callers opt into logging, and audit performance test call sites to ensure no regressions (ensure any benchmark/endurance specs explicitly pass { log: true } if they need logs).
🧹 Nitpick comments (1)
web/cypress/views/incidents-page.ts (1)
206-209: Avoid fixed sleeps insetDays().Line 206 and Line 209 add 500ms of unconditional delay to every call. In a shared helper that the new perf flows can hit, that makes timings noisier and the suite slower; the existing visibility assertions should be the sync point instead.
Suggested change
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(' ', '-');As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/incidents-page.ts` around lines 206 - 209, The two unconditional cy.wait(250) calls around the dropdown interaction in setDays() should be removed; rely on the existing sync assertions instead by removing the wait before clicking incidentsPage.elements.daysSelectToggle() and the wait after incidentsPage.elements.daysSelectList().should('be.visible'), and ensure you assert visibility (daysSelectList().should('be.visible')) before proceeding to any actions that depend on the list being open (using incidentsPage.elements.daysSelectToggle() and daysSelectList() as the unique selectors to locate the code to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/cypress/views/incidents-page.ts`:
- Around line 412-417: The selectIncidentById and deselectIncidentById helpers
default the Cypress { log } option to true which forces heavy command logging on
the hot path; change the default to false (e.g., const log = options?.log ??
false) so callers opt into logging, and audit performance test call sites to
ensure no regressions (ensure any benchmark/endurance specs explicitly pass {
log: true } if they need logs).
---
Nitpick comments:
In `@web/cypress/views/incidents-page.ts`:
- Around line 206-209: The two unconditional cy.wait(250) calls around the
dropdown interaction in setDays() should be removed; rely on the existing sync
assertions instead by removing the wait before clicking
incidentsPage.elements.daysSelectToggle() and the wait after
incidentsPage.elements.daysSelectList().should('be.visible'), and ensure you
assert visibility (daysSelectList().should('be.visible')) before proceeding to
any actions that depend on the list being open (using
incidentsPage.elements.daysSelectToggle() and daysSelectList() as the unique
selectors to locate the code to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f16d92e5-e85c-430f-8f99-5c95cb5bf851
📒 Files selected for processing (8)
docs/incident_detection/tests/performance/03.endurance_test_source.mddocs/incident_detection/tests/performance/overview.mdweb/cypress.config.tsweb/cypress/e2e/incidents/performance/01.performance_benchmark.cy.tsweb/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.tsweb/cypress/fixtures/incident-scenarios/22-benchmark-20-incidents.yamlweb/cypress/fixtures/incident-scenarios/23-benchmark-mixed-size-incidents.yamlweb/cypress/views/incidents-page.ts
✅ Files skipped from review due to trivial changes (6)
- web/cypress.config.ts
- web/cypress/fixtures/incident-scenarios/23-benchmark-mixed-size-incidents.yaml
- web/cypress/e2e/incidents/performance/01.performance_benchmark.cy.ts
- docs/incident_detection/tests/performance/03.endurance_test_source.md
- web/cypress/fixtures/incident-scenarios/22-benchmark-20-incidents.yaml
- docs/incident_detection/tests/performance/overview.md
🚧 Files skipped from review as they are similar to previous changes (1)
- web/cypress/e2e/incidents/performance/02.performance_walkthrough.cy.ts
DavidRajnoha
force-pushed
the
test/OBSINTA-1006-UI-load-tests
branch
2 times, most recently
from
c91ba4c to
449af96
Compare
…OBSINTA-1006) Add Cypress performance benchmarks measuring wall-clock render times for incidents chart, alerts detail chart, filter interactions, time range switching, and table row expansion under escalating data loads (100-500 alerts, 12-20 incidents). Thresholds calibrated at 1.5x+ worst observed values across two CI clusters for stable pass rates. - 01.performance_benchmark: chart render benchmarks with escalating loads - 02.performance_walkthrough: interactive filter/time-range/table benchmarks - Fixtures: 20 uniform incidents, 12 mixed-size heterogeneous incidents - Revert selectIncidentById/deselectIncidentById to main (optimized selectors showed no measurable advantage across 6 benchmark runs) - Documentation: performance test overview and endurance test source Made-with: Cursor
DavidRajnoha
force-pushed
the
test/OBSINTA-1006-UI-load-tests
branch
from
76cede7 to
f1c6df9
Compare
|
[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 |
DavidRajnoha
changed the title
|
@DavidRajnoha: This pull request references OBSINTA-1006 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. 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. |
|
@DavidRajnoha: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Adds Cypress performance benchmarks that measure wall-clock render times for key Incidents page operations under escalating data loads, serving as regression indicators for future code changes.
New test specs
01.performance_benchmark.cy.ts— Measures initial chart render times:02.performance_walkthrough.cy.ts— Measures incremental re-render cost during interactive use:New fixtures
22-benchmark-20-incidents.yaml— 20 uniform single-alert incidents23-benchmark-mixed-size-incidents.yaml— 12 incidents with 1–15 alerts eachThreshold calibration
Thresholds were calibrated across 6 runs on 2 different CI clusters and set at 1.5x+ the worst observed value, rounded to a single significant digit. The Incidents page rendering is not on the critical performance path of the overall system, so the thresholds are intentionally generous — the goal is to catch significant regressions, not to enforce tight performance budgets.
Other changes
docs/incident_detection/tests/performance/overview.mdand03.endurance_test_source.md