[React18] Migrate test suites to account for testing library upgrades security-solution #201176
+666
−825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eokoneyo
added
release_note:skip
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
|
/ci |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
@elasticmachine merge upstream |
1 similar comment
|
@elasticmachine merge upstream |
eokoneyo
force-pushed
the
chore/testing-library-migration-for-security-solution
branch
from
88cae85 to
0ec6d48
Compare
|
@elasticmachine merge upstream |
eokoneyo
force-pushed
the
chore/testing-library-migration-for-security-solution
branch
from
57858f5 to
ef19e82
Compare
|
@elasticmachine merge upstream |
| signalIndexMappingOutdated: null, | ||
| }); | ||
| expect(result.error).toBeUndefined(); | ||
| expect(result.current).toEqual({ |
There was a problem hiding this comment.
why removing expect(result.all).toHaveLength(1); and expect(result.error).toBeUndefined(); here?
There was a problem hiding this comment.
renderHook no longer returns the result.all and result.error property
PhilippeOberti
approved these changes
Dec 9, 2024
There was a problem hiding this comment.
changes for the Threat Hunting Investigations team look good. I left this comment just in case it wasn't done on purpose
|
@elasticmachine merge upstream |
Files by Code Ownerelastic/security-detection-rule-management
elastic/security-solution
elastic/security-threat-hunting-explore
elastic/security-threat-hunting-investigations
|
maximpn
reviewed
Dec 12, 2024
There was a problem hiding this comment.
@eokoneyo Thanks for that huge effort to make React 18 upgrade possible 🙏
I have concerns regarding waitFor(() => new Promise((resolve) => resolve(null)) (it's used a lot in this PR). It's pretty tricky to comprehend why it replaces waitForNextUpdate. An utility function could be created to encapsulate waitFor(() => new Promise((resolve) => resolve(null)). Additionally it'd be great to have an explanation comment. Obviously these two don't match 1 to 1 and it's important to understand pros and cons. It'd be great to know performance and execution time wise difference between these two. It looks beneficial to get rid of such a hack in favor of explicit result state expectation though it's definitely out of scope of this PR.
Rule Management changes LGTM but the major part of the diff is in common Security Solution ownership where comments above are related.
|
|
||
| await waitForNextUpdate(); | ||
| await waitFor(() => new Promise((resolve) => resolve(null))); |
There was a problem hiding this comment.
Adding a comment why await waitFor(() => new Promise((resolve) => resolve(null))); gives the same result as await waitForNextUpdate(); would be great.
Is it possible to replace it with rerender invocation?
| const { result } = renderHook(() => useAggregatedAnomaliesByJob({ skip: false, from, to }), { | ||
| wrapper: TestProviders, | ||
| }); | ||
| await waitFor(() => new Promise((resolve) => resolve(null))); |
There was a problem hiding this comment.
The same as above. Maybe create a reusable utility function?
|
Hi @PhilippeOberti, I'm curious for better understanding. Have you looked at some files changed in |
eokoneyo
force-pushed
the
chore/testing-library-migration-for-security-solution
branch
from
d749341 to
5747fce
Compare
Hi @maximpn there's no 1-to-1 replacement for |
Hi @eokoneyo, such question I asked may pop up in everyone's mind reading tests code. That's why I suggest to create an utility function named Having multiple entries Do you have any concerns with that? |
…ary owned by security-solution
eokoneyo
force-pushed
the
chore/testing-library-migration-for-security-solution
branch
from
5747fce to
198bc5e
Compare
eokoneyo
force-pushed
the
chore/testing-library-migration-for-security-solution
branch
from
198bc5e to
a80bf74
Compare
Hi @maximpn I'm sorry we've had this long conversation, whilst attempting to act on your suggestion, I took a second attempt at the test and actually removed all instances of the "hack". |
kapral18
approved these changes
Dec 19, 2024
maximpn
approved these changes
Dec 19, 2024
| }); | ||
|
|
||
| mockSearchStrategy.mockReturnValue(searchStrategy$.asObservable()); | ||
|
|
||
| (useKibana as jest.Mock).mockReturnValue(mockUseKibana); |
There was a problem hiding this comment.
nit: Ideally we should have mock in each test below. The latter one will have of({lastSeen: '1 minute ago'}. It wil improve readability.
(useKibana as jest.Mock).mockReturnValue({
search: jest.fn(). mockReturnValue(of({ lastSeen: null }))
});
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
cc @eokoneyo |
eokoneyo
deleted the
chore/testing-library-migration-for-security-solution
branch
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 30, 2024
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit f3d7fa7)
💚 All backports created successfully
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
Dec 30, 2024
…grades security-solution (#201176) (#205265) # Backport This will backport the following commits from `main` to `8.x`: - [[React18] Migrate test suites to account for testing library upgrades security-solution (#201176)](#201176) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Eyo O. Eyo","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-30T14:23:51Z","message":"[React18] Migrate test suites to account for testing library upgrades security-solution (#201176)\n\nThis PR migrates test suites that use `renderHook` from the library\r\n`@testing-library/react-hooks` to adopt the equivalent and replacement\r\nof `renderHook` from the export that is now available from\r\n`@testing-library/react`. This work is required for the planned\r\nmigration to react18.\r\n\r\n## Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously could have\r\nbeen destructured from `renderHook` are now been replaced with `waitFor`\r\nexported from `@testing-library/react`, furthermore `waitFor`\r\nthat would also have been destructured from the same renderHook result\r\nis now been replaced with `waitFor` from the export of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient enough replacement for\r\n`waitForNextUpdate`, and better for testing values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry the provided callback if an error is returned, till\r\nthe configured timeout elapses. By default the retry interval is `50ms`\r\nwith a timeout value of `1000ms` that\r\neffectively translates to at least 20 retries for assertions placed\r\nwithin waitFor. See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor more information.\r\nThis however means that for person's writing tests, said person has to\r\nbe explicit about expectations that describe the internal state of the\r\nhook being tested.\r\nThis implies checking for instance when a react query hook is being\r\nrendered, there's an assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd notice that this pattern has been adopted, with most\r\nexisting assertions following an invocation of `waitForNextUpdate` being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the replacement is simply a `waitFor(() => new\r\nPromise((resolve) => resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why this works),\r\nwhere this suffices the assertions that follow aren't placed within a\r\nwaitFor so this PR doesn't get larger than it needs to be.\r\n\r\nIt's also worth pointing out this PR might also contain changes to test\r\nand application code to improve said existing test.\r\n\r\n### What to do next?\r\n1. Review the changes in this PR.\r\n2. If you think the changes are correct, approve the PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help with this PR, please leave\r\ncomments in this PR.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f3d7fa7d122087b06eed098827d4c909b1ba90f2","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team: SecuritySolution","backport:prev-minor","React@18"],"title":"[React18] Migrate test suites to account for testing library upgrades security-solution","number":201176,"url":"https://github.com/elastic/kibana/pull/201176","mergeCommit":{"message":"[React18] Migrate test suites to account for testing library upgrades security-solution (#201176)\n\nThis PR migrates test suites that use `renderHook` from the library\r\n`@testing-library/react-hooks` to adopt the equivalent and replacement\r\nof `renderHook` from the export that is now available from\r\n`@testing-library/react`. This work is required for the planned\r\nmigration to react18.\r\n\r\n## Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously could have\r\nbeen destructured from `renderHook` are now been replaced with `waitFor`\r\nexported from `@testing-library/react`, furthermore `waitFor`\r\nthat would also have been destructured from the same renderHook result\r\nis now been replaced with `waitFor` from the export of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient enough replacement for\r\n`waitForNextUpdate`, and better for testing values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry the provided callback if an error is returned, till\r\nthe configured timeout elapses. By default the retry interval is `50ms`\r\nwith a timeout value of `1000ms` that\r\neffectively translates to at least 20 retries for assertions placed\r\nwithin waitFor. See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor more information.\r\nThis however means that for person's writing tests, said person has to\r\nbe explicit about expectations that describe the internal state of the\r\nhook being tested.\r\nThis implies checking for instance when a react query hook is being\r\nrendered, there's an assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd notice that this pattern has been adopted, with most\r\nexisting assertions following an invocation of `waitForNextUpdate` being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the replacement is simply a `waitFor(() => new\r\nPromise((resolve) => resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why this works),\r\nwhere this suffices the assertions that follow aren't placed within a\r\nwaitFor so this PR doesn't get larger than it needs to be.\r\n\r\nIt's also worth pointing out this PR might also contain changes to test\r\nand application code to improve said existing test.\r\n\r\n### What to do next?\r\n1. Review the changes in this PR.\r\n2. If you think the changes are correct, approve the PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help with this PR, please leave\r\ncomments in this PR.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f3d7fa7d122087b06eed098827d4c909b1ba90f2"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201176","number":201176,"mergeCommit":{"message":"[React18] Migrate test suites to account for testing library upgrades security-solution (#201176)\n\nThis PR migrates test suites that use `renderHook` from the library\r\n`@testing-library/react-hooks` to adopt the equivalent and replacement\r\nof `renderHook` from the export that is now available from\r\n`@testing-library/react`. This work is required for the planned\r\nmigration to react18.\r\n\r\n## Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously could have\r\nbeen destructured from `renderHook` are now been replaced with `waitFor`\r\nexported from `@testing-library/react`, furthermore `waitFor`\r\nthat would also have been destructured from the same renderHook result\r\nis now been replaced with `waitFor` from the export of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient enough replacement for\r\n`waitForNextUpdate`, and better for testing values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry the provided callback if an error is returned, till\r\nthe configured timeout elapses. By default the retry interval is `50ms`\r\nwith a timeout value of `1000ms` that\r\neffectively translates to at least 20 retries for assertions placed\r\nwithin waitFor. See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor more information.\r\nThis however means that for person's writing tests, said person has to\r\nbe explicit about expectations that describe the internal state of the\r\nhook being tested.\r\nThis implies checking for instance when a react query hook is being\r\nrendered, there's an assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd notice that this pattern has been adopted, with most\r\nexisting assertions following an invocation of `waitForNextUpdate` being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the replacement is simply a `waitFor(() => new\r\nPromise((resolve) => resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why this works),\r\nwhere this suffices the assertions that follow aren't placed within a\r\nwaitFor so this PR doesn't get larger than it needs to be.\r\n\r\nIt's also worth pointing out this PR might also contain changes to test\r\nand application code to improve said existing test.\r\n\r\n### What to do next?\r\n1. Review the changes in this PR.\r\n2. If you think the changes are correct, approve the PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help with this PR, please leave\r\ncomments in this PR.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>","sha":"f3d7fa7d122087b06eed098827d4c909b1ba90f2"}}]}] BACKPORT--> Co-authored-by: Eyo O. Eyo <[email protected]>
stratoula
pushed a commit
to stratoula/kibana
that referenced
this pull request
Jan 2, 2025
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]>
benakansara
pushed a commit
to benakansara/kibana
that referenced
this pull request
Jan 2, 2025
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]>
cqliu1
pushed a commit
to cqliu1/kibana
that referenced
this pull request
Jan 2, 2025
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]>
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Jan 13, 2025
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]>
viduni94
pushed a commit
to viduni94/kibana
that referenced
this pull request
Jan 23, 2025
… security-solution (elastic#201176) 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. --------- Co-authored-by: Elastic Machine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR migrates test suites that use
renderHookfrom the library@testing-library/react-hooksto adopt the equivalent and replacement ofrenderHookfrom 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
waitForNextUpdatethat previously could have been destructured fromrenderHookare now been replaced withwaitForexported from@testing-library/react, furthermorewaitForthat would also have been destructured from the same renderHook result is now been replaced with
waitForfrom the export of@testing-library/react.Why is
waitFora sufficient enough replacement forwaitForNextUpdate, 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
50mswith a timeout value of1000msthateffectively 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
waitForNextUpdatebeing placed within awaitForinvocation. 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?
Any questions?
If you have any questions or need help with this PR, please leave comments in this PR.