OU-1107: ACM alerting UI with alerts and perses - #1105
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@etmurasaki: This pull request references OU-1107 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
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
WalkthroughFleet Management and Core Platform Cypress alerting suites now use perspective-specific regression runners and alert targets. Monitoring fixtures enable additional components, while navigation, filtering, empty-state, and hidden-feature assertions are updated for the revised UI behavior. ChangesAlerting regression coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ 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: 1
🧹 Nitpick comments (1)
web/cypress/support/monitoring/01.reg_alerts.cy.ts (1)
43-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate "Alerting Rules page - Filtering" test between Fleet Management and Core Platform.
testAlertsFleetManagementRegression's seconditblock (lines 73-112) andtestAlertsCorePlatformHeaderRegression's seconditblock (lines 142-182) are identical apart from one extralistPage.filter.removeIndividualTag(Source.PLATFORM);call (line 146). Extracting the shared filtering flow into a single helper (parameterized by the one extra teardown step) would prevent the two copies from drifting apart over time.♻️ Suggested extraction
function testAlertingRulesFilteringRegression( perspective: PerspectiveConfig, beforeFilters?: () => void, ) { it(`${perspective.name} perspective - Alerting > Alerting Rules page - Filtering`, () => { cy.log('2.1 use sidebar nav to go to Observe > Alerting'); nav.tabs.switchTab('Alerting rules'); alertingRuleListPage.shouldBeLoaded(); beforeFilters?.(); cy.log('2.2 clear all filters, verify filters and tags'); // ...shared body... alertingRuleListPage.filter.assertNoClearAllFilters(); }); }Then call it with
() => listPage.filter.removeIndividualTag(Source.PLATFORM)fromtestAlertsCorePlatformHeaderRegressionand with no callback fromtestAlertsFleetManagementRegression.Also applies to: 142-183
🤖 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/monitoring/01.reg_alerts.cy.ts` around lines 43 - 113, Extract the duplicated Alerting Rules filtering test flow from testAlertsFleetManagementRegression and testAlertsCorePlatformHeaderRegression into a shared testAlertingRulesFilteringRegression helper. Parameterize the helper with an optional beforeFilters callback, invoke it before applying filters, and pass the Source.PLATFORM removal callback only from the Core Platform test while leaving the Fleet Management call without one.
🤖 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/e2e/coo/02.acm_alerting_ui.cy.ts`:
- Around line 58-69: Add cy.closeOnboardingModalIfPresent() after
cy.switchPerspective('Fleet management') in the “Visual validation for features
that should not be displayed under Fleet management perspective” test, before
navigating with nav.sidenav.clickNavLink(...), so the test works independently
when the onboarding modal is present.
---
Nitpick comments:
In `@web/cypress/support/monitoring/01.reg_alerts.cy.ts`:
- Around line 43-113: Extract the duplicated Alerting Rules filtering test flow
from testAlertsFleetManagementRegression and
testAlertsCorePlatformHeaderRegression into a shared
testAlertingRulesFilteringRegression helper. Parameterize the helper with an
optional beforeFilters callback, invoke it before applying filters, and pass the
Source.PLATFORM removal callback only from the Core Platform test while leaving
the Fleet Management call without one.
🪄 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: 4b8b056f-70a6-4cd9-8420-8be26cad4f42
📒 Files selected for processing (12)
web/cypress/e2e/coo/02.acm_alerting_ui.cy.tsweb/cypress/e2e/monitoring/regression/01.reg_alerts_admin.cy.tsweb/cypress/fixtures/coo/acm-install.shweb/cypress/fixtures/monitoring/constants.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/monitoring/04.reg_alerts_namespace.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/incidents-page.tsweb/cypress/views/list-page.tsweb/cypress/views/nav.tsweb/cypress/views/silences-list-page.ts
💤 Files with no reviewable changes (1)
- web/cypress/views/silences-list-page.ts
| it('Visual validation for features that should not be displayed under Fleet management perspective', () => { | ||
| // check Incidents page is not displayed under Fleet management perspective | ||
| cy.log('Incidents page should not be displayed under Fleet management perspective'); | ||
| cy.switchPerspective('Fleet management'); | ||
| nav.sidenav.clickNavLink(['Observe', 'Alerting']); | ||
| commonPages.titleShouldHaveText('Alerting'); | ||
| incidentsPage.incidentsPageShouldNotBeVisible(); | ||
|
|
||
| // check Signal correlation option is not displayed under Fleet management perspective | ||
| cy.log('Signal correlation option should not be displayed under Fleet management perspective'); | ||
| troubleshootingPanelPage.signalCorrelationShouldNotBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential flakiness: onboarding modal not dismissed before navigating in this new test.
This test calls cy.switchPerspective('Fleet management') again but, unlike the first test (line 42), never calls cy.closeOnboardingModalIfPresent(). If this test executes without the preceding test having already dismissed the modal (e.g. run in isolation, retried, or split across CI shards), the modal could still be open and block nav.sidenav.clickNavLink(...).
🔧 Suggested fix
cy.switchPerspective('Fleet management');
+ cy.closeOnboardingModalIfPresent();
nav.sidenav.clickNavLink(['Observe', 'Alerting']);📝 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.
| it('Visual validation for features that should not be displayed under Fleet management perspective', () => { | |
| // check Incidents page is not displayed under Fleet management perspective | |
| cy.log('Incidents page should not be displayed under Fleet management perspective'); | |
| cy.switchPerspective('Fleet management'); | |
| nav.sidenav.clickNavLink(['Observe', 'Alerting']); | |
| commonPages.titleShouldHaveText('Alerting'); | |
| incidentsPage.incidentsPageShouldNotBeVisible(); | |
| // check Signal correlation option is not displayed under Fleet management perspective | |
| cy.log('Signal correlation option should not be displayed under Fleet management perspective'); | |
| troubleshootingPanelPage.signalCorrelationShouldNotBeVisible(); | |
| }); | |
| it('Visual validation for features that should not be displayed under Fleet management perspective', () => { | |
| // check Incidents page is not displayed under Fleet management perspective | |
| cy.log('Incidents page should not be displayed under Fleet management perspective'); | |
| cy.switchPerspective('Fleet management'); | |
| cy.closeOnboardingModalIfPresent(); | |
| nav.sidenav.clickNavLink(['Observe', 'Alerting']); | |
| commonPages.titleShouldHaveText('Alerting'); | |
| incidentsPage.incidentsPageShouldNotBeVisible(); | |
| // check Signal correlation option is not displayed under Fleet management perspective | |
| cy.log('Signal correlation option should not be displayed under Fleet management perspective'); | |
| troubleshootingPanelPage.signalCorrelationShouldNotBeVisible(); | |
| }); |
🤖 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/coo/02.acm_alerting_ui.cy.ts` around lines 58 - 69, Add
cy.closeOnboardingModalIfPresent() after cy.switchPerspective('Fleet
management') in the “Visual validation for features that should not be displayed
under Fleet management perspective” test, before navigating with
nav.sidenav.clickNavLink(...), so the test works independently when the
onboarding modal is present.
|
/test e2e-monitoring |
|
@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. |
2 participants
Summary by CodeRabbit