Skip to content

fix: Stop calling waitFor callback after timeout #1271

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 8, 2024
Merged

fix: Stop calling waitFor callback after timeout #1271

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a0bb005
prevent waitFor callback racing condition
KevinBonHubspot Oct 3, 2023
6b41483
change logic
KevinBonHubspot Oct 3, 2023
55ff2f2
adapt the waitFor resolve test to make it more readable
KevinBonHubspot Oct 4, 2023
67793f8
surface `timeout` + waitFor `callback` calls
KevinBonHubspot Oct 4, 2023
86c5c08
adding comment
KevinBonHubspot Oct 4, 2023
ce11c0f
adding reproductible test
KevinBonHubspot Oct 4, 2023
10a1ac7
robuster assertion for not calling callback again
Jan 8, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/wait-for.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,15 @@ function waitFor(
jest.advanceTimersByTime(interval)
})

// Could have timed-out
if (finished) {
Copy link
Member

@eps1lon eps1lon Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this factoring, I wonder if we could simplify further by moving checkCallback before the advanceTimers. That way the loop condition makes sure we exit as soon as we're finished. And we can remove another checkCallback on L54.

At this point I would remove the "It's really important that checkCallback [...]" comment. We might have fixed the issue along the way. And since I'd rather ship this with the next major, we have a good opportunity to resurface the bug so that we can at least link a repro. It doesn't have to be a Jest test but somehow the reordering fixed "something". That "something" needs to be linked a t least. Doesn't need to be an automated test but it needs to be something. Otherwise we're chasing ghosts.
Copy link
Contributor Author

@KevinBon KevinBon Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move checkCallback before the advanceTimers, the waitFor callback will be called twice before we advance the clock, instead of once right now.

Which will break this fix: https://github.com/testing-library/dom-testing-library/pull/1073/files
Copy link
Member

@eps1lon eps1lon Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the waitFor callback will be called twice before we advance the clock, instead of once right now.

but not if we remove the checkCallback call earlier from L54, no?
Copy link
Contributor Author

@KevinBon KevinBon Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a change of behavior in

dom-testing-library/src/__tests__/fake-timers.js

Line 43 in 37ccada

).rejects.toMatchInlineSnapshot(`[Error: always throws]`)
, for a timeout smaller than the interval, it will instead be rejected with [Error: Timed out in waitFor.]
break
}
// It's really important that checkCallback is run *before* we flush
// in-flight promises. To be honest, I'm not sure why, and I can't quite
// think of a way to reproduce the problem in a test, but I spent
// an entire day banging my head against a wall on this.
checkCallback()

if (finished) {
break
}
}
} else {
try {
Expand Down