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

Add best practices section to test_utils readme #82393

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

alisonelizabeth
Copy link
Contributor

This PR updates the test_utils readme with a best practices section. This information was previously shared with the team by @sebelga in a different medium.

@alisonelizabeth alisonelizabeth added v8.0.0 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 v7.11.0 labels Nov 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

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.

Thanks for putting this doc here @alisonelizabeth ! 👍

});
```

- **Do not use** using `nextTick()`, `waitFor`, or `waitForFunc` helpers in tests. These helpers use `setTimeout` underneath and add latency in the tests, especially on CI where a timeout (even of a few ms) can trigger a timeout error. These helpers will eventually be deprecated once existing tests has been updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any downside to adding console.warn calls to these functions? This will help people developing tests realize they shouldn't use these helpers. They'll emit in CI too but I don't see a downside to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this as well. The one downside I can think of is nextTick is used a fair bit in some of the existing tests, so I could see it being a little noisy. I did a quick search and I don't see waitForFunc being used anywhere, so I think this one can be safely removed. waitFor is only used in remote clusters and CCR right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think our team is the only consumer of these functions so I thought that being all warned 😊 removes the need for a console.warn. The idea is to deprecate and remove those functions eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants