Skip to content
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

fix: Only use a single clock #966

Merged
merged 3 commits into from
Jun 3, 2021
Merged

fix: Only use a single clock #966

merged 3 commits into from
Jun 3, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 1, 2021

BREAKING CHANGE:

  1. Remove deprecated waitFormDOMChange
  2. The timeout in waitFor(callback, { interval, timeout } ) now uses the same clock as interval
    Previously timeout was always using the real clock while interval was using the global clock which could've been mocked out. For the old behavior I'd recommend waitFor(callback, { interval, timeout: Number.PositiveInfinity }) and rely on your test runner to timeout considering real timers.

TODO:

  • Should Number.PositiveInfinity be the new default for timeout?

What:

Only run with a single clock.

Closes #939
Closes #756
Closes #880

Hopefully also fixes flaky test failures such as https://github.com/testing-library/dom-testing-library/runs/2727330116

Why:

The previous approach breaks jest 27. I would also consider it suprising behavior (agreeing with Closes #880). If I pass waitFor(callback, { interval: 50, timeout: 900 }) then I expect both times to run with the same clock. Previously the behavior of waitFor was different depending on fake or real timers which I consider problematic.

I also don't think we need to force the timeout to run with the real clock. Test runners already implement this behavior. So if people don't want the timeout to use the same clock as interval then they can use waitFor(callback, { timeout: Number.PositiveInfinity }).

How:

Backported f1a8a14 (#962) from #962. This ensures that the new approach works with jest 26 (this PR) and jest 27 (#962)

  • use globally declared setTimout instead of setImmediate and setTimeout from the real clock
  • remove deprecated waitForDOMChange since it relied on the now removed runWithRealTimers

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • [ ] Typescript definitions updated
  • Ready to be merged

/cc @testing-library/all-maintainers Since this might be fairly invasive.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 1, 2021

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 24f082b:

Sandbox Source
react-testing-library-examples Configuration
react-testing-library demo Issue #756

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #966 (24f082b) into main (c273ed5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #966   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        25    -1     
  Lines          966       914   -52     
  Branches       293       282   -11     
=========================================
- Hits           966       914   -52     
Flag Coverage Δ
node-10.14.2 100.00% <100.00%> (ø)
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-15 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/helpers.js 100.00% <100.00%> (ø)
src/wait-for.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86fb094...24f082b. Read the comment docs.

@eps1lon eps1lon added BREAKING CHANGE This change will require a major version bump bug Something isn't working labels Jun 1, 2021
@eps1lon eps1lon changed the base branch from main to alpha June 1, 2021 15:11
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

I'm no expert here, but I can say that the failing tests in the ATL repository are resolved with this change (on Jest 26).
If this gets released, should we also remove the remaining deprecated methods (wait and waitForElement) ?

@eps1lon
Copy link
Member Author

eps1lon commented Jun 1, 2021

If this gets released, should we also remove the remaining deprecated methods (wait and waitForElement) ?

Yep, this PR already targets an alpha branch so we'll collect some more breaking changes if this gets released.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Previously the behavior of waitFor was different depending on fake or real timers which I consider problematic.

This is enough to convince me. The goal of our utilities is to be able to use the async utilities and have all the tests run the same whether the timers are faked or not. And the workaround for the old behavior is pretty sensible IMO.

This looks good to me, and I love all the code that can be removed with this change 👍

src/helpers.js Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Jun 3, 2021

Update: I also noticed flaky test failures such as https://github.com/testing-library/dom-testing-library/runs/2727330116 (also remember one PR) and these instances are hopefully also fixed by this change

@eps1lon eps1lon mentioned this pull request Jun 3, 2021
4 tasks
@kentcdodds
Copy link
Member

Looks like everything's been resolved here and considering this is just going to the alpha branch I'm going to go ahead with the merge 👍

Thanks for the collaboration!

@kentcdodds kentcdodds merged commit 3ae2702 into alpha Jun 3, 2021
@kentcdodds kentcdodds deleted the fix/fake-timers branch June 3, 2021 12:27
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

🎉 This PR is included in version 8.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ValentinH
Copy link

While updating Jest to v27, I faced many broken tests where waitForElementToBeRemoved() was used along with fake timers. After finding this PR, I tried to force the usage of @testing-library/dom@v8.0.0-alpha.1 and it fixed my issues.

As I'm not sure when v8 will be released to the stable channel, I'm wondering if the fix to use a single clock could be back-ported to v7? Or is this fix tightly linked with the removal of waitFormDOMChange and must wait for a major version release ?

@timdeschryver timdeschryver mentioned this pull request Jun 4, 2021
4 tasks
@eps1lon
Copy link
Member Author

eps1lon commented Jun 7, 2021

I'm wondering if the fix to use a single clock could be back-ported to v7?

It includes breaking changes that are not coupled with waitForDOMChange. See item 2 on the breaking changes list in the PR description:

The timeout in waitFor(callback, { interval, timeout } ) now uses the same clock as interval
Previously timeout was always using the real clock while interval was using the global clock which could've been mocked out. For the old behavior I'd recommend waitFor(callback, { interval, timeout: Number.PositiveInfinity }) and rely on your test runner to timeout considering real timers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump bug Something isn't working released on @alpha
Projects
None yet
6 participants