NO-JIRA: perses and monitoring automation testing - stabilization#924
NO-JIRA: perses and monitoring automation testing - stabilization#924etmurasaki wants to merge 1 commit 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
|
Skipping CI for Draft Pull Request. |
|
[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
|
/test e2e-monitoring |
|
/test e2e-perses |
|
/test all |
WalkthroughStabilizes Perses/dashboard E2E tests and monitoring specs: adds cleanup and namespace steps, replaces brittle waits with state-based sync, updates fixtures/constants and data-test IDs, and refactors page-object selectors and support commands. ChangesPerses Dashboard Test Stabilization and Fixture Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `openshift-cluster-sample-dashboard.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
|
|
||
| cy.log('Create perses-dashboard-sample instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `perses-dashboard-sample.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| cy.log('Create perses-dashboard-sample instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `perses-dashboard-sample.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
|
|
||
| cy.log('Create prometheus-overview-variables instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `prometheus-overview-variables.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| cy.log('Create prometheus-overview-variables instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `prometheus-overview-variables.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
|
|
||
| cy.log('Create thanos-compact-overview-1var instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `thanos-compact-overview-1var.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| cy.log('Create thanos-compact-overview-1var instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `thanos-compact-overview-1var.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
|
|
||
| cy.log('Create Thanos Querier instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `thanos-querier-datasource.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| } | ||
| cy.log('Create Thanos Querier instance.'); | ||
| cy.exec( | ||
| `oc apply -f ./cypress/fixtures/coo/coo140_perses/dashboards/` + | ||
| `thanos-querier-datasource.yaml ` + | ||
| `--kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to see the actual code
head -100 web/cypress/support/commands/dashboards-commands.ts | tail -n +1 | cat -nRepository: openshift/monitoring-plugin
Length of output: 4301
🏁 Script executed:
# Let's get the full context around the lines in question
sed -n '1,100p' web/cypress/support/commands/dashboards-commands.ts | cat -nRepository: openshift/monitoring-plugin
Length of output: 4301
🏁 Script executed:
# Search for all uses of KUBECONFIG_PATH in the codebase
rg 'KUBECONFIG_PATH' --type ts --type js -nRepository: openshift/monitoring-plugin
Length of output: 11844
🏁 Script executed:
# Search for all cy.exec calls to understand the pattern
rg 'cy\.exec\(' web/cypress --type ts --type js -A 3 | head -100Repository: openshift/monitoring-plugin
Length of output: 8006
🏁 Script executed:
# Look for any helper functions that might be handling environment variables safely
rg 'function|const.*=.*\(' web/cypress/support/commands/dashboards-commands.ts -n | cat -nRepository: openshift/monitoring-plugin
Length of output: 267
Quote the KUBECONFIG_PATH environment variable before passing it to cy.exec.
The five oc apply calls at lines 34–66 interpolate Cypress.env('KUBECONFIG_PATH') unquoted into shell commands. A path with spaces will cause the command to fail, and a tainted env value can inject shell tokens. Wrap the value in quotes: --kubeconfig "${Cypress.env('KUBECONFIG_PATH')}", or extract it to a variable and pass via the env option instead.
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 34-38: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 41-45: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 48-52: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 55-59: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 62-66: 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/dashboards-commands.ts` around lines 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.
| cy.changeNamespace(namespace); | ||
| // Wait for the pod table to load after namespace change so the page stabilizes | ||
| cy.get('table tbody tr, [data-ouia-component-id="DataViewTable"] tbody tr', { | ||
| timeout: 30000, | ||
| }).should('have.length.greaterThan', 0); | ||
| // Re-check for DataViewFilters after the table has stabilized |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find the file
find . -name "utility-commands.ts" -type fRepository: openshift/monitoring-plugin
Length of output: 122
🏁 Script executed:
# Read the specific lines mentioned in the review
cat -n web/cypress/support/commands/utility-commands.ts | sed -n '140,160p'Repository: openshift/monitoring-plugin
Length of output: 1274
🏁 Script executed:
# Find the changeNamespace command implementation
rg -A 20 "addCommand.*changeNamespace|\.changeNamespace\s*=" web/cypress/support/commands/ -A 15Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Search for how namespace changes are handled elsewhere in the codebase
rg "cy\.changeNamespace" web/cypress -B 3 -A 5Repository: openshift/monitoring-plugin
Length of output: 50383
🏁 Script executed:
# Search for changeNamespace command definition
rg "changeNamespace|\.add\(" web/cypress/support/commands/ -A 8 | head -100Repository: openshift/monitoring-plugin
Length of output: 8804
🏁 Script executed:
# Search more specifically for the changeNamespace implementation
rg "changeNamespace" web/cypress/support/commands/utility-commands.ts -B 2 -A 15Repository: openshift/monitoring-plugin
Length of output: 2356
🏁 Script executed:
# Let's look at the entire utility-commands file to understand the structure
wc -l web/cypress/support/commands/utility-commands.tsRepository: openshift/monitoring-plugin
Length of output: 124
🏁 Script executed:
# Get the complete changeNamespace command definition
sed -n '67,120p' web/cypress/support/commands/utility-commands.tsRepository: openshift/monitoring-plugin
Length of output: 2190
🏁 Script executed:
# Let's get the complete changeNamespace implementation from the start
sed -n '60,105p' web/cypress/support/commands/utility-commands.tsRepository: openshift/monitoring-plugin
Length of output: 1847
🏁 Script executed:
# Check if there are any wait patterns after changeNamespace in other tests
rg "changeNamespace" web/cypress/support -A 3 | grep -E "(cy\.|should|wait)" | head -30Repository: openshift/monitoring-plugin
Length of output: 3197
🏁 Script executed:
# Let's check if there are any race condition issues documented or similar patterns
rg "stale|race|synchron" web/cypress/support -i | head -20Repository: openshift/monitoring-plugin
Length of output: 2387
Add namespace selector validation before checking table row count to prevent matching stale rows.
At lines 145–150, the row count check can pass against stale rows from the previous namespace because cy.changeNamespace() completes its click action asynchronously without guaranteeing the table data has reloaded. Insert a namespace dropdown validation immediately after cy.changeNamespace(namespace) to ensure the UI has processed the namespace change before asserting row presence:
Suggested fix
cy.changeNamespace(namespace);
+ cy.get('body').then(($body) => {
+ const hasNamespaceBarDropdown =
+ $body.find('[data-test-id="' + LegacyTestIDs.NamespaceBarDropdown + '"]').length > 0;
+ if (hasNamespaceBarDropdown) {
+ cy.byLegacyTestID(LegacyTestIDs.NamespaceBarDropdown).should('contain.text', namespace);
+ } else {
+ cy.get(Classes.NamespaceDropdown).should('contain.text', namespace);
+ }
+ });
// Wait for the pod table to load after namespace change so the page stabilizes
cy.get('table tbody tr, [data-ouia-component-id="DataViewTable"] tbody tr', {
timeout: 30000,
}).should('have.length.greaterThan', 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.changeNamespace(namespace); | |
| // Wait for the pod table to load after namespace change so the page stabilizes | |
| cy.get('table tbody tr, [data-ouia-component-id="DataViewTable"] tbody tr', { | |
| timeout: 30000, | |
| }).should('have.length.greaterThan', 0); | |
| // Re-check for DataViewFilters after the table has stabilized | |
| cy.changeNamespace(namespace); | |
| cy.get('body').then(($body) => { | |
| const hasNamespaceBarDropdown = | |
| $body.find('[data-test-id="' + LegacyTestIDs.NamespaceBarDropdown + '"]').length > 0; | |
| if (hasNamespaceBarDropdown) { | |
| cy.byLegacyTestID(LegacyTestIDs.NamespaceBarDropdown).should('contain.text', namespace); | |
| } else { | |
| cy.get(Classes.NamespaceDropdown).should('contain.text', namespace); | |
| } | |
| }); | |
| // Wait for the pod table to load after namespace change so the page stabilizes | |
| cy.get('table tbody tr, [data-ouia-component-id="DataViewTable"] tbody tr', { | |
| timeout: 30000, | |
| }).should('have.length.greaterThan', 0); | |
| // Re-check for DataViewFilters after the table has stabilized |
🤖 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/utility-commands.ts` around lines 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.
| cy.waitUntil(() => cy.bySemanticElement('h1', title).scrollIntoView().should('be.visible'), { | ||
| timeout: 60000, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In cypress-wait-until, is it recommended to use .should(...)assertions inside thewaitUntil check function, or should the check function return a boolean/truthy value instead?
💡 Result:
It is not recommended to use.should(...) assertions inside the cypress-wait-until waitUntil check function. The check function must return a boolean/truthy value when the wait condition is met; assertions inside it will cause the test to fail immediately if they don't pass during retries, preventing proper waiting [1][2][3]. Instead, manually check conditions (e.g., Cypress.$('#id').length > 0 instead of cy.get('#id').should('exist')) and return truthy/falsy [1][3]. Assertions should be chained after waitUntil, as the truthy return value becomes the subject for subsequent commands [2][3].
Citations:
- 1: https://www.npmjs.com/package/cypress-wait-until
- 2: https://github.com/NoriSte/cypress-wait-until
- 3: https://github.com/NoriSte/cypress-wait-until/blob/master/README.md
Remove the .should() assertion from the cy.waitUntil check callback; it should return a boolean instead.
The check function passed to cy.waitUntil must return a boolean/truthy value when the condition is met. Assertions like .should() inside the callback will cause the test to fail immediately during retries, preventing the 60s timeout from being honored and defeating the purpose of waiting. Extract the visibility check into a manual boolean check (e.g., using Cypress.$() and Cypress.dom.isVisible()), then chain assertions after the waitUntil call.
Suggested fix
- cy.waitUntil(() => cy.bySemanticElement('h1', title).scrollIntoView().should('be.visible'), {
- timeout: 60000,
- });
+ cy.waitUntil(
+ () =>
+ Cypress.$('h1')
+ .toArray()
+ .some((el) => el.textContent?.trim() === title && Cypress.dom.isVisible(el)),
+ { timeout: 60000 },
+ );
+ cy.bySemanticElement('h1', title).scrollIntoView().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.
| cy.waitUntil(() => cy.bySemanticElement('h1', title).scrollIntoView().should('be.visible'), { | |
| timeout: 60000, | |
| }); | |
| cy.waitUntil( | |
| () => | |
| Cypress.$('h1') | |
| .toArray() | |
| .some((el) => el.textContent?.trim() === title && Cypress.dom.isVisible(el)), | |
| { timeout: 60000 }, | |
| ); | |
| cy.bySemanticElement('h1', title).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/common.ts` around lines 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.
| 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.
| 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.
| 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.
| 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.
|
/test e2e-monitoring |
1 similar comment
|
/test e2e-monitoring |
etmurasaki
force-pushed
the
etmura-main-fix
branch
from
e7cc169 to
6f2dfa0
Compare
|
/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
| cy.get(Classes.SilenceAlertTitle).should( | ||
| 'contain.text', | ||
| 'invalid silence: at least one matcher must not match the empty string', | ||
| 'Danger alert:invalid silence: matcher set 0: at least one matcher must not match the empty string', |
There was a problem hiding this comment.
Assertion is overly specific and likely brittle.
Line 391 matches a long, fully qualified message (Danger alert:... matcher set 0 ...). This is likely to fail on harmless wording/prefix changes. Assert on the stable semantic substring (e.g., at least one matcher must not match the empty string) instead.
🤖 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/silence-alert-page.ts` at 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.
|
@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. |
2 participants
Summary by CodeRabbit
Tests
Chores