-
Notifications
You must be signed in to change notification settings - Fork 230
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 #631, add jestFakeTimersAreEnabled and use it to detect faketimer in createTimeoutController #688
Conversation
✔️ Deploy Preview for react-hooks-testing-library ready! 🔨 Explore the source changes: d623572 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-hooks-testing-library/deploys/614738594b33ad0008148c1f 😎 Browse the preview: https://deploy-preview-688--react-hooks-testing-library.netlify.app |
Codecov Report
@@ Coverage Diff @@
## alpha #688 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 16 +1
Lines 244 264 +20
Branches 32 38 +6
=========================================
+ Hits 244 264 +20
Continue to review full report at Codecov.
|
b4849bc
to
4452982
Compare
add new fakeTimer test and revise the function add advanceTime revise the advanceTime use jest.advanceTimersByTime change timeout type fix converage and revise type
@chris110408 I've just pushed some more tests for the fake timers test suite (they are the same as the regular async utils, except one). Theoretically all these tests should pass, when fake timers are enabled. With your current changes, most are timing out (the CI agents are not handling this very well as it's taking a long time for all of the tests to time out). |
@chris110408, you probably got a notification about it (but in case anyone is reading along) but I've pushed up a branch with most of the suggested changes here and a modified advance timers implementation that passes all the tests (at least locally for me). |
Thank you. I will look at those change today |
) | ||
}) | ||
|
||
test('should not reject when waiting for next update if timeout has been disabled', async () => { |
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.
@mpeyper The skip has been removed and I made some other revise to make this test pass
https://github.com/testing-library/react-hooks-testing-library/pull/688/files#r705710781
https://github.com/testing-library/react-hooks-testing-library/pull/688/files#r705708190
…testing-library#689) Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 4.30.0 to 4.31.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/parser/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.31.0/packages/parser) --- updated-dependencies: - dependency-name: "@typescript-eslint/parser" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ry#690) Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 4.30.0 to 4.31.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.31.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Nice. Is there any thing I need to double check before we merge this PR? |
I need to properly review it (spent most of my time looking at the build issue the other night), but I'm thinking of putting this out in a beta release first. |
Thanks a lot @chris110408. I've made a few, most cosmetic changes. The biggest change was no using The only other thing I noticed that might have been an issue (no tests were failing) was that timers were being advance in both the timeout and interval controllers since they were moved out of the the I'm going to push out an alpha build for this now. |
🎉 This PR is included in version 8.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Just wondering if we need some a note in the async utils docs about fake timers? |
I've got a fairly complicated real world example that I thought would be fixed by this change, but after testing out the alpha it's still not working. I'll try to get a boiled down version onto a test to help work through it, but so far it appears as though the microtask queue is not flushing as we advance timers. I'm not sure why this is occurring for the setup yet, but wanted to drop a comment here in case anyone else tries it with a more real world example and is also running into issues. |
@mpeyper Could you please add me to the code contributor list? |
I could work on that If we need to |
will look at it this weekend |
I did, it's just in the alpha branch for now.
Thanks. Just target the alpha branch if it's not in main when you make the PR.
Hopefully I will have boiled down the example by then. I'll post it here if I do. I'm actively working on this too as the hook/test in question is part of my day job. |
Thank you |
So after a few hours of replacing portions of this very complex hook setup, I've managed to narrow it down to an API call being made with I'm out of steam today to look at this anymore, but I'll try to setup a small example just with |
Hi! thanks for fixing this! We are facing this issue, when are we planning to release it? thanks 🙇 |
Hi @icatalina, I'm not sure this will ever see a full release at this point, given what is happening over in RTL. I believe |
Ohhh!! I had no idea! thanks heaps for pointing that out! :) |
What:
Fixes #631, add jestFakeTimersAreEnabled and use it detect the useFakeTimer.
when useFakeTimer has been detected and jest.advanceTimersByTime will advance time to the timeout-1
test:'should waitFor arbitrary expectation to pass when fake timers are not advanced explicitly' has been added
Why:
we need to add jestFakeTimersAreEnabled function the same function used on
@testing-library/dom
and use it to detect the fake timer and use advanced time to make sure time is not stuck
How:
The problem was when the fake timer is used. the time will not move forward unless using the advanced time.
#682
createTimeoutController
has been used to replacePromise.race
.the problem is to detect the fake timber and use advance time to move time forward to the last ms until the timeout.
Checklist: