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 Solutions] Fixes a cypress flake by adding pipe, click, and should #92762

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Feb 25, 2021

Summary

We were seeing flake tests with the error messages:

expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '

Running this locally several times I was able to reproduce the flake. I was able to see that the click handler when clicked on the check boxes is not always taking effect for the first check box. A common issue with Cypress is that a lot of page loads and JS activities on pages can add/remove click handlers quickly such as when we are frosting and un-frosting a loading screen.

When we try and click on the click handlers for a test and the click handlers are not added yet, we miss one or more and end up with a flake test. Instead we can click as fast as possible using pipe and then checking that the check box is clicked before continuing using a should

Even though the loading screen is done frosting in these tests, the click handler can take one or two milliseconds before they end up being added to the checkboxes.

Analysis

When you have the flake and fail you can scroll in Cypress and pin a point in time and see that indeed we did not get the first checkbox checked even though we had a click. Which makes sense as within a few milliseconds the click handlers are added and we do check the second checkbox:
analysis

Checklist

@elasticmachine
Copy link
Contributor

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

@FrankHassanabad FrankHassanabad added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 25, 2021
@FrankHassanabad FrankHassanabad enabled auto-merge (squash) February 25, 2021 08:03
Copy link
Contributor

@peluja1012 peluja1012 left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement here, Frank!

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to improve this ^^

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

cc @FrankHassanabad

@FrankHassanabad FrankHassanabad merged commit bb8e505 into elastic:master Feb 25, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 25, 2021
## Summary

We were seeing flake tests with the error messages:

```ts
expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '
```

Running this locally several times I was able to reproduce the flake. I was able to see that the click handler when clicked on the check boxes is not always taking effect for the first check box. A common issue with Cypress is that a lot of page loads and JS activities on pages can add/remove click handlers quickly such as when we are frosting and un-frosting a loading screen.

When we try and click on the click handlers for a test and the click handlers are not added yet, we miss one or more and end up with a flake test. Instead we can click as fast as possible using `pipe` and then checking that the check box is clicked before continuing using a `should`

Even though the loading screen is done frosting in these tests, the click handler can take one or two milliseconds before they end up being added to the checkboxes.

**Analysis**

When you have the flake and fail you can scroll in Cypress and pin a point in time and see that indeed we did not get the first checkbox checked even though we had a click. Which makes sense as within a few milliseconds the click handlers are added and we do check the second checkbox:
<img width="2083" alt="analysis" src="https://user-images.githubusercontent.com/1151048/109119992-a19d1b80-7702-11eb-882a-c035eba97455.png">

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.12 / #92776
7.x / #92777

Successful backport PRs will be merged automatically after passing CI.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 25, 2021
## Summary

We were seeing flake tests with the error messages:

```ts
expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '
```

Running this locally several times I was able to reproduce the flake. I was able to see that the click handler when clicked on the check boxes is not always taking effect for the first check box. A common issue with Cypress is that a lot of page loads and JS activities on pages can add/remove click handlers quickly such as when we are frosting and un-frosting a loading screen.

When we try and click on the click handlers for a test and the click handlers are not added yet, we miss one or more and end up with a flake test. Instead we can click as fast as possible using `pipe` and then checking that the check box is clicked before continuing using a `should`

Even though the loading screen is done frosting in these tests, the click handler can take one or two milliseconds before they end up being added to the checkboxes.

**Analysis**

When you have the flake and fail you can scroll in Cypress and pin a point in time and see that indeed we did not get the first checkbox checked even though we had a click. Which makes sense as within a few milliseconds the click handlers are added and we do check the second checkbox:
<img width="2083" alt="analysis" src="https://user-images.githubusercontent.com/1151048/109119992-a19d1b80-7702-11eb-882a-c035eba97455.png">

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 25, 2021
* master: (38 commits)
  Fixes Cypress flake by adding pipe, click, and should (elastic#92762)
  [Discover] Fix filtering selected sidebar fields (elastic#91828)
  [ML] Fixes positions of calendar arrow buttons in start datafeed modal (elastic#92625)
  [dev/build_ts_refs] check that commit in outDirs matches mergeBase (elastic#92513)
  add dep on `@kbn/config` so it is built first
  [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (elastic#90481)
  [Lens] Fix Workspace hidden when using Safari (elastic#92616)
  [Lens] Fixes vertical alignment validation messages (elastic#91878)
  forbid x-elastic-product-origin header in elasticsearch configuration (elastic#92359)
  [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules (elastic#92081)
  [ILM][Accessibility] Added A11y test for ILM new policy form. (elastic#92570)
  [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (elastic#90634)
  Automatically generated Api documentation (elastic#86232)
  Increase index pattern select limit to 1000 (elastic#92093)
  [core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492)
  [Security Solution][Detection Rules] Update prebuilt rule threats to match schema (elastic#92281)
  [Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches (elastic#92565)
  [Dashboard] Export appropriate references from byValue panels (elastic#91567)
  [Upgrade Assistant] Align code between branches (elastic#91862)
  [Security Solution][Case] Fix alerts push (elastic#91638)
  ...
@FrankHassanabad FrankHassanabad deleted the fix-flake branch February 25, 2021 17:02
kibanamachine added a commit that referenced this pull request Feb 25, 2021
## Summary

We were seeing flake tests with the error messages:

```ts
expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '
```

Running this locally several times I was able to reproduce the flake. I was able to see that the click handler when clicked on the check boxes is not always taking effect for the first check box. A common issue with Cypress is that a lot of page loads and JS activities on pages can add/remove click handlers quickly such as when we are frosting and un-frosting a loading screen.

When we try and click on the click handlers for a test and the click handlers are not added yet, we miss one or more and end up with a flake test. Instead we can click as fast as possible using `pipe` and then checking that the check box is clicked before continuing using a `should`

Even though the loading screen is done frosting in these tests, the click handler can take one or two milliseconds before they end up being added to the checkboxes.

**Analysis**

When you have the flake and fail you can scroll in Cypress and pin a point in time and see that indeed we did not get the first checkbox checked even though we had a click. Which makes sense as within a few milliseconds the click handlers are added and we do check the second checkbox:
<img width="2083" alt="analysis" src="https://user-images.githubusercontent.com/1151048/109119992-a19d1b80-7702-11eb-882a-c035eba97455.png">

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
kibanamachine added a commit that referenced this pull request Feb 25, 2021
## Summary

We were seeing flake tests with the error messages:

```ts
expected '<button.euiButton.euiButton--primary>' to have text 'Install 2 Elastic prebuilt rules ', but the text was 'Install 1 Elastic prebuilt rule '
```

Running this locally several times I was able to reproduce the flake. I was able to see that the click handler when clicked on the check boxes is not always taking effect for the first check box. A common issue with Cypress is that a lot of page loads and JS activities on pages can add/remove click handlers quickly such as when we are frosting and un-frosting a loading screen.

When we try and click on the click handlers for a test and the click handlers are not added yet, we miss one or more and end up with a flake test. Instead we can click as fast as possible using `pipe` and then checking that the check box is clicked before continuing using a `should`

Even though the loading screen is done frosting in these tests, the click handler can take one or two milliseconds before they end up being added to the checkboxes.

**Analysis**

When you have the flake and fail you can scroll in Cypress and pin a point in time and see that indeed we did not get the first checkbox checked even though we had a click. Which makes sense as within a few milliseconds the click handlers are added and we do check the second checkbox:
<img width="2083" alt="analysis" src="https://user-images.githubusercontent.com/1151048/109119992-a19d1b80-7702-11eb-882a-c035eba97455.png">

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
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 v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants