NO-JIRA: perses and monitoring automation testing - stabilization#924
Draft
etmurasaki wants to merge 2 commits into
Draft
NO-JIRA: perses and monitoring automation testing - stabilization#924etmurasaki wants to merge 2 commits into
etmurasaki wants to merge 2 commits into
Conversation
|
@etmurasaki: This pull request explicitly references no jira issue. 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-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etmurasaki The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/test e2e-monitoring |
Contributor
Author
|
/test e2e-perses |
Contributor
Author
|
/test all |
WalkthroughThis PR improves Perses dashboard test robustness and monitoring test reliability by standardizing test setup with cleanup and namespace switching, updating validation constants and page object assertions, quoting kubeconfig paths across CLI commands, and replacing fixed waits with visibility-based state checks throughout the test suite. ChangesPerses Test Stabilization and Fixture Updates
Kubeconfig Path Quoting (Infrastructure Safety)
Monitoring and Test Robustness Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 5
🧹 Nitpick comments (8)
web/cypress/support/commands/dashboards-commands.ts (1)
33-66: ⚡ Quick winCentralize the Perses fixture list instead of hardcoding each manifest here.
The same five filenames are duplicated in
setupDashboardsAndPlugins()andcleanupDashboards(), andcleanupDashboards()still has two identical delete branches. Moving these fixture names into one shared array/helper will keep setup and cleanup from drifting and reduce future flake from leftover resources.🤖 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/support/commands/dashboards-commands.ts` around lines 33 - 66, In setupDashboardsAndPlugins() and cleanupDashboards(), remove the repeated hardcoded manifest strings and duplicate delete branches by extracting the five dashboard filenames (openshift-cluster-sample-dashboard.yaml, perses-dashboard-sample.yaml, prometheus-overview-variables.yaml, thanos-compact-overview-1var.yaml, thanos-querier-datasource.yaml) into a single shared array or helper (e.g., DASHBOARD_FIXTURES) and iterate over that array: in setup call cy.exec with `oc apply -f ./cypress/fixtures/.../<filename> --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}` for each entry, and in cleanup iterate the same array to run `oc delete -f .../<filename> --kubeconfig ...`; also remove the duplicated delete branch currently present in cleanupDashboards().web/cypress/views/alerting-rule-list-page.ts (1)
33-47: ⚡ Quick winRemove
try/catcharound Cypress command chains inbyName.This
try/catchwon't reliably catch Cypress command/assertion failures because commands execute asynchronously via Cypress's queue. Cypress commands intentionally fail tests on errors rather than allowing recovery through catch blocks. Use direct command chaining and let Cypress fail naturally.♻️ Suggested refactor
byName: (name: string, ouiaId: string = FilterOUIAIDs.RuleNameFilter) => { - cy.log('listPage.filter.byName'); - try { - listPage.filter.selectAttribute('Name'); - cy.byOUIAID(`${ouiaId}-input`) - .find('input') - .scrollIntoView() - .as('input') - .should('be.visible'); - cy.get('@input', { timeout: 10000 }) - .scrollIntoView() - .type(name + '{enter}'); - cy.get('@input', { timeout: 10000 }).scrollIntoView().should('have.attr', 'value', name); - } catch (error) { - cy.log(`${error.message}`); - throw error; - } + cy.log('alertingRuleListPage.filter.byName'); + listPage.filter.selectAttribute('Name'); + cy.byOUIAID(`${ouiaId}-input`) + .find('input') + .should('be.visible') + .as('input'); + cy.get('@input', { timeout: 10000 }).clear().type(`${name}{enter}`); + cy.get('@input', { timeout: 10000 }).should('have.value', name); },🤖 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/alerting-rule-list-page.ts` around lines 33 - 47, Remove the try/catch wrapper around the Cypress command chain in the byName implementation: delete the try { ... } catch (error) { ... } block and keep the direct command chain starting with listPage.filter.selectAttribute('Name') followed by cy.byOUIAID(`${ouiaId}-input`).find('input').scrollIntoView().as('input').should('be.visible'); cy.get('@input', { timeout: 10000 }).scrollIntoView().type(name + '{enter}'); cy.get('@input', { timeout: 10000 }).scrollIntoView().should('have.attr', 'value', name); so that Cypress can handle failures natively instead of attempting to catch them.web/cypress/views/perses-dashboards-create-dashboard.ts (1)
69-83: ⚡ Quick winAvoid strict full-text equality for backend error alerts.
Using
have.texthere is fragile for server-originated messages. Prefercontain.text(or stable fragments) to keep tests robust across minor wording/formatting changes.♻️ Suggested change
assertMaxLengthValidation: () => { cy.log('persesCreateDashboardsPage.assertMaxLengthValidation'); cy.byPFRole('dialog') .find('h4') - .should('have.text', persesCreateDashboard.DIALOG_MAX_LENGTH_VALIDATION) + .should('contain.text', 'cannot contain more than 75 characters') .should('be.visible'); }, assertDuplicatedNameValidation: () => { cy.log('persesCreateDashboardsPage.assertDuplicatedNameValidation'); cy.byPFRole('dialog') .find('h4') - .should('have.text', persesCreateDashboard.DIALOG_CREATE_NAME_BKD_VALIDATION) + .should('contain.text', persesCreateDashboard.DIALOG_CREATE_NAME_BKD_VALIDATION) .should('be.visible'); },🤖 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/perses-dashboards-create-dashboard.ts` around lines 69 - 83, The two assertions in persesCreateDashboardsPage (assertMaxLengthValidation and assertDuplicatedNameValidation) use strict full-text checks via should('have.text', ...) which is brittle for backend-originated alerts; change both to use a containment check (e.g., should('contain.text', ...)) or assert on a stable fragment of the expected message so the tests tolerate minor wording/formatting changes while still verifying the correct validation dialog is shown.web/cypress/views/perses-dashboards.ts (1)
170-174: ⚡ Quick winReplace fixed sleep with a deterministic readiness check.
Line 173 adds a hard wait that can still flake under slow CI and always adds delay under fast CI. Prefer asserting a stable post-navigation signal instead of sleeping.
♻️ Suggested change
cy.byTestID(DataTestIDs.PersesDashboardDropdown) .find('input') .should('have.value', dashboardName); - cy.wait(2000); + cy.byAriaLabel(persesAriaLabels.RefreshButton) + .scrollIntoView() + .should('be.visible');🤖 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/perses-dashboards.ts` around lines 170 - 174, The fixed cy.wait(2000) should be replaced with a deterministic readiness check: remove cy.wait and instead assert a stable post-navigation signal such as verifying the dashboard UI has fully loaded (for example, assert the dashboard title or header contains dashboardName, or wait on a network alias via cy.intercept and cy.wait('@alias') that represents fetching the dashboard). Locate the snippet using DataTestIDs.PersesDashboardDropdown and the input assertion, then replace the static wait with a targeted assertion like checking the visible dashboard header/text equals dashboardName or waiting on an XHR/fetch alias that finishes loading the dashboard content.web/cypress/support/perses/01.coo_list_perses_admin.cy.ts (1)
369-371: ⚡ Quick winUse page-ready sync instead of an extra fixed delay.
Line 370 adds another hard wait in the duplicate flow. Replace it with a deterministic list-page load assertion to reduce flakiness and runtime.
♻️ Suggested change
persesDashboardsPage.backToListPersesDashboardsPage(); - cy.wait(2000); + listPersesDashboardsPage.shouldBeLoaded(); listPersesDashboardsPage.filter.byProject('perses-dev');🤖 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/support/perses/01.coo_list_perses_admin.cy.ts` around lines 369 - 371, Replace the fixed cy.wait(2000) after persesDashboardsPage.backToListPersesDashboardsPage() with a deterministic "page ready" assertion: remove the hard delay and instead wait for the list page to be fully loaded by asserting a stable UI signal (for example, a visible table or filter input, or an existing page-ready helper like listPersesDashboardsPage.waitForReady()/ensureLoaded()); then call listPersesDashboardsPage.filter.byProject('perses-dev') once that assertion passes to avoid flakiness.web/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.ts (1)
71-73: ⚡ Quick winUse nav-state assertion instead of fixed delay in
beforeEach.At Line 72,
cy.wait(2000)is brittle and slows every test. Wait for the target sidebar item to be visible before clicking it.♻️ Suggested adjustment
beforeEach(() => { nav.sidenav.clickNavLink(['Observe', 'Dashboards']); - cy.wait(2000); + cy.get('#page-sidebar').contains('Dashboards (Perses)').should('be.visible'); nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); });🤖 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/e2e/perses/99.coo_rbac_perses_user6.cy.ts` around lines 71 - 73, Replace the brittle fixed delay by waiting for the sidebar item to be present/visible before clicking: remove cy.wait(2000) in the beforeEach and instead assert the nav state for the target item (use the nav.sidenav API such as checking the specific nav item or a visibility/state assertion for ['Observe','Dashboards'] or ['Observe','Dashboards (Perses)']) then call nav.sidenav.clickNavLink([...]) once the assertion passes; update the beforeEach that currently calls nav.sidenav.clickNavLink(['Observe','Dashboards']); cy.wait(2000); nav.sidenav.clickNavLink(['Observe','Dashboards (Perses)']) to assert visibility/state on the sidebar entry and then perform the clicks without any fixed wait.web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts (1)
445-453: ⚡ Quick winReplace fixed sleep with state-based synchronization in kebab flow.
On Line 445,
cy.wait(2000)can still race on slow environments and adds idle delay on fast ones. Prefer waiting on dashboard list state before opening the kebab menu.♻️ Suggested adjustment
cy.log(`6.4. Change namespace to openshift-cluster-observability-operator`); cy.changeNamespace('openshift-cluster-observability-operator'); - cy.wait(2000); cy.log(`6.5. Assert Kebab icon is enabled`); listPersesDashboardsPage.clearAllFilters(); listPersesDashboardsPage.filter.byProject('openshift-cluster-observability-operator'); listPersesDashboardsPage.filter.byName( persesDashboardsDashboardDropdownCOO.K8S_COMPUTE_RESOURCES_CLUSTER[0], ); + listPersesDashboardsPage.countDashboards('1'); listPersesDashboardsPage.clickKebabIcon();🤖 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/support/perses/99.coo_rbac_perses_user1.cy.ts` around lines 445 - 453, Replace the fixed cy.wait(2000) with a state-based wait that ensures the target dashboard row is present and ready before opening the kebab: after calling listPersesDashboardsPage.filter.byName(...) wait for the specific dashboard entry to exist/be visible (e.g., use the page object's selector for the dashboard row or a new helper like listPersesDashboardsPage.waitForDashboard(name) that uses cy.get(...).should('be.visible') or cy.contains(...).should('exist')) and only then call listPersesDashboardsPage.clickKebabIcon(); this removes the arbitrary sleep and synchronizes the kebab flow to the actual DOM state.web/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.ts (1)
72-74: ⚡ Quick winReplace fixed sleep with a readiness assertion for the Dashboards page heading.
Line 72 adds a hardcoded delay, which can flake under slower conditions and always adds runtime cost. Prefer waiting for the page to be ready by checking for the h1 heading. Use the same pattern already established in the codebase:
Proposed change
nav.sidenav.clickNavLink(['Observe', 'Dashboards']); - cy.wait(2000); nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); + cy.contains('h1', 'Dashboards', { timeout: 10000 }).should('be.visible'); cy.changeNamespace('All Projects');🤖 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/e2e/perses/99.coo_rbac_perses_user4.cy.ts` around lines 72 - 74, Remove the hardcoded cy.wait(2000) and instead wait for the Dashboards page to be ready by asserting the page heading is visible; after nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']) replace the sleep with the established readiness check (e.g., assert the h1 heading contains or is visible for the Dashboards page) so the test waits deterministically before calling cy.changeNamespace('All Projects').
🤖 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/support/commands/dashboards-commands.ts`:
- Around line 34-66: The cy.exec calls that build the oc apply commands
interpolate Cypress.env('KUBECONFIG_PATH') directly which breaks for paths with
spaces or malicious values; update each cy.exec string construction (the oc
apply invocations) to quote the kubeconfig value (e.g. --kubeconfig
"${Cypress.env('KUBECONFIG_PATH')}") or alternatively read const kube =
Cypress.env('KUBECONFIG_PATH') and pass it safely via the cy.exec env option,
ensuring every cy.exec invocation that references Cypress.env('KUBECONFIG_PATH')
is updated.
In `@web/cypress/support/commands/utility-commands.ts`:
- Around line 145-150: After calling cy.changeNamespace(namespace) add a check
that the namespace dropdown or selector has updated to the new namespace value
before asserting row counts so you don't match stale rows; specifically,
immediately after cy.changeNamespace(namespace) validate the namespace control
(the same UI element used for selecting namespaces) reflects the requested
namespace, then proceed to wait for the table rows (e.g., DataViewTable tbody
tr) to ensure the table has reloaded in the new namespace.
In `@web/cypress/views/common.ts`:
- Around line 10-12: The wait callback passed to cy.waitUntil currently contains
an assertion (.should('be.visible')) which throws during retries; change the
callback used in cy.waitUntil to return a boolean by querying the element with
cy.bySemanticElement('h1', title) via Cypress.$(selector) (or get the DOM node
in a .then and use Cypress.dom.isVisible(node)) and return that visibility
boolean, then after cy.waitUntil completes call .scrollIntoView() and assert
.should('be.visible'); keep the existing timeout option.
In `@web/cypress/views/perses-dashboards-edit-variables.ts`:
- Line 132: Replace the brittle positional selector
cy.get('input[type="checkbox"]').eq(2).click() with a stable, scoped selector:
find the modal/container element used in this test (e.g., the edit variables
modal) and target the checkbox by a semantic hook such as a data-testid or by
its associated label text (using cy.contains or aria-label) before calling
.click(); update the test to use that scoped selector (referencing the current
cy.get(...) call and the eq(2) usage) so the intent and checkbox selection
remain stable if DOM order changes.
In `@web/cypress/views/perses-dashboards-list-dashboards.ts`:
- Around line 303-308: The assertion in assertDuplicateDashboardAlreadyExists is
using cy.byPFRole('list') and can match unrelated content; change it to scope to
the active dialog (e.g., target the dialog context with cy.byRole('dialog') or
cy.byPFRole('dialog') and then find the h4). Update the selector in
assertDuplicateDashboardAlreadyExists so the lookup for
persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION is
performed inside the dialog (use .within or .find on the dialog element) and
keep the final .contains(...).should('be.visible') assertion.
---
Nitpick comments:
In `@web/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.ts`:
- Around line 72-74: Remove the hardcoded cy.wait(2000) and instead wait for the
Dashboards page to be ready by asserting the page heading is visible; after
nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']) replace the sleep
with the established readiness check (e.g., assert the h1 heading contains or is
visible for the Dashboards page) so the test waits deterministically before
calling cy.changeNamespace('All Projects').
In `@web/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.ts`:
- Around line 71-73: Replace the brittle fixed delay by waiting for the sidebar
item to be present/visible before clicking: remove cy.wait(2000) in the
beforeEach and instead assert the nav state for the target item (use the
nav.sidenav API such as checking the specific nav item or a visibility/state
assertion for ['Observe','Dashboards'] or ['Observe','Dashboards (Perses)'])
then call nav.sidenav.clickNavLink([...]) once the assertion passes; update the
beforeEach that currently calls
nav.sidenav.clickNavLink(['Observe','Dashboards']); cy.wait(2000);
nav.sidenav.clickNavLink(['Observe','Dashboards (Perses)']) to assert
visibility/state on the sidebar entry and then perform the clicks without any
fixed wait.
In `@web/cypress/support/commands/dashboards-commands.ts`:
- Around line 33-66: In setupDashboardsAndPlugins() and cleanupDashboards(),
remove the repeated hardcoded manifest strings and duplicate delete branches by
extracting the five dashboard filenames
(openshift-cluster-sample-dashboard.yaml, perses-dashboard-sample.yaml,
prometheus-overview-variables.yaml, thanos-compact-overview-1var.yaml,
thanos-querier-datasource.yaml) into a single shared array or helper (e.g.,
DASHBOARD_FIXTURES) and iterate over that array: in setup call cy.exec with `oc
apply -f ./cypress/fixtures/.../<filename> --kubeconfig
${Cypress.env('KUBECONFIG_PATH')}` for each entry, and in cleanup iterate the
same array to run `oc delete -f .../<filename> --kubeconfig ...`; also remove
the duplicated delete branch currently present in cleanupDashboards().
In `@web/cypress/support/perses/01.coo_list_perses_admin.cy.ts`:
- Around line 369-371: Replace the fixed cy.wait(2000) after
persesDashboardsPage.backToListPersesDashboardsPage() with a deterministic "page
ready" assertion: remove the hard delay and instead wait for the list page to be
fully loaded by asserting a stable UI signal (for example, a visible table or
filter input, or an existing page-ready helper like
listPersesDashboardsPage.waitForReady()/ensureLoaded()); then call
listPersesDashboardsPage.filter.byProject('perses-dev') once that assertion
passes to avoid flakiness.
In `@web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts`:
- Around line 445-453: Replace the fixed cy.wait(2000) with a state-based wait
that ensures the target dashboard row is present and ready before opening the
kebab: after calling listPersesDashboardsPage.filter.byName(...) wait for the
specific dashboard entry to exist/be visible (e.g., use the page object's
selector for the dashboard row or a new helper like
listPersesDashboardsPage.waitForDashboard(name) that uses
cy.get(...).should('be.visible') or cy.contains(...).should('exist')) and only
then call listPersesDashboardsPage.clickKebabIcon(); this removes the arbitrary
sleep and synchronizes the kebab flow to the actual DOM state.
In `@web/cypress/views/alerting-rule-list-page.ts`:
- Around line 33-47: Remove the try/catch wrapper around the Cypress command
chain in the byName implementation: delete the try { ... } catch (error) { ... }
block and keep the direct command chain starting with
listPage.filter.selectAttribute('Name') followed by
cy.byOUIAID(`${ouiaId}-input`).find('input').scrollIntoView().as('input').should('be.visible');
cy.get('@input', { timeout: 10000 }).scrollIntoView().type(name + '{enter}');
cy.get('@input', { timeout: 10000 }).scrollIntoView().should('have.attr',
'value', name); so that Cypress can handle failures natively instead of
attempting to catch them.
In `@web/cypress/views/perses-dashboards-create-dashboard.ts`:
- Around line 69-83: The two assertions in persesCreateDashboardsPage
(assertMaxLengthValidation and assertDuplicatedNameValidation) use strict
full-text checks via should('have.text', ...) which is brittle for
backend-originated alerts; change both to use a containment check (e.g.,
should('contain.text', ...)) or assert on a stable fragment of the expected
message so the tests tolerate minor wording/formatting changes while still
verifying the correct validation dialog is shown.
In `@web/cypress/views/perses-dashboards.ts`:
- Around line 170-174: The fixed cy.wait(2000) should be replaced with a
deterministic readiness check: remove cy.wait and instead assert a stable
post-navigation signal such as verifying the dashboard UI has fully loaded (for
example, assert the dashboard title or header contains dashboardName, or wait on
a network alias via cy.intercept and cy.wait('@alias') that represents fetching
the dashboard). Locate the snippet using DataTestIDs.PersesDashboardDropdown and
the input assertion, then replace the static wait with a targeted assertion like
checking the visible dashboard header/text equals dashboardName or waiting on an
XHR/fetch alias that finishes loading the dashboard content.
🪄 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: 6e756565-efc6-4d04-a8f5-cfb4f074cd47
📒 Files selected for processing (30)
web/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/perses/00.coo_bvt_perses_admin.cy.tsweb/cypress/e2e/perses/04.coo_import_perses_admin.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user2.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.tsweb/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.jsonweb/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.yamlweb/cypress/fixtures/perses/constants.tsweb/cypress/support/commands/dashboards-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/monitoring/00.bvt_monitoring.cy.tsweb/cypress/support/monitoring/03.reg_legacy_dashboards.cy.tsweb/cypress/support/monitoring/06.reg_legacy_dashboards_namespace.cy.tsweb/cypress/support/perses/01.coo_list_perses_admin.cy.tsweb/cypress/support/perses/03.coo_create_perses_admin.cy.tsweb/cypress/support/perses/04.coo_import_perses_admin.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/common.tsweb/cypress/views/metrics.tsweb/cypress/views/perses-dashboards-create-dashboard.tsweb/cypress/views/perses-dashboards-edit-variables.tsweb/cypress/views/perses-dashboards-list-dashboards.tsweb/cypress/views/perses-dashboards-panel.tsweb/cypress/views/perses-dashboards.tsweb/src/components/data-test.ts
💤 Files with no reviewable changes (4)
- web/cypress/support/monitoring/03.reg_legacy_dashboards.cy.ts
- web/cypress/views/perses-dashboards-panel.ts
- web/cypress/support/monitoring/06.reg_legacy_dashboards_namespace.cy.ts
- web/cypress/support/perses/04.coo_import_perses_admin.cy.ts
| if (allowAllValue) { | ||
| cy.get('input[name="' + editPersesDashboardsAddVariable.inputAllowAllValue + '"]').click(); | ||
| if (customAllValue !== undefined && customAllValue !== '') { | ||
| cy.get('input[type="checkbox"]').eq(2).click(); |
There was a problem hiding this comment.
Use a stable, scoped selector instead of positional checkbox indexing.
Line 132 uses cy.get('input[type="checkbox"]').eq(2), which is brittle and can click the wrong checkbox when DOM order changes. Scope to the modal and target by semantic label or test id to avoid reintroducing flakiness.
Suggested change
- cy.get('input[type="checkbox"]').eq(2).click();
+ cy.byDataTestID(persesMUIDataTestIDs.editDashboardVariablesModal)
+ .contains('label', 'Custom all value')
+ .find('input[type="checkbox"]')
+ .click();📝 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.
Suggested change
| cy.get('input[type="checkbox"]').eq(2).click(); | |
| cy.byDataTestID(persesMUIDataTestIDs.editDashboardVariablesModal) | |
| .contains('label', 'Custom all value') | |
| .find('input[type="checkbox"]') | |
| .click(); |
🤖 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/perses-dashboards-edit-variables.ts` at line 132, Replace
the brittle positional selector cy.get('input[type="checkbox"]').eq(2).click()
with a stable, scoped selector: find the modal/container element used in this
test (e.g., the edit variables modal) and target the checkbox by a semantic hook
such as a data-testid or by its associated label text (using cy.contains or
aria-label) before calling .click(); update the test to use that scoped selector
(referencing the current cy.get(...) call and the eq(2) usage) so the intent and
checkbox selection remain stable if DOM order changes.
Comment on lines
303
to
308
| assertDuplicateDashboardAlreadyExists: () => { | ||
| cy.log('listPersesDashboardsPage.assertDuplicateDashboardAlreadyExists'); | ||
| cy.byPFRole('dialog') | ||
| .find(Classes.PersesCreateDashboardDashboardNameError) | ||
| cy.byPFRole('list') | ||
| .find('h4') | ||
| .contains(persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION) | ||
| .should('be.visible'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd web && find . -name "perses-dashboards-list-dashboards.ts" -type fRepository: openshift/monitoring-plugin
Length of output: 124
🏁 Script executed:
cd web && wc -l cypress/views/perses-dashboards-list-dashboards.tsRepository: openshift/monitoring-plugin
Length of output: 126
🏁 Script executed:
cd web && sed -n '290,320p' cypress/views/perses-dashboards-list-dashboards.tsRepository: openshift/monitoring-plugin
Length of output: 1240
🏁 Script executed:
cd web && rg 'byPFRole.*dialog' -A 3 -B 1 cypress/Repository: openshift/monitoring-plugin
Length of output: 18885
🏁 Script executed:
cd web && rg 'assertDuplicateDashboardAlreadyExists' -A 10 -B 5 cypress/Repository: openshift/monitoring-plugin
Length of output: 4819
🏁 Script executed:
cd web && rg 'DIALOG_DUPLICATED_NAME_VALIDATION' -B 2 -A 2 cypress/Repository: openshift/monitoring-plugin
Length of output: 999
Scope duplicated-name error assertion to the active dialog.
The selector currently uses cy.byPFRole('list'), which is inconsistent with all other dialog validation assertions in this file (e.g., assertMaxLengthValidation, assertRenameDashboardMaxLength) and can match unrelated list content. This method is called immediately after opening a duplicate dialog and should query the dialog context.
🐛 Suggested fix
assertDuplicateDashboardAlreadyExists: () => {
cy.log('listPersesDashboardsPage.assertDuplicateDashboardAlreadyExists');
- cy.byPFRole('list')
+ cy.byPFRole('dialog')
.find('h4')
.contains(persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION)
.should('be.visible');
},📝 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.
Suggested change
| assertDuplicateDashboardAlreadyExists: () => { | |
| cy.log('listPersesDashboardsPage.assertDuplicateDashboardAlreadyExists'); | |
| cy.byPFRole('dialog') | |
| .find(Classes.PersesCreateDashboardDashboardNameError) | |
| cy.byPFRole('list') | |
| .find('h4') | |
| .contains(persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION) | |
| .should('be.visible'); | |
| assertDuplicateDashboardAlreadyExists: () => { | |
| cy.log('listPersesDashboardsPage.assertDuplicateDashboardAlreadyExists'); | |
| cy.byPFRole('dialog') | |
| .find('h4') | |
| .contains(persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION) | |
| .should('be.visible'); | |
| }, |
🤖 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/perses-dashboards-list-dashboards.ts` around lines 303 -
308, The assertion in assertDuplicateDashboardAlreadyExists is using
cy.byPFRole('list') and can match unrelated content; change it to scope to the
active dialog (e.g., target the dialog context with cy.byRole('dialog') or
cy.byPFRole('dialog') and then find the h4). Update the selector in
assertDuplicateDashboardAlreadyExists so the lookup for
persesDashboardsDuplicateDashboard.DIALOG_DUPLICATED_NAME_VALIDATION is
performed inside the dialog (use .within or .find on the dialog element) and
keep the final .contains(...).should('be.visible') assertion.
Contributor
Author
|
/test e2e-monitoring |
1 similar comment
Contributor
Author
|
/test e2e-monitoring |
etmurasaki
force-pushed
the
etmura-main-fix
branch
from
e7cc169 to
6f2dfa0
Compare
Contributor
Author
|
/test e2e-monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.ts (1)
72-72: ⚡ Quick winReplace fixed sleep with a state-based wait.
Line 72's
cy.wait(2000)introduces unnecessary delays and brittle tests sensitive to cluster/UI variance. Cypress best practices strongly recommend deterministic waits instead: chain assertions like.should('be.visible')or.should('exist')to wait for DOM state, usecy.url().should('include', '/expected-path')for navigation verification, or usecy.intercept()withcy.wait('@alias')if waiting for specific network responses. These approaches let tests proceed as soon as conditions are met rather than always consuming the full duration.🤖 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/e2e/perses/99.coo_rbac_perses_user6.cy.ts` at line 72, Replace the fixed cy.wait(2000) with a deterministic state-based wait: remove the cy.wait call and instead wait for the UI or network condition that indicates readiness (for example, assert the target element becomes visible via cy.get('<selector>').should('be.visible'), assert navigation via cy.url().should('include', '/expected-path'), or use cy.intercept(...) and cy.wait('`@alias`') to wait for the backend response); locate the explicit cy.wait(2000) in the perses test (file 99.coo_rbac_perses_user6.cy.ts) and replace it with the appropriate .should(...) or cy.wait('`@alias`') that matches the action immediately following that pause.web/cypress/e2e/monitoring/00.bvt_admin.cy.ts (1)
18-20: ⚡ Quick winReplace double-click workaround with explicit route/state assertion.
Lines 18–20 and 41–42 use repeated clicks because the underlying
clickNavLinkhelper relies on a hardcodedcy.wait(2000), which is brittle and insufficient. Rather than doubling clicks to mask the timing issue, refactor the nav helper to gate on actual state readiness: usecy.intercept()to await the navigation request, assert URL changes withcy.location('pathname'), or confirm page-specific elements (e.g., title text) are visible before returning. This eliminates flakiness and aligns with Cypress best practices.🤖 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/e2e/monitoring/00.bvt_admin.cy.ts` around lines 18 - 20, Replace the double-click workaround by updating the nav helper and tests so navigation is gated on real readiness: remove the duplicate calls to nav.sidenav.clickNavLink(['Observe', 'Metrics']) in the test and modify clickNavLink to wait for the actual navigation to complete (e.g., register a cy.intercept() for the route, assert cy.location('pathname') changed, or wait for a page-specific element like the Metrics title to be visible) before returning; update any calls (e.g., nav.sidenav.clickNavLink) to rely on this readiness check instead of hardcoded cy.wait(2000).
🤖 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/views/silence-alert-page.ts`:
- Line 391: The test assertion uses a brittle, fully qualified error string
('Danger alert:invalid silence: matcher set 0: at least one matcher must not
match the empty string'); change the assertion to check only the stable semantic
substring "at least one matcher must not match the empty string" instead (e.g.,
use a contains/should('contain', ...) or string.includes assertion) so the test
anchors on the meaningful message rather than the exact prefix; update the
assertion where that long literal appears to use the shorter substring.
---
Nitpick comments:
In `@web/cypress/e2e/monitoring/00.bvt_admin.cy.ts`:
- Around line 18-20: Replace the double-click workaround by updating the nav
helper and tests so navigation is gated on real readiness: remove the duplicate
calls to nav.sidenav.clickNavLink(['Observe', 'Metrics']) in the test and modify
clickNavLink to wait for the actual navigation to complete (e.g., register a
cy.intercept() for the route, assert cy.location('pathname') changed, or wait
for a page-specific element like the Metrics title to be visible) before
returning; update any calls (e.g., nav.sidenav.clickNavLink) to rely on this
readiness check instead of hardcoded cy.wait(2000).
In `@web/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.ts`:
- Line 72: Replace the fixed cy.wait(2000) with a deterministic state-based
wait: remove the cy.wait call and instead wait for the UI or network condition
that indicates readiness (for example, assert the target element becomes visible
via cy.get('<selector>').should('be.visible'), assert navigation via
cy.url().should('include', '/expected-path'), or use cy.intercept(...) and
cy.wait('`@alias`') to wait for the backend response); locate the explicit
cy.wait(2000) in the perses test (file 99.coo_rbac_perses_user6.cy.ts) and
replace it with the appropriate .should(...) or cy.wait('`@alias`') that matches
the action immediately following that pause.
🪄 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: 565045bb-3dc7-4be2-b428-3264eba166a2
📒 Files selected for processing (31)
web/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/perses/00.coo_bvt_perses_admin.cy.tsweb/cypress/e2e/perses/04.coo_import_perses_admin.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user2.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.tsweb/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.jsonweb/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.yamlweb/cypress/fixtures/perses/constants.tsweb/cypress/support/commands/dashboards-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/monitoring/00.bvt_monitoring.cy.tsweb/cypress/support/monitoring/03.reg_legacy_dashboards.cy.tsweb/cypress/support/monitoring/06.reg_legacy_dashboards_namespace.cy.tsweb/cypress/support/perses/01.coo_list_perses_admin.cy.tsweb/cypress/support/perses/03.coo_create_perses_admin.cy.tsweb/cypress/support/perses/04.coo_import_perses_admin.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/common.tsweb/cypress/views/metrics.tsweb/cypress/views/perses-dashboards-create-dashboard.tsweb/cypress/views/perses-dashboards-edit-variables.tsweb/cypress/views/perses-dashboards-list-dashboards.tsweb/cypress/views/perses-dashboards-panel.tsweb/cypress/views/perses-dashboards.tsweb/cypress/views/silence-alert-page.tsweb/src/components/data-test.ts
💤 Files with no reviewable changes (4)
- web/cypress/views/perses-dashboards-panel.ts
- web/cypress/support/monitoring/03.reg_legacy_dashboards.cy.ts
- web/cypress/support/perses/04.coo_import_perses_admin.cy.ts
- web/cypress/support/monitoring/06.reg_legacy_dashboards_namespace.cy.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.json
🚧 Files skipped from review as they are similar to previous changes (19)
- web/cypress/e2e/perses/04.coo_import_perses_admin.cy.ts
- web/cypress/support/perses/03.coo_create_perses_admin.cy.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user3.cy.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user2.cy.ts
- web/cypress/views/perses-dashboards.ts
- web/cypress/views/perses-dashboards-edit-variables.ts
- web/cypress/support/monitoring/00.bvt_monitoring.cy.ts
- web/cypress/views/alerting-rule-list-page.ts
- web/cypress/support/perses/01.coo_list_perses_admin.cy.ts
- web/cypress/views/perses-dashboards-list-dashboards.ts
- web/cypress/views/common.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user4.cy.ts
- web/cypress/e2e/perses/00.coo_bvt_perses_admin.cy.ts
- web/cypress/views/perses-dashboards-create-dashboard.ts
- web/cypress/fixtures/perses/constants.ts
- web/src/components/data-test.ts
- web/cypress/fixtures/coo/coo140_perses/import/testing-perses-dashboard.yaml
- web/cypress/support/commands/utility-commands.ts
- web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts
Contributor
|
@etmurasaki: 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. |
Contributor
Author
|
/test e2e-monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/support/commands/coo-install-commands.ts`:
- Around line 112-114: The readiness wait cy.exec calls use the kubeconfig
variable unquoted which breaks when the KUBECONFIG_PATH contains spaces; update
all occurrences that call cy.exec with --kubeconfig ${kubeconfig} (e.g., the
cy.exec lines around the kubeconfig const and other instances at the noted
locations) to wrap the kubeconfig value in quotes (--kubeconfig "${kubeconfig}")
so the shell receives the path as a single argument; ensure you update every
similar cy.exec invocation in this file (including the ones near lines 120-121,
133-134, 143-144, 299-300, 311-312) to use the quoted form.
🪄 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: 02f34f6a-b5e8-4f85-a158-08fe7113dc53
📒 Files selected for processing (13)
web/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/coo-install-commands.tsweb/cypress/support/commands/dashboards-commands.tsweb/cypress/support/commands/operator-commands.tsweb/cypress/support/commands/traces-logging-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/commands/virtualization-commands.tsweb/cypress/support/monitoring/02.reg_metrics_1.cy.tsweb/cypress/support/monitoring/02.reg_metrics_2.cy.tsweb/cypress/support/monitoring/05.reg_metrics_namespace_2.cy.tsweb/cypress/views/common.tsweb/cypress/views/metrics.tsweb/cypress/views/silence-alert-page.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/support/monitoring/02.reg_metrics_1.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/cypress/support/commands/utility-commands.ts
Comment on lines
+112
to
114
| const kubeconfig = Cypress.env('KUBECONFIG_PATH') as string; | ||
|
|
||
| cy.exec(`oc project ${MCP.namespace} --kubeconfig ${kubeconfig}`); |
There was a problem hiding this comment.
Quote kubeconfig in readiness wait commands as well.
Install/cleanup paths were hardened, but these wait paths still pass --kubeconfig unquoted. If the path contains spaces, waits can fail and make stabilization flaky.
Suggested patch
- cy.exec(`oc project ${MCP.namespace} --kubeconfig ${kubeconfig}`);
+ cy.exec(`oc project ${MCP.namespace} --kubeconfig "${kubeconfig}"`);
- `oc get pods -n ${MCP.namespace} -o name --kubeconfig ${kubeconfig} ` +
+ `oc get pods -n ${MCP.namespace} -o name --kubeconfig "${kubeconfig}" ` +
'| grep observability-operator | grep -v bundle',
- `oc get pods -n ${MCP.namespace} -o name --kubeconfig ${kubeconfig} ` +
+ `oc get pods -n ${MCP.namespace} -o name --kubeconfig "${kubeconfig}" ` +
'| grep observability-operator | grep -v bundle',
- `--timeout=120s --kubeconfig ${kubeconfig}`,
+ `--timeout=120s --kubeconfig "${kubeconfig}"`,
- cy.exec(`oc get pods -n ${namespace} --kubeconfig ${kubeconfigPath} -o name`, {
+ cy.exec(`oc get pods -n ${namespace} --kubeconfig "${kubeconfigPath}" -o name`, {
failOnNonZeroExit: false,
}).then((result) => {Also applies to: 120-121, 133-134, 143-144, 299-300, 311-312
🧰 Tools
🪛 OpenGrep (1.21.0)
[ERROR] 114-114: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/support/commands/coo-install-commands.ts` around lines 112 - 114,
The readiness wait cy.exec calls use the kubeconfig variable unquoted which
breaks when the KUBECONFIG_PATH contains spaces; update all occurrences that
call cy.exec with --kubeconfig ${kubeconfig} (e.g., the cy.exec lines around the
kubeconfig const and other instances at the noted locations) to wrap the
kubeconfig value in quotes (--kubeconfig "${kubeconfig}") so the shell receives
the path as a single argument; ensure you update every similar cy.exec
invocation in this file (including the ones near lines 120-121, 133-134,
143-144, 299-300, 311-312) to use the quoted form.
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
Tests
Bug Fixes
Chores