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

[React18] Migrate test suites to account for testing library upgrades security-entity-analytics #201178

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Nov 21, 2024

This PR migrates test suites that use renderHook from the library @testing-library/react-hooks to adopt the equivalent and replacement of renderHook from the export that is now available from
@testing-library/react. This work is required for the planned migration to react18.

Context

In this PR, usages of waitForNextUpdate that previously could have been destructured from renderHook are now been replaced with waitFor exported from @testing-library/react, furthermore waitFor
that would also have been destructured from the same renderHook result is now been replaced with waitFor from the export of @testing-library/react.

Why is waitFor a sufficient enough replacement for waitForNextUpdate, and better for testing values subject to async computations?

WaitFor will retry the provided callback if an error is returned, till the configured timeout elapses. By default the retry interval is 50ms with a timeout value of 1000ms that
effectively translates to at least 20 retries for assertions placed within waitFor. See https://testing-library.com/docs/dom-testing-library/api-async/#waitfor for more information.
This however means that for person's writing tests, said person has to be explicit about expectations that describe the internal state of the hook being tested.
This implies checking for instance when a react query hook is being rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most existing assertions following an invocation of waitForNextUpdate being placed within a waitFor
invocation. In some cases the replacement is simply a waitFor(() => new Promise((resolve) => resolve(null))) (many thanks to @kapral18, for point out exactly why this works),
where this suffices the assertions that follow aren't placed within a waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test and application code to improve said existing test.

What to do next?

  1. Review the changes in this PR.
  2. If you think the changes are correct, approve the PR.

Any questions?

If you have any questions or need help with this PR, please leave comments in this PR.

@eokoneyo eokoneyo added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:Entity Analytics Security Entity Analytics Team React@18 labels Nov 21, 2024
@eokoneyo eokoneyo self-assigned this Nov 21, 2024
@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo eokoneyo force-pushed the chore/testing-library-migration-for-security-entity-analytics branch from 3aec23a to ca64f75 Compare November 27, 2024 14:29
@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo eokoneyo marked this pull request as ready for review November 29, 2024 11:09
@eokoneyo eokoneyo requested review from a team as code owners November 29, 2024 11:09
@eokoneyo eokoneyo requested a review from tiansivive November 29, 2024 11:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-entity-analytics (Team:Entity Analytics)

@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 2, 2024

@elasticmachine merge upstream

1 similar comment
@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 4, 2024

@elasticmachine merge upstream

@elasticmachine elasticmachine requested a review from a team as a code owner December 4, 2024 13:18
@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 5, 2024

@elasticmachine merge upstream

1 similar comment
@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 9, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

4 similar comments
@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo eokoneyo force-pushed the chore/testing-library-migration-for-security-entity-analytics branch from 8df8789 to 1d07dce Compare December 17, 2024 16:21
@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @eokoneyo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) React@18 release_note:skip Skip the PR/issue when compiling release notes Team:Entity Analytics Security Entity Analytics Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants