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

Run ILM jest tests as integration tests allowing them to run beyond 5s timeout #141750

Merged
merged 7 commits into from
Sep 28, 2022

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Sep 26, 2022

Summary

fix #141645
fix #141160
fix #115307

ILM heavy unit tests are flaky and can fail with a 5-second timeout. It happened in the past but it looks like it got worse with newly added features (#140804, #138748). (I assume that the problem is not specific to the new code, but just caused by adding more code)

We could have increased the timeout manually, but @spalger suggested to run these tests as integration tests. Here Spencer also explains the differences:

...the gist is that Jest integration tests are just in a different directory and have two main differences:

  1. They never use parallelism, this allows them to access file system resources, launch services, etc. without needing to worry about conflicts with other tests
  2. They are allowed to take their sweet time, the default timeout is currently 10 minutes.

Unit tests are really intended for testing one small piece of the code in isolation. Shared state between test/dependencies on previous tests, taking more than a few milliseconds to run, or integrating with many systems and running an entire simulated front-end application in Node are all examples of things that go beyond the scope of what a unit test is intended for.

In order to move these tests to be "integration tests" they should be in a directory named "integration_tests". These tests will run when you use node scripts/jest_integration <some_project_dir>.

@Dosant
Copy link
Contributor Author

Dosant commented Sep 27, 2022

@elasticmachine merge upstream

@Dosant Dosant changed the title ILM form validation tests Run ILM jest tests as integration tests allowing them to run beyond 5s timeout Sep 27, 2022
@Dosant Dosant added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Team:AppServicesUx Feature:ILM labels Sep 27, 2022
@Dosant Dosant marked this pull request as ready for review September 27, 2022 14:26
@Dosant Dosant requested a review from a team as a code owner September 27, 2022 14:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

👍 thank you @Dosant

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for doing this @Dosant !

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Thanks for addressing this @Dosant!

One small request 😄 What do you think about updating the README with some background information around these tests and why they were moved to integration_tests? The deployment management team owns many apps with similar testing infrastructure, and I could see someone coming back to the code confused why ILM follows a slightly different pattern.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@Dosant Dosant added the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label Sep 28, 2022
@Dosant Dosant merged commit 0471095 into elastic:main Sep 28, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 28, 2022
…s timeout (#141750) (#142067)

(cherry picked from commit 0471095)

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
awahab07 pushed a commit to awahab07/kibana that referenced this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment