Skip to content

fix(sa): Add visibility checks and waits to alerts row button acknowl… - #2101

Open
yashgupte21 wants to merge 1 commit into
opensearch-project:2.11from
yashgupte21:fix-sa-2.11
Open

fix(sa): Add visibility checks and waits to alerts row button acknowl…#2101
yashgupte21 wants to merge 1 commit into
opensearch-project:2.11from
yashgupte21:fix-sa-2.11

Conversation

@yashgupte21

Copy link
Copy Markdown
Contributor

…edge test

The 'can be acknowledged via row button' test fails because the Status filter popover is not ready when the test interacts with it. Added .should('be.visible') assertions and short waits to ensure the popover renders before clicking filter options.

Description

Fix flaky "can be acknowledged via row button" test in 3_alerts.spec.js. The Status filter popover wasn't ready when the test interacted with it. Added .should('be.visible') assertions and short waits to ensure the popover renders before clicking filter options.

Issues Resolved

Resolves consistent failure of can be acknowledged via row button test .

Test:
Ran against OS_2.11 domain

Alerts
    ✓ are generated (165995ms)
    ✓ contain expected values in table (30671ms)
    ✓ contain expected values in alert details flyout (15961ms)
    ✓ contain expected values in finding details flyout (45240ms)
    ✓ can be bulk acknowledged (82407ms)
    ✓ can be acknowledged via row button (22753ms)
    ✓ can be acknowledged via flyout button (15840ms)


  7 passing (6m)


  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        7                                                                                │
  │ Passing:      7                                                                                │
  │ Failing:      0                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  0                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     6 minutes, 20 seconds                                                            │
  │ Spec Ran:     plugins/security-analytics-dashboards-plugin/3_alerts.spec.js                    │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


====================================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  plugins/security-analytics-dashboar      06:20        7        7        -        -        - │
  │    ds-plugin/3_alerts.spec.js                                                                  │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✔  All specs passed!                        06:20        7        7        -        -        -  

✨  Done in 392.76s.

Check List

  • [x ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…edge test

The 'can be acknowledged via row button' test fails because the Status
filter popover is not ready when the test interacts with it. Added
.should('be.visible') assertions and short waits to ensure the popover
renders before clicking filter options.

Signed-off-by: YashPradeep Gupte <yashprg@amazon.com>
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Missing Status filter reopen

In the second filter block (for switching to "Acknowledged" alerts), the code clicks items inside euiFilterSelect__items but the previous version relied on the Status popover already being open. The new code adds cy.get('[data-text="Status"]').should('be.visible').click(...) before it, which is correct for opening the popover — however, after selecting Active (to deselect) and Acknowledged, the popover state may differ from the original test expectation. Verify that clicking Status here toggles the popover open rather than closing an already-open one, since this test previously assumed the popover was still open from earlier interactions.

cy.wait(2000);
cy.get('[data-text="Status"]').should('be.visible').click({ force: true });
cy.get('[class="euiFilterSelect__items"]')
  .should('be.visible')
  .within(() => {
    cy.contains('Active').click({ force: true });
    cy.contains('Acknowledged').click({ force: true });
  });
Fixed waits

Multiple cy.wait(2000) calls are used to work around timing issues. Arbitrary fixed waits reduce test speed and can still be flaky under load. Prefer waiting on network requests (cy.intercept + cy.wait('@alias')) or asserting on DOM state (e.g., .should('have.length', 1) already implicitly retries) instead of fixed sleeps.

cy.wait(2000);
cy.get('tbody > tr')
  .filter(`:contains(${alertName})`)
  .should('have.length', 1);

// Filter the table to show only "Acknowledged" alerts
cy.wait(2000);
cy.get('[data-text="Status"]').should('be.visible').click({ force: true });
cy.get('[class="euiFilterSelect__items"]')
  .should('be.visible')
  .within(() => {
    cy.contains('Active').click({ force: true });
    cy.contains('Acknowledged').click({ force: true });
  });

// Wait for filter to apply
cy.wait(2000);

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid arbitrary waits, rely on retries

Replace the arbitrary cy.wait(2000) with Cypress's built-in retry-ability on the
assertion. The .should('have.length', 1) assertion will already retry until it
passes or times out, making the fixed wait unnecessary and reducing test
flakiness/duration. Consider increasing the assertion timeout instead if needed.

cypress/integration/plugins/security-analytics-dashboards-plugin/3_alerts.spec.js [391-395]

-// Wait for acknowledge to go through
-cy.wait(2000);
-cy.get('tbody > tr')
+cy.get('tbody > tr', { timeout: 10000 })
   .filter(`:contains(${alertName})`)
   .should('have.length', 1);
Suggestion importance[1-10]: 6

__

Why: Valid suggestion to replace cy.wait(2000) with Cypress's built-in retry-ability, improving test determinism and reducing flakiness. Moderate impact as it improves test quality but doesn't fix a critical bug.

Low
Prefer assertion timeouts over fixed waits

Replace cy.wait(2000) with a longer assertion timeout so Cypress retries the query
until the filter is applied. This makes the test more deterministic and faster when
the UI responds quickly, while still tolerating slower environments.

cypress/integration/plugins/security-analytics-dashboards-plugin/3_alerts.spec.js [407-412]

-// Wait for filter to apply
-cy.wait(2000);
 // Confirm there are now 3 "Acknowledged" alerts
-cy.get('tbody > tr')
+cy.get('tbody > tr', { timeout: 10000 })
   .filter(`:contains(${alertName})`)
   .should('have.length', 3);
Suggestion importance[1-10]: 6

__

Why: Similarly valid suggestion to replace fixed waits with assertion timeouts, making the test more efficient and reliable. Moderate impact on test quality and maintainability.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants