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

[Cases] Fix flakiness in all_cases_list.test.tsx #157285

Merged
merged 19 commits into from
May 15, 2023

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented May 10, 2023

Summary

This PR fixes the flaky tests in x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx. It modifies the getComputedStyle to be more performant. For more information see https://blog.sentry.io/sentrys-frontend-tests-migrating-from-enzyme-to-react-testing-library and jsdom/jsdom#3234.

I run the test multiple times on CI to ensure that the test is not flaky anymore: Specifically:

Fixes: #148096, #150923

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.9.0 labels May 12, 2023
@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@cnasikas cnasikas changed the title [Cases] Fix flaky UI tests [Cases] Fix flakiness in all_cases_list.test.tsx May 13, 2023
@cnasikas cnasikas marked this pull request as ready for review May 13, 2023 12:43
@cnasikas cnasikas requested a review from a team as a code owner May 13, 2023 12:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Verified locally. It took some time to run whole test but didn't time out.
Great job!! 👏 🎉

@@ -705,7 +739,7 @@ describe.skip('AllCasesListGeneric', () => {
});
});

describe('Actions', () => {
describe.each(Array(55).fill(null))('Actions', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget to remove it before merge 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Thanks!

// The JSDOM implementation is too slow
// Especially for dropdowns that try to position themselves
// perf issue - https://github.com/jsdom/jsdom/issues/3234
Object.defineProperty(window, 'getComputedStyle', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great finding!! 👏
perhaps we could try the same for xpack/plugins/cases/public/components/case_view/components/ case_view_activity.test.tsx component as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcger told me about it! Yes I try to improve the case_view_activity.test.tsx on another PR.

@adcoelho
Copy link
Contributor

I run the test multiple times on CI to ensure that the test is not flaky anymore: Specifically:

Why not use the flaky test runner?

@cnasikas
Copy link
Member Author

I run the test multiple times on CI to ensure that the test is not flaky anymore: Specifically:

Why not use the flaky test runner?

Unfortunately, the flaky test runner does not support unit tests.

@cnasikas cnasikas enabled auto-merge (squash) May 15, 2023 12:20
Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
cases 55 56 +1
enterpriseSearch 19 21 +2
securitySolution 400 404 +4
total +7

Total ESLint disabled count

id before after diff
cases 73 74 +1
enterpriseSearch 20 22 +2
securitySolution 480 484 +4
total +7

History

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

@cnasikas cnasikas merged commit b782df8 into elastic:main May 15, 2023
@cnasikas cnasikas deleted the fix_flaky_tests branch May 15, 2023 13:23
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 15, 2023
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
## Summary

This PR fixes the flaky tests in
`x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx`.
It modifies the `getComputedStyle` to be more performant. For more
information see
https://blog.sentry.io/sentrys-frontend-tests-migrating-from-enzyme-to-react-testing-library
and jsdom/jsdom#3234.

I run the test multiple times on CI to ensure that the test is not flaky
anymore: Specifically:

- Test run 50 times in
ccf2716
- Test run 40 times in
c56c650
- Test run 50 times in
b04d613
- Test run 50 times in
e0ef292
- Test run 55 times in
1ba6bac

Fixes: #148096,
#150923

### Checklist

Delete any items that are not applicable to this PR.

- [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

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.9.0
Projects
None yet
6 participants