-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Jest config: use real timers by default #46714
Conversation
Size Change: 0 B Total Size: 1.32 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say I like changing so many files, but this workaround makes sense to me, given the constraints 👍
P.S. I can still see BorderControl
changes in this PR; is that intentional?
Yes. I'd like to first review and merge #46713, and only then rebase this one on top of |
d15ead7
to
5fdd5e4
Compare
The There are just two tests ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing, thank you Jarda 🚀
Left a few remarks, but nothing is blocking. I'd only prefer adding a CHANGELOG entry for the jest default preset change before shipping.
@@ -24,7 +24,7 @@ module.exports = { | |||
'**/?(*.)test.[jt]s?(x)', | |||
], | |||
testPathIgnorePatterns: [ '/node_modules/', '<rootDir>/vendor/' ], | |||
timers: 'fake', | |||
timers: 'real', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it will need a CHANGELOG entry. For some relying on fake timers to be the default, this might be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll return to this in a followup. Now my goal is to:
- land this real timers change
- make
useSelect
withuseSyncExternalStore
pass all the tests and merge it - try out the "enable concurrent mode" PR with the new
useSelect
, to see if the current e2e failures disappear or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
const onUpdateSlug = jest.fn(); | ||
|
||
render( <PostSlug postSlug="index" onUpdateSlug={ onUpdateSlug } /> ); | ||
|
||
const input = screen.getByRole( 'textbox', { name: 'Slug' } ); | ||
await user.clear( input ); | ||
await user.type( input, 'Foo Bar-Baz 9!' ); | ||
input.blur(); | ||
act( () => input.blur() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just trigger a tab
and that'd blur it anyway, but in a way resembling user behavior more closely?
@@ -11,17 +11,15 @@ import { PostSlug } from '../'; | |||
|
|||
describe( 'PostSlug', () => { | |||
it( 'should update slug with sanitized input', async () => { | |||
const user = userEvent.setup( { | |||
advanceTimers: jest.advanceTimersByTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have to remove that in more places where it's not necessary.
This patch changes the default timers used by Jest unit tests from
fake
toreal
. That should unblock theuseSelect
reimplementation in #46538 whose unit tests are now failing because of a React bug (facebook/react#25889). Changing the default works around that bug and should make the unit tests green again.The patch adds an explicit
jest.useFakeTimers()
call to all tests that use fake timers explicitly, i.e., use a Jest API to run the fake timers.The
BorderControl
test required somewhat more complex changes, which I'm proposing separately in #46713.