[React18] Migrate test suites to account for testing library upgrades security-solution #201176
Conversation
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
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
|
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". |
| }); | ||
|
|
||
| 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 |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
cc @eokoneyo |
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.