fix: Stop calling `waitFor` callback after timeout by KevinBon · Pull Request #1271 · testing-library/dom-testing-library

KevinBon · 2023-10-03T13:33:03Z

What:

Prevent waitFor callback from being invoked even after it resolved, on waitFor timeout.

Why:

More context can be found in this issue, but with that context in mind, it's preventing test side-effects:

afterEach should be enough to "clean" the previous test from any side-effect. However, because of the callback leak issue, window.variable will remain to be 123 even after being set back to 1 for a short period of time.

afterEach(() => {
 window.variable = 1;
})
it('my test', () => {
  return waitFor(() => {
    window.variable = 123;
    throw new Error('I want it to timeout');
  })
})

How:

When the clock is mocked, only call checkCallback when finished is false

Checklist:

Documentation added to the
docs site: N/A
Tests
TypeScript definitions updated: N/A
Ready to be merged

resolves #1270

codesandbox-ci · 2023-10-03T13:34:09Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 10a1ac7:

Sandbox	Source
react-testing-library-examples	Configuration

codecov · 2023-10-03T13:35:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a7b7252) 100.00% compared to head (10a1ac7) 100.00%.
Report is 4 commits behind head on alpha.

Additional details and impacted files

@@            Coverage Diff            @@
##             alpha     #1271   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1038      1041    +3     
  Branches       346       347    +1     
=========================================
+ Hits          1038      1041    +3

Flag	Coverage Δ
node-14	`100.00% <100.00%> (ø)`
node-16	`?`
node-18	`100.00% <100.00%> (ø)`
node-20	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eps1lon

Thanks for working on the fix. However, we should rather make sure that callback is not being invoked again. The proposed fix just hides the issue.

Also: Can you provide a minimal repro against which we can test this fix? Ideally we could extract a unit test from it.

KevinBon · 2023-10-03T17:24:32Z

@eps1lon I changed the solution implementation.

You can find a reproduction example on this repository.

Output example:

It's been a while since I have used jest, so I'm not sure how to test "this behavior" on jest to satisfy the test code coverage

Don't hesitate if you have any other questions 🙇

github-actions · 2023-10-03T17:24:45Z

Uh oh! @KevinBon, the image you shared is missing helpful alt text. Check #1271 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

KevinBon · 2023-10-03T17:48:23Z

I created a branch to show-case that applying this patch will fix this behavior: KevinBon/jasmine-tl-waitfor-leak#1

eps1lon · 2023-10-03T18:24:03Z

