-
Notifications
You must be signed in to change notification settings - Fork 233
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
waitFor
doesn't work if jest fake timers are used
#631
Comments
When I was reading this I was thinking the only way I could think of solving it would be to query jest about the timers, so it's interesting that that's effectively how they solved it in Unfortunately we can use the same function as we do not depend on it, or the DOM in general, which their solution uses. That said, the way we If anyone is interested in working on the fix for this, I'm more than happy the help you out. |
@mpeyper
function jestFakeTimersAreEnabled() {
return hasJestTimers()
? usedFakeTimers
:false
}
function hasJestTimers() {
return (
typeof jest !== 'undefined' &&
jest !== null &&
typeof jest.useRealTimers === 'function'
)
}
const usedFakeTimers = Object.entries(timerAPI).some(
([name, func]) => func !== globalObj[name],
)
const globalObj = typeof window === 'undefined' ? global : window
let setImmediateFn = globalObj.setImmediate || setImmediatePolyfill,
function setImmediatePolyfill(fn) {
return globalObj.setTimeout(fn, 0)
}
const wait = async (callback: () => boolean | void, { interval, timeout }: WaitOptions) => {
....
const waitForFakeTimer=async (interval)=>{
let finished = false
while (!finished) {
if (!jestFakeTimersAreEnabled()) {
throw new Error(
`Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to real timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`,
)
}
jest.advanceTimersByTime(interval)
await new Promise(r => setImmediate(r))
}
}
....
if (!checkResult()) {
....
await act(() => Promise.race([waitForResult(), timeoutPromise(),usingJestFakeTimers && waitForFakeTimer(interval)].filter(Boolean)))
} else {
await act(() => Promise.race([waitForResult(),usingJestFakeTimers && waitForFakeTimer(interval)].filter(Boolean)))
}
} |
Sorry for the delay in responding @chris110408. Any contributions towards this are very welcome. I think taking a similar approach to My only other thought is the inclusion of |
Thanks @mpeyper I will work on the PR |
I have been trying to find the fix for a typescript error. |
Give this a shot: Object.entries(timerAPI).some(
([name, func]) => func !== globalObj[name as keyof typeof timerAPI]
)
Hope this helps. |
It works |
@mpeyper const usedFakeTimers = ()=>!!(global.setTimeout.mock||global.setTimeout.clock) However, I have a hard time to understand the issue, import { renderHook } from '@testing-library/react-hooks'
it('does not work', async () => {
jest.useFakeTimers()
const { waitFor } = renderHook(() => {})
await waitFor(() => {
console.log('poll') // Is printed just once
expect(false).toBe(true)
}, { timeout: 25, interval: 10 })
// Fails with Exceeded timeout of 5000 ms for a test.
}) this test will never pass no matter we use useFakeTimers or not. and I change the test code to it('useFakeTimers test', async () => {
jest.useFakeTimers()
const { waitFor } = renderHook(() =>{})
let actual = 0
const expected = 1
setTimeout(() => {
actual = expected
}, 200)
// show Fails with Exceeded timeout of 5000 ms for a test. if we do not use jest.advanceTimersByTime(200)
jest.advanceTimersByTime(200)
let complete = false
await waitFor(() => {
expect(actual).toBe(expected)
complete = true
},{ interval: 50 })
expect(complete).toBe(true)
}) our test library does not have any problem running it. Could you please help me confirm what is our problem here? Should I just add this unit test to prove waitFor works when using jest.useFakeTimers() ? |
To be honest, I don't use fake timers when I write tests, so I'm also unsure of what the desired outcome is. I think the difference you have here is @DennisSkoko is there more info you can provide to help @chris110408 out? |
I add another test that uses the runOnlyPendingTimers and useFakeTimer and both test successfully pass |
Now I'm even more confused about what the actual issue is here. There must be a reason dom-testing-library has such an implementation? Do they want it to work automagically without explicitly advancing the timers? (Some of the samples provided imply this) |
I am also confused about the actual issue. Yes, the dom-testing-library solution is useFakeTimer and without explicitly advancing the timers? in the test. However, I think it is comment practice to use advanceTimersByTime when use useFakeTimeber in the test. (I have not to use useFakeTimer before. my assumption was made by reading the Jest document the timer mock is used for setting up a testing environment not depend on real-time to elapse and jest mock timer functions that allow you to control the passage of time ) and I am not sure why they make it work automagically without explicitly advancing the timers? Please let me know if you want me to make the |
Hi @chris110408 @mpeyper, sorry for the late response. From my point of view, what is happening is that the implementation of
I think the reason this works fine is that the it('asd', async () => {
jest.useFakeTimers()
const fn = jest.fn().mockReturnValueOnce(false).mockReturnValueOnce(true)
const { waitFor } = renderHook(() => {})
await waitFor(() => {
expect(fn()).toBe(true)
})
}) Here we can see that test cases fails with a Jest timeout. Reason being that Hopefully this provides the information you are looking for. |
Yep, that makes sense to me. Just to clarify, no amount of running pending timers or advancing them before the What if the run/advance was in the |
For that test case the the run/advance was in the |
We're seeing what I believe is a related issue where
This also appears to be related to fake timers -- but in this case with the |
Sorry I was distract by my work and personal stuff. I will work on this issue this weekend. |
it looks Promise.race() will behave unexpectedly when use with fake timer jestjs/jest#10258. |
Interesting. There was also another issue raised in discord around our use of |
Yes Thanks for the note we need to know why we use const safeCallback to fix this issue |
await waitFor(() => {
expect(result.current.example).toBe(somethingExpected)
}) The intent is to wait until the callback stops throwing an error. We also support waiting for a truth value to be returned: await waitFor(() => result.current.isReady)
|
in this case, no mater what error we have we will always get a timeout error. is that an expected behavior?https://github.com/testing-library/react-hooks-testing-library/blob/main/src/core/asyncUtils.ts#L70 |
Yes, it either passes or times out trying. |
Since the example @DennisSkoko provided is an expected behavior, should we continue work on this issue? |
Does this mean the expected behavior for |
@chris110408 Yes, I think there is still work to be done here. Using fake timers stills fails in the scenario described here. @bsk26 I would not expect that test to pass, regardless of the use of fake timers or not. The callback gets called synchronously once before using timeouts to poll for subsequent checks. With the current implementation, I'd expect the test to timeout with our error if not using fake timers, or jest's timeout if you are. Can you please that you are const { result, waitFor } = renderHook(
...
);
await waitFor(() => {
expect(false).toBe(true);
});
I assume you mean other than us not supporting the use of fake timers (yet)? There has been a bit of deviation in our implementations which is not ideal, but for the most part the core use case is consistent, that is, it will run the callback on an interval until it stops throwing an error. We implemented it to also work of returning a boolean (unsupported by const { result, waitFor } = renderHook(() => useSomething())
await waitFor(() => !result.current.loading)
expect(result.current.data).toBe(...) I suspect that this use case is less useful to The other main difference is they now support returning a There are also some subtle differences in that they they run additional checks with a Fundamentally though, the implementation is different because our use cases and our dependencies are different. We try to offer the best utilities specifically for testing hook functions, rather than ensuring 100% API consistency with the libraries designed for testing DOM output. |
@mpeyper the createTimeoutController is awesome and I am able to fix this issue. if (timeout) {
timeoutId = setTimeout(() => {
timeoutController.timedOut = true
timeoutCallbacks.forEach((callback) => callback())
resolve()
}, timeout)
if (jestFakeTimersAreEnabled()) {
jest.advanceTimersByTime(timeout-1)
}
} however, there is one typescript lint error I am not sure how to solve and need your help. This function was cloned from export const jestFakeTimersAreEnabled = () => {
/* istanbul ignore else */
if (typeof jest !== 'undefined' && jest !== null) {
return (
// legacy timers
setTimeout._isMockFunction === true ||
// modern timers
Object.prototype.hasOwnProperty.call(setTimeout, 'clock')
)
}
// istanbul ignore next
return false
} However, it looks the global setTimeout does not have the props of _isMockFunction, and pop out the following lint error Could you please help me provide some suggestions for how to handle this lint error? I should be able to close this ticket after I could handle this lint error. :) |
Hi @chris110408, You can use When making these changes, please add this test to the fake timer tests: test('should waitFor arbitrary expectation to pass when fake timers are not advanced explicitly', async () => {
const fn = jest.fn().mockReturnValueOnce(false).mockReturnValueOnce(true)
const { waitFor } = renderHook(() => null)
await waitFor(() => {
expect(fn()).toBe(true)
})
}) (and a bunch of other cases probably) |
#688 PR has been created to close this ticket |
Thanks @chris110408. We'll move implementation discussion to the PR now. I'll try to find time to take a look later today. |
…waiting Fixes #631 * add jestFakeTimersAreEnabled and use it to detect faketimer in createTimeoutController (#688) * fix fakeTimer problem * add new fakeTimer test and revise the function * add advanceTime * revise the advanceTime * use jest.advanceTimersByTime * change timeout type * fix converage and revise type * test(fake-timers): add more tests to test suite for fake timers * fix the code after code review and clean up * fix the test timeout is false * clean up * fix coverage * add skip for pass checkers * add comment * test(async-utils): enable test to test CI fix * test(async-utils): combine fake timer tests with async tests * refactor(async-utils): Move DEFAULT_TIMEOUT out of timeout controller * refactor(async-utils): move fake timer advancement into seperate utility * refactor(async-utils): simplify fake timer advancement logic * docs: add chris110408 as a contributor for code * refactor(async-utils): only advance timers on a single timeoutController BREAKING CHANGE: tests that used to manually advance fake timers and use async utilities may now fail as timer would advance further Co-authored-by: Lei Chen <[email protected]> Co-authored-by: Michael Peyper <[email protected]>
🎉 This issue has been resolved in version 8.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I'm very surprised no one has mentioned to tell jest not to fake other things it fakes. This is an exhaustive list of things jest fakes when using Can someone try |
Hey all, seems there was a PR merged, but into the alpha branch and not main ... is this at some point planned to be released ? |
The implementation in the alpha branch has some issues and I never found time to get to the bottom of it. It’s unlikely we’ll ever merge that into the main branch now. For what it’s worth, the new RTL version already supports fake timers. I’m not sure about the new RNTL version though. |
Very good pointer @joeythomaschaske 🎯 Indeed, For Vitest users, the API is a bit better as you can directly specify the only thing you want to fake. For example: |
I wanted only
|
Just had to update RTL, which resolved it for me. |
I've been using this solution, but depending on what you want it might not be all that useful async function waitFor(cb) {
let success = false;
do {
try {
success = cb();
} catch {}
jest.runOnlyPendingTimers();
await Promise.resolve();
} while (!success);
} |
Thanks @enzoferey!!! |
As a workaround, you can switch back to using real timers immediately before invoking e.g. // Arrange
jest.useFakeTimers();
// Act
doStuffThatTakesALongTime();
await jest.advanceTimersByTimeAsync(MAX_EXPECTED_TIME); // Fast forward
// Assert
jest.useRealTimers(); // Switch back to real timers so that we can use waitFor()
await waitFor(() => expect(inputControl).toBeEnabled()); |
@flakey-bit approach worked for me even without the |
This is a bit disappointing. Especially since the changelog between 8.0.0-alpha.1 and 8.0.0 did not mention that this was effectively removed from the v8 major version. We just spent time upgrading from v7 to v8 for the sole purpose of getting this fix just to find out that it's not there.
I don't see how this is relevant. The new RTL versions all require upgrading to react v18 which is no small ask. |
My apologies @jcready for the waste of your time. This library has been effectively deprecated for quite some time now. While I understand upgrading to React 18 is no small feat, I do feel like it was a relevant thing for me to point out as many of my users would be looking to make that upgrade and along with it would likely need to migrate away from this library anyway. After all, React 18 has been out now for almost 2 years now. All that being said, I’ll update the release notes to mention that it was not included to hopefully save someone else from wasting their time also. |
I'm not sure if you've actually read through that issue thread, but on top of the major react 18 changes it sounds like we'd have to switch to an utterly crippled version of
React 18 is the Python 3 of version changes. |
Oh, I definitely had opinions about the changes, but ultimately I was not prepared to continue supporting an offshoot of functionality from the main RTL version which would largely serve as a source of confusion to many users. Admittedly, I don’t think we have done a good enough job in actually deprecating this library and thats largely been due to changes in my life distracting me (I don’t work with react or even JavaScript anymore) and the burnout I was feeling (largely caused by the thread I linked above) in actually supporting a dying library. I suggest if the new version is missing features that you use, and there is a good use case for them, raise an issue in their repo to add support for it. |
react-hooks-testing-library
version: 7.0.0react
version: 17.0.2react-dom
version: 17.0.2node
version: 14.16.0npm
version: 7.10.0Problem
When using
waitFor
when Jest has been configured to use fake timers then thewaitFor
will not work and only "polls" once. After that the test just hangs until Jest comes in and fails the test with that the test exceeds the timeout time. Below is some code that showcases the problem.Basically the
waitFor
from@testing-library/react-hooks
is using the fakedsetTimeout
orsetInterval
which prevents it from working correctly.There is a workaround (see suggested solution) but I recommend providing a nice error message when
waitFor
is used together with faked timers or maybe change the implemenation so it will work with fake timers.Suggested solution
I found this issue and it seems that person has already been fixed in
@testing-library/dom
. From my perspective I can suggest maybe reuse that function instead of implementing it yourselves but I don't really know the internal structure / code.But after finding that issue and realizing that is has been fixed there, then I use the following code as a workaround which works fine.
A more real world scenario
If curios on the actual problem I'm facing is to test the following hook:
What I want to do is test that it invokes the
onSuccess
function on a successfull poll.The text was updated successfully, but these errors were encountered: