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 1 commit
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

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: 7 additions & 1 deletion src/wait-for.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ function waitFor(
}

function onDone(error, result) {
if (finished) {
return
}
finished = true
clearTimeout(overallTimeoutTimer)

Expand Down Expand Up @@ -134,7 +137,7 @@ function waitFor(
}

function checkCallback() {
if (promiseStatus === 'pending') return
if (finished || promiseStatus === 'pending') return
Copy link
Member

Choose a reason for hiding this comment

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

Can we cancel whatever is calling checkCallback instead?

Copy link
Contributor Author

@KevinBon KevinBon Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would love to, but I'm afraid it might introduce breaking changes.

We'll need to move the if(finished) before checkCallback here: https://github.com/testing-library/dom-testing-library/pull/1271/files#diff-68584107d5f50eb95702a47b23bb6e0f6c2d78f9bab7456a1a4150c79229db78R84-R92

But the following comment is daunting:

dom-testing-library/src/wait-for.js

Lines 84 to 87 in a0bb005

// 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually https://github.com/testing-library/dom-testing-library/pull/1271/files#diff-68584107d5f50eb95702a47b23bb6e0f6c2d78f9bab7456a1a4150c79229db78R84-R92 is un-related, so I should move the if(finished) before checkCallback 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended in 6b41483

try {
const result = runWithExpensiveErrorDiagnosticsDisabled(callback)
if (typeof result?.then === 'function') {
Expand All @@ -160,6 +163,9 @@ function waitFor(
}

function handleTimeout() {
if (finished) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we cancel the timeout instead when we finish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we finish, onDone should have been called with clearTimeout (src), therefore I don't really think this one is needed, but it's more for extra-safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended in 6b41483

return
}
let error
if (lastError) {
error = lastError
Expand Down