src/wait-for.js

    }

    function handleTimeout() {
+      if (finished) {


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

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.

Amended in 6b41483

eps1lon · 2023-10-03T18:24:21Z

src/wait-for.js


    function checkCallback() {
-      if (promiseStatus === 'pending') return
+      if (finished || promiseStatus === 'pending') return


Can we cancel whatever is calling checkCallback instead?

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.

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 👍

Amended in 6b41483

eps1lon · 2023-10-04T08:37:20Z

src/wait-for.js

        })

+        // Could have timed-out
+        if (finished) {


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.

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

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?

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

KevinBon · 2023-10-04T13:33:58Z

src/__tests__/wait-for.js

 test('does not work after it resolves', async () => {
  jest.useFakeTimers('modern')
-  let context = 'initial'
+  const contextStack = []


Or

Suggested change

const contextStack = []

const contextCallStack = []

KevinBon · 2023-10-04T16:21:01Z

@eps1lon I added a reproducible test for jest through ce11c0f

KevinBon · 2023-10-04T16:22:01Z

src/__tests__/wait-for.js

+})

-  expect(context).toEqual('initial')
+test(`when fake timer is installed, on waitFor timeout, it doesn't call the callback afterward`, async () => {


This test ensures that the fix is working

KevinBon · 2023-10-04T16:23:22Z

src/__tests__/wait-for.js

@@ -292,12 +292,11 @@ test('the real timers => fake timers error shows the original stack trace when c

 test('does not work after it resolves', async () => {


I made some changes on this test to help understand the outcome: everything that starts needs to end

KevinBon · 2023-10-10T11:09:48Z

@eps1lon let me know if you need additional information or are unhappy with my changes 🙇

KevinBon · 2023-10-23T13:29:28Z

@eps1lon bumping, let me know if you need anything from me. Thank you!

eps1lon · 2024-01-08T17:50:23Z

Thanks for the patience. Checking this out now so that we can land it ASAP.

eps1lon

Looks great, thank you. I verified the tests are actually failing on main

eps1lon

Looks great, thank you. I verified the tests are actually failing on main

The base branch was changed.

eps1lon · 2024-01-08T20:11:49Z

@all-contributors add @KevinBon for code, bugs

allcontributors · 2024-01-08T20:12:00Z

@eps1lon

I've put up a pull request to add @KevinBon! 🎉

KevinBon mentioned this pull request Oct 3, 2023

Leaking waitFor callback against jasmine when the clock is mocked #1270

Closed

eps1lon requested changes Oct 3, 2023

View reviewed changes

prevent waitFor callback racing condition

a0bb005

KevinBon force-pushed the fix-waitfor-callback-leak branch from 73bbd53 to a0bb005 Compare October 3, 2023 16:18

eps1lon requested changes Oct 3, 2023

View reviewed changes

change logic

6b41483

eps1lon reviewed Oct 4, 2023

View reviewed changes

eps1lon changed the base branch from main to alpha October 4, 2023 08:37

adapt the waitFor resolve test to make it more readable

55ff2f2

KevinBon commented Oct 4, 2023

View reviewed changes

KevinBonHubspot added 3 commits October 4, 2023 09:46

surface timeout + waitFor callback calls

67793f8

adding comment

86c5c08

adding reproductible test

ce11c0f

KevinBon commented Oct 4, 2023

View reviewed changes

KevinBon changed the title ~~Fix waitfor callback leak~~ Fix waitFor callback leak on waitFor timeout Oct 4, 2023

KevinBon requested a review from eps1lon October 4, 2023 17:01

robuster assertion for not calling callback again

10a1ac7

eps1lon changed the title ~~Fix waitFor callback leak on waitFor timeout~~ fix: waitFor callback leak on waitFor timeout Jan 8, 2024

eps1lon changed the title ~~fix: waitFor callback leak on waitFor timeout~~ fix: Stop calling waitFor callback after timeout Jan 8, 2024

eps1lon reviewed Jan 8, 2024

View reviewed changes

eps1lon previously approved these changes Jan 8, 2024

View reviewed changes

eps1lon changed the base branch from alpha to main January 8, 2024 20:10

eps1lon approved these changes Jan 8, 2024

View reviewed changes

allcontributors bot mentioned this pull request Jan 8, 2024

docs: add KevinBon as a contributor for code, and bug #1284

Merged

eps1lon merged commit 9aaf715 into testing-library:main Jan 8, 2024

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

		@@ -292,12 +292,11 @@ test('the real timers => fake timers error shows the original stack trace when c

		test('does not work after it resolves', async () => {

Conversation

KevinBon commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

KevinBon commented Oct 3, 2023

Uh oh!

github-actions bot commented Oct 3, 2023

Uh oh!

KevinBon commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinBon Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinBon Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinBon commented Oct 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinBon commented Oct 10, 2023

Uh oh!

KevinBon commented Oct 23, 2023

Uh oh!

eps1lon commented Jan 8, 2024

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

eps1lon commented Jan 8, 2024

Uh oh!

allcontributors bot commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KevinBon commented Oct 3, 2023 •

edited

Loading

codesandbox-ci bot commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

KevinBon Oct 3, 2023 •

edited

Loading

KevinBon Oct 4, 2023 •

edited

Loading