Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Detections]Fixes Rule Management Cypress Tests #96505

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

spong
Copy link
Member

@spong spong commented Apr 7, 2021

Summary

Fixes two cypress tests:

Deleting prebuilt rules "before each" hook for "Does not allow to delete one rule when more than one is selected"
#68607

This one is more of a drive around the pot-hole fix as we were waiting for the Alerts Table to load when we really didn't need to. Removed unnecessary check.

Alerts rules, prebuilt rules Loads prebuilt rules
#71300

This one was fixed with a .pipe() and .should('not.be.visible') to ensure the click was successful. Also removed unnecessary check on the Alerts Table loading that was present here as well too..

@spong spong added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.13.0 labels Apr 7, 2021
@spong spong requested a review from a team as a code owner April 7, 2021 21:01
@spong spong self-assigned this Apr 7, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

Copy link
Contributor

@ecezalp ecezalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for taking care of this! FWIW tests are locally green for me.

Screen Shot 2021-04-07 at 5 43 14 PM

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -191,7 +191,10 @@ export const resetAllRulesIdleModalTimeout = () => {

export const changeRowsPerPageTo = (rowsCount: number) => {
cy.get(PAGINATION_POPOVER_BTN).click({ force: true });
cy.get(rowsPerPageSelector(rowsCount)).click();
cy.get(rowsPerPageSelector(rowsCount))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Click until we see the desired effect, the effect here being "element is not visible," indicating that the click was received and thus the popover was hidden.

In this way, we avoid the issue at hand, where we were trying to click an element as it was animated/redrawing. 👍

@spong spong enabled auto-merge (squash) April 8, 2021 00:36
@spong spong added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 8, 2021
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / general / Closes and opens alerts.Closing alerts Closes and opens alerts

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: expected '<span.euiBadge.euiBadge--hollow.euiBadge--iconLeft>' to have text '297', but the text was '497'
    at Context.eval (http://localhost:6121/__cypress/tests?p=cypress/integration/detection_alerts/closing.spec.ts:20202:43)

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @spong

@spong spong merged commit d63fbb1 into elastic:master Apr 8, 2021
@spong spong deleted the fixes-rule-mgmt-cypress-tests branch April 8, 2021 00:59
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 8, 2021
…lastic#96505)

## Summary
Fixes two cypress tests:

> Deleting prebuilt rules "before each" hook for "Does not allow to delete one rule when more than one is selected"
elastic#68607

This one is more of a drive around the pot-hole fix as we were waiting for the Alerts Table to load when we really didn't need to. Removed unnecessary check.
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113932347-b9ab9480-97b0-11eb-8e07-5f3e0c4b6c78.png" />
</p>

> Alerts rules, prebuilt rules Loads prebuilt rules
elastic#71300

This one was fixed with a `.pipe()` and `.should('not.be.visible')` to ensure the click was successful. Also removed unnecessary check on the Alerts Table loading that was present here as well too..
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113932285-ac8ea580-97b0-11eb-90e1-618c510d33a7.png" />
</p>
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #96521

This backport PR will be merged automatically after passing CI.

@spong
Copy link
Member Author

spong commented Apr 8, 2021

May've regressed this one... 👀

Failed Tests Reporter:

  • Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: expected '<span.euiBadge.euiBadge--hollow.euiBadge--iconLeft>' to > have text '297', but the text was '497'
at Context.eval (http://localhost:6121/__cypress/tests?p=cypress/integration/detection_alerts/closing.spec.ts:20202:43)

@spong
Copy link
Member Author

spong commented Apr 8, 2021

Passing locally, though upon further inspection, this test is definitely going to be flakey as it's checking counts on alerts as they move through different states and there are new alerts that keep coming in (hence the count mis-match in the above failure). Potential fixes would be to use an absolute daterange to after the first round of alerts were generated, or just stop generating alerts before performing the alert state changes.

edit: PR here #96523

kibanamachine added a commit that referenced this pull request Apr 8, 2021
…96505) (#96521)

## Summary
Fixes two cypress tests:

> Deleting prebuilt rules "before each" hook for "Does not allow to delete one rule when more than one is selected"
#68607

This one is more of a drive around the pot-hole fix as we were waiting for the Alerts Table to load when we really didn't need to. Removed unnecessary check.
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113932347-b9ab9480-97b0-11eb-8e07-5f3e0c4b6c78.png" />
</p>

> Alerts rules, prebuilt rules Loads prebuilt rules
#71300

This one was fixed with a `.pipe()` and `.should('not.be.visible')` to ensure the click was successful. Also removed unnecessary check on the Alerts Table loading that was present here as well too..
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113932285-ac8ea580-97b0-11eb-90e1-618c510d33a7.png" />
</p>

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
spong added a commit that referenced this pull request Apr 8, 2021
…6523)

## Summary

As identified in #96505 (comment), this fixes the flakiness in the `Closing alerts` cypress test. Method used was to just delete the rule after the initial batch of alerts were generated. Alternatively we could add a function for disabling the rule (didn't see one in there), but the outcome is the same, no more alerts generated while the test is being performed. 🙂 

> Passing locally, though upon further inspection, this test is definitely going to be flakey as it's checking counts on alerts as they move through different states and there are new alerts that keep coming in (hence the count mis-match in the above failure). Potential fixes would be to use an absolute daterange to after the first round of alerts were generated, or just stop generating alerts before performing the alert state changes.


##### Before:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113952824-ae1d9500-97d3-11eb-9021-6737544b9c50.png" />
</p>


##### After:

<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113958216-ed50e380-97dd-11eb-9d22-d1c6aafc97d2.png" />
</p>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 8, 2021
…astic#96523)

## Summary

As identified in elastic#96505 (comment), this fixes the flakiness in the `Closing alerts` cypress test. Method used was to just delete the rule after the initial batch of alerts were generated. Alternatively we could add a function for disabling the rule (didn't see one in there), but the outcome is the same, no more alerts generated while the test is being performed. 🙂 

> Passing locally, though upon further inspection, this test is definitely going to be flakey as it's checking counts on alerts as they move through different states and there are new alerts that keep coming in (hence the count mis-match in the above failure). Potential fixes would be to use an absolute daterange to after the first round of alerts were generated, or just stop generating alerts before performing the alert state changes.


##### Before:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113952824-ae1d9500-97d3-11eb-9021-6737544b9c50.png" />
</p>


##### After:

<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113958216-ed50e380-97dd-11eb-9d22-d1c6aafc97d2.png" />
</p>
kibanamachine added a commit that referenced this pull request Apr 8, 2021
…6523) (#96526)

## Summary

As identified in #96505 (comment), this fixes the flakiness in the `Closing alerts` cypress test. Method used was to just delete the rule after the initial batch of alerts were generated. Alternatively we could add a function for disabling the rule (didn't see one in there), but the outcome is the same, no more alerts generated while the test is being performed. 🙂 

> Passing locally, though upon further inspection, this test is definitely going to be flakey as it's checking counts on alerts as they move through different states and there are new alerts that keep coming in (hence the count mis-match in the above failure). Potential fixes would be to use an absolute daterange to after the first round of alerts were generated, or just stop generating alerts before performing the alert state changes.


##### Before:
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113952824-ae1d9500-97d3-11eb-9021-6737544b9c50.png" />
</p>


##### After:

<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/113958216-ed50e380-97dd-11eb-9d22-d1c6aafc97d2.png" />
</p>

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants