Bump jest to v26 and update focus tests #1102
Conversation
Signed-off-by: Josh Romero <[email protected]>
joshuarrrr
requested review from
seanneumann,
AMoo-Miki,
rednaksi91,
KrooshalUX,
BSFishy,
bandinib-amzn,
ashwin-pc,
andreymyssak and
SergeyMyssak
as code owners
|
How is this different from #1094? Is there a case we would want this older version as apposed to the newer one? |
Signed-off-by: Josh Romero <[email protected]>
Signed-off-by: Josh Romero <[email protected]>
joshuarrrr
changed the title
It's more incremental, so there's a smaller diff. It gives us a chance to avoid a bunch of simultaneous changes due to jest, jsdom, and others. But now that I've got all the focus tests working (including re-enabling some long skipped), we may be able to very easily follow this with an upgrade to v29. |
| setTimeout(() => { | ||
| expect(childWrapper).toBe(document.activeElement); | ||
| }, 0); |
There was a problem hiding this comment.
How does the setTimeout fix the issues? I remember seeing changes about spying on the focus event, why don't we do that? It seems less flaky to me
There was a problem hiding this comment.
I don't know the exact reason this changed between jsdom versions, but previously you could check for the activeElement immediately, whereas with the update you need to await an update cycle for focus changes to be applied. (There are probably other possible ways of doing this that may be better, such as jest.useFakeTimers(); and jest.advanceTimersByTime(0);). But I found that the setTimeout method worked consistently in all cases, including some which were previously skipped, with minimal changes to existing test structure.
Before I got this working, I was spying on the focus event, but that's actually worse, because it only tells you that the event was triggered, but mostly we want to test where the focus actually ends up, which is better tested via document.activeElement.
There was a problem hiding this comment.
How confident are we that we are not introducing flakiness?
Have you by chance experimented with enzymejs/enzyme#2337 (comment)?
src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap
Outdated
Show resolved
Hide resolved
Description
Smaller change than #1094, but actually resolves the CVE, unlike #1091
Issues Resolved
Partially fixes #630 (CVE-2023-28155)
Check List
yarn lintyarn test-unitBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.