NO-JIRA: perses automation testing fixes after project selector fixes#833
Open
etmurasaki wants to merge 1 commit intoopenshift:mainfrom
Open
NO-JIRA: perses automation testing fixes after project selector fixes#833etmurasaki wants to merge 1 commit intoopenshift:mainfrom
etmurasaki wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
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
📝 WalkthroughWalkthroughCypress tests and page objects were updated: preliminary navigation steps added in multiple RBAC tests, dialog scoping applied for dropdown/error selectors, validation messages and fixture tags adjusted, a few support test sequences and selectors were reordered or corrected, and one page-object method signature was simplified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.ts (1)
66-67: Extract the two-step Perses navigation into one helper.This exact sequence now appears across all
99.coo_rbac_perses_user{1..6}.cy.tsspecs. Wrapping it in something likenav.sidenav.openPersesDashboards()would keep future route fixes in one place.♻️ Suggested cleanup
- nav.sidenav.clickNavLink(['Observe', 'Dashboards']); - nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); + nav.sidenav.openPersesDashboards();// web/cypress/views/nav.ts openPersesDashboards: () => { nav.sidenav.clickNavLink(['Observe', 'Dashboards']); nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.ts` around lines 66 - 67, Extract the repeated two-step navigation into a single helper method on the nav view: create a function named openPersesDashboards (e.g., nav.sidenav.openPersesDashboards) that calls nav.sidenav.clickNavLink(['Observe', 'Dashboards']) then nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']), and replace the two-line sequences in all 99.coo_rbac_perses_user{1..6}.cy.ts specs with a single call to nav.sidenav.openPersesDashboards() so future route changes are centralized.web/cypress/views/perses-dashboards-create-dashboard.ts (1)
65-69: Drop the new fixed pause before clicking Cancel.The button is already gated by
should('be.visible'), so the extracy.wait(2000)only adds suite latency and still leaves the interaction time-based instead of state-based.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/perses-dashboards-create-dashboard.ts` around lines 65 - 69, In createDashboardDialogCancelButton remove the pre-click fixed pause (the cy.wait(2000) immediately before the .byPFRole('dialog')...click call) and rely on the existing should('be.visible') assertion for a state-based interaction; leave the rest of the method unchanged so the click uses the visibility guard instead of a time-based wait.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts`:
- Around line 479-494: The test is filtering/deleting using a hard-coded prefix
('Testing Dashboard - UP') which can match multiple dashboards; change the flow
to use the exact generated dashboard name stored in suite scope (e.g. a variable
created when the dashboard is made) and pass that variable to
listPersesDashboardsPage.filter.byName(generatedName) and to the delete action;
instead of listPersesDashboardsPage.clickKebabIcon() use a row-scoped action
(e.g. clickKebabForDashboard(generatedName) or locate the row by name before
clicking) so the kebab targets the exact row, and update countDashboards
assertions to expect '1'/'0' against that exact name. Ensure the generatedName
is accessible where the delete steps run (store in outer describe or
create+delete in the same it).
In `@web/cypress/views/perses-dashboards-import-dashboard.ts`:
- Around line 101-108: The assertions assertFailedToMigrateGrafanaDashboard and
assertDuplicatedDashboardError currently use cy.get('h4') which is too broad;
scope them to the alert component instead (e.g., use cy.get('[role="alert"]') or
your alert class) and then check the heading text inside that alert (for
example, cy.get('[role="alert"]').contains('h4',
persesDashboardsImportDashboard.DIALOG_FAILED_TO_MIGRATE_GRAFANA_DASHBOARD').should('be.visible')
and similarly for DIALOG_DUPLICATED_DASHBOARD_ERROR) so the match is constrained
to the alert component.
In `@web/src/components/data-test.ts`:
- Line 221: The selector PersesCreateDashboardProjectDropdown uses an exact
class-match string which is brittle; replace it with a standard class selector
(e.g., button.pf-v6-c-menu-toggle__button or just .pf-v6-c-menu-toggle__button)
wherever PersesCreateDashboardProjectDropdown is defined/used so it tolerates
additional classes and order changes—update the definition in
web/src/components/data-test.ts and replace all 13 usages across the Cypress
helpers (create, duplicate, import) to reference the new class-style selector.
---
Nitpick comments:
In `@web/cypress/e2e/perses/99.coo_rbac_perses_user1.cy.ts`:
- Around line 66-67: Extract the repeated two-step navigation into a single
helper method on the nav view: create a function named openPersesDashboards
(e.g., nav.sidenav.openPersesDashboards) that calls
nav.sidenav.clickNavLink(['Observe', 'Dashboards']) then
nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']), and replace the
two-line sequences in all 99.coo_rbac_perses_user{1..6}.cy.ts specs with a
single call to nav.sidenav.openPersesDashboards() so future route changes are
centralized.
In `@web/cypress/views/perses-dashboards-create-dashboard.ts`:
- Around line 65-69: In createDashboardDialogCancelButton remove the pre-click
fixed pause (the cy.wait(2000) immediately before the
.byPFRole('dialog')...click call) and rely on the existing should('be.visible')
assertion for a state-based interaction; leave the rest of the method unchanged
so the click uses the visibility guard instead of a time-based wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f57fe68d-cda1-40c6-a1d1-6480b2a0fdc9
📒 Files selected for processing (16)
web/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/coo141_perses/import/acm-vm-status.jsonweb/cypress/fixtures/perses/constants.tsweb/cypress/support/perses/00.coo_bvt_perses_admin_1.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/views/perses-dashboards-create-dashboard.tsweb/cypress/views/perses-dashboards-import-dashboard.tsweb/cypress/views/perses-dashboards-list-dashboards.tsweb/src/components/data-test.ts
jgbernalp
reviewed
Mar 12, 2026
| }); | ||
|
|
||
| beforeEach(() => { | ||
| nav.sidenav.clickNavLink(['Observe', 'Dashboards']); |
Contributor
There was a problem hiding this comment.
Not sure why we need to click on legacy dashboards to test perses. Is it some reset step missing?
jgbernalp
reviewed
Mar 12, 2026
| cy.log(`5.5. Click on the Kebab icon - Delete`); | ||
| nav.sidenav.clickNavLink(['Observe', 'Alerting']); | ||
| nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']);s | ||
| nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); |
Contributor
There was a problem hiding this comment.
Are the testing files being excluded from typescript and lint checks? This should have been caught by the ts compiler.
etmurasaki
force-pushed
the
etmura-perses-fix
branch
from
337dbdc to
b0f33ff
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts (1)
479-494:⚠️ Potential issue | 🟠 MajorUse the exact generated dashboard name for delete cleanup (not a shared prefix).
Line 480 and Line 493 still filter by
'Testing Dashboard - UP', which can match multiple leftover dashboards and make delete/counter assertions flaky.🛠️ Suggested fix
+let createdDashboardName = ''; it(`5.${perspective.name} perspective - Create Dashboard with panel groups, panels and variables`, () => { - let dashboardName = 'Testing Dashboard - UP '; - let randomSuffix = Math.random().toString(5); - dashboardName += randomSuffix; + createdDashboardName = `Testing Dashboard - UP ${Math.random().toString(36).slice(2, 10)}`; @@ - persesCreateDashboardsPage.enterDashboardName(dashboardName); + persesCreateDashboardsPage.enterDashboardName(createdDashboardName); @@ }); it(`9.${perspective.name} perspective - Delete dashboard`, () => { + expect(createdDashboardName, 'created dashboard name').to.not.equal(''); @@ - listPersesDashboardsPage.filter.byName('Testing Dashboard - UP'); + listPersesDashboardsPage.filter.byName(createdDashboardName); @@ - listPersesDashboardsPage.filter.byName('Testing Dashboard - UP'); + listPersesDashboardsPage.filter.byName(createdDashboardName); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts` around lines 479 - 494, The test is filtering and deleting using the shared prefix 'Testing Dashboard - UP', which can match multiple dashboards and make assertions flaky; update both calls to listPersesDashboardsPage.filter.byName(...) (lines filtering before delete and final verify) to use the exact generated dashboard name variable used when the dashboard was created (e.g., createdDashboardName or dashboardName) instead of the hardcoded prefix, ensuring listPersesDashboardsPage.countDashboards(...) and subsequent delete steps (clickKebabIcon, clickDeleteOption, deleteDashboardDeleteButton, emptyState) operate on the single intended dashboard.
🧹 Nitpick comments (2)
web/cypress/support/perses/01.coo_list_perses_admin_namespace.cy.ts (1)
73-76: Extract repeated “return to Dashboards (Perses)” navigation into one helper.The same two-step nav sequence is duplicated in two places. A small local helper will reduce drift and keep future fixes in one place.
♻️ Suggested refactor
export function testCOOListPersesNamespace(perspective: PerspectiveConfig) { + const returnToPersesDashboards = () => { + nav.sidenav.clickNavLink(['Observe', 'Alerting']); + nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); + listPersesDashboardsPage.shouldBeLoaded(); + }; it(`1.${perspective.name} perspective - List Dashboards (Perses) page`, () => { @@ - nav.sidenav.clickNavLink(['Observe', 'Alerting']); - nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); + returnToPersesDashboards(); cy.log(`1.11. Click on a dashboard`); @@ it(`2.${perspective.name} perspective - Duplicate from a project to another, Rename and Delete`, () => { @@ - nav.sidenav.clickNavLink(['Observe', 'Alerting']); - nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']); + returnToPersesDashboards();Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/perses/01.coo_list_perses_admin_namespace.cy.ts` around lines 73 - 76, The duplicated two-step navigation using nav.sidenav.clickNavLink(['Observe', 'Alerting']) followed by nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']) should be extracted into a small helper (e.g., returnToDashboardsPerses or goToDashboardsPerses) and called from both places where it repeats; update the calls that currently perform the two-step sequence (the lines invoking nav.sidenav.clickNavLink with ['Observe','Alerting'] and ['Observe','Dashboards (Perses)']) to a single call to the new helper to centralize the behavior and reduce duplication.web/cypress/views/perses-dashboards-create-dashboard.ts (1)
67-69: Replace hard-coded waits with explicit UI state assertions.Lines 67 and 69 use arbitrary 2-second delays that slow tests and mask synchronization issues. The pre-click wait (line 67) is redundant since the cancel button already checks
.should('be.visible')before clicking. Replace both waits with assertions that verify the dialog closes or expected state changes—for example, wait for the dialog to disappear or confirm that submit/cancel actions complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/perses-dashboards-create-dashboard.ts` around lines 67 - 69, Replace the two hard-coded cy.wait(2000) calls by relying on explicit UI assertions: keep the existing cy.byPFRole('dialog').find('button').contains('Cancel').should('be.visible').click({ force: true }) but remove the pre-click wait, and after the click assert the dialog has closed (e.g., cy.byPFRole('dialog').should('not.exist') or should('not.be.visible')) or assert the expected post-cancel state (a specific element appears/disappears or URL changes) so the test synchronizes on UI state instead of timeouts; update the code referencing cy.byPFRole('dialog') and the Cancel button accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/cypress/support/perses/99.coo_rbac_perses_user1.cy.ts`:
- Around line 479-494: The test is filtering and deleting using the shared
prefix 'Testing Dashboard - UP', which can match multiple dashboards and make
assertions flaky; update both calls to
listPersesDashboardsPage.filter.byName(...) (lines filtering before delete and
final verify) to use the exact generated dashboard name variable used when the
dashboard was created (e.g., createdDashboardName or dashboardName) instead of
the hardcoded prefix, ensuring listPersesDashboardsPage.countDashboards(...) and
subsequent delete steps (clickKebabIcon, clickDeleteOption,
deleteDashboardDeleteButton, emptyState) operate on the single intended
dashboard.
---
Nitpick comments:
In `@web/cypress/support/perses/01.coo_list_perses_admin_namespace.cy.ts`:
- Around line 73-76: The duplicated two-step navigation using
nav.sidenav.clickNavLink(['Observe', 'Alerting']) followed by
nav.sidenav.clickNavLink(['Observe', 'Dashboards (Perses)']) should be extracted
into a small helper (e.g., returnToDashboardsPerses or goToDashboardsPerses) and
called from both places where it repeats; update the calls that currently
perform the two-step sequence (the lines invoking nav.sidenav.clickNavLink with
['Observe','Alerting'] and ['Observe','Dashboards (Perses)']) to a single call
to the new helper to centralize the behavior and reduce duplication.
In `@web/cypress/views/perses-dashboards-create-dashboard.ts`:
- Around line 67-69: Replace the two hard-coded cy.wait(2000) calls by relying
on explicit UI assertions: keep the existing
cy.byPFRole('dialog').find('button').contains('Cancel').should('be.visible').click({
force: true }) but remove the pre-click wait, and after the click assert the
dialog has closed (e.g., cy.byPFRole('dialog').should('not.exist') or
should('not.be.visible')) or assert the expected post-cancel state (a specific
element appears/disappears or URL changes) so the test synchronizes on UI state
instead of timeouts; update the code referencing cy.byPFRole('dialog') and the
Cancel button accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65c31440-367e-4770-b822-aad7914f4997
📒 Files selected for processing (18)
web/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/coo141_perses/import/acm-vm-status.jsonweb/cypress/fixtures/perses/constants.tsweb/cypress/support/perses/00.coo_bvt_perses_admin_1.cy.tsweb/cypress/support/perses/01.coo_list_perses_admin_namespace.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user1.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user3.cy.tsweb/cypress/support/perses/99.coo_rbac_perses_user5.cy.tsweb/cypress/views/perses-dashboards-create-dashboard.tsweb/cypress/views/perses-dashboards-import-dashboard.tsweb/cypress/views/perses-dashboards-list-dashboards.tsweb/cypress/views/perses-dashboards.tsweb/src/components/data-test.ts
💤 Files with no reviewable changes (1)
- web/cypress/views/perses-dashboards.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- web/cypress/views/perses-dashboards-list-dashboards.ts
- web/cypress/views/perses-dashboards-import-dashboard.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user6.cy.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user3.cy.ts
- web/cypress/support/perses/99.coo_rbac_perses_user5.cy.ts
- web/cypress/support/perses/00.coo_bvt_perses_admin_1.cy.ts
- web/cypress/e2e/perses/99.coo_rbac_perses_user2.cy.ts
Contributor
|
@etmurasaki: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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