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
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions x-pack/test_utils/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,50 @@ type TestSubjects = 'indicesTable' | 'createIndexButton' | 'pageTitle';
export const setup = registerTestBed<TestSubjects>(MyComponent);
```

## Best practices

In order to prevent flakiness in component integration tests, please consider the following best practices:

- **Use** `jest.useFakeTimers()`.

The code under a test might be using `setTimeout()` calls. This is bad for deterministic tests. In order to avoid it, we need to use mocked timers from jest. For that we declare at the top of each component integration test the following:
```js
beforeAll(() => {
jest.useFakeTimers();
});
afterAll(() => {
jest.useRealTimers();
});
```

- **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.


- **Do not declare** `component.update()` inside `act()`. Each `act()` call should contain a chunk of actions that updates the internal state(s). The `component.update()` that re-renders the internal DOM needs to be called outside, before asserting against the updated DOM.

- Be **synchronous** as much as possible.
Hooks are delicate when it comes to state updates. Sometimes calling `act()` synchronously works, sometimes it doesn't. The reasoning behind this isn't clear yet. The best approach is to try synchrounsly first and if it fails, because of an `act()` error, then use the async version.

```js
// First try this
act(() => {
find('someTestSubject').simulate('click');
});
component.update();
// If there is a "missing `act()` error", then change it to
await act(async () => {
find('someTestSubject').simulate('click');
});
component.update();
```

## Chrome extension

There is a small Chrome extension that you can install in order to track the test subject on the current page. As it is meant to be used
during development, the extension is only active when navigating a `localhost` URL.
There is a small Chrome extension that you can install in order to track the test subjects on the current page. As it is meant to be used
during development, the extension is only active when navigating to a `localhost` URL.

You will find the "Test subjects finder" extension in the `x-pack/test_utils/chrome_extension` folder.

Expand Down Expand Up @@ -158,7 +198,7 @@ describe('<RemoteClusterList />', () => {
});
```

**@returns** An object with the following **properties** and **helpers** for the testing
**@returns** An object with the following **properties** and **helpers** for testing

#### `component`

Expand Down