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

Mock timers broken with 20.12.0 / 21.7.0 #52325

Closed
phryneas opened this issue Apr 2, 2024 · 4 comments
Closed

Mock timers broken with 20.12.0 / 21.7.0 #52325

phryneas opened this issue Apr 2, 2024 · 4 comments
Labels
test_runner Issues and PRs related to the test runner subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@phryneas
Copy link

phryneas commented Apr 2, 2024

Version

v20.12.0 v21.7.0

Platform

Darwin ApolloMac.lan 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000 arm64 arm Darwin

Subsystem

No response

What steps will reproduce the bug?

This test repeatedly fails in Node v20.12.0 and v21.7.0, but works in v21.11.1 and v21.6.1:

https://github.com/apollographql/apollo-client-nextjs/blob/pr/test-changes/packages/client-react-streaming/src/AccumulateMultipartResponsesLink.test.ts#L84-L170

The line that fails is https://github.com/apollographql/apollo-client-nextjs/blob/a4e19844a05d6933a4454ff80ec784394525dc7f/packages/client-react-streaming/src/AccumulateMultipartResponsesLink.test.ts#L145

mock.timers.tick(2);

which triggers a callback scheduled by setTimeout here.

How often does it reproduce? Is there a required condition?

It fails on almost every run.
I believe in CI I could retry multiple times to make it succeed at some point, but locally it fails every time.

What is the expected behavior? Why is that the expected behavior?

The test passes in older versions without a problem.

What do you see instead?

✖ `next` call will be debounced and results will be merged together (0.971291ms)
  TypeError [Error]: Cannot read properties of undefined (reading 'id')
      at MockTimers.tick (node:internal/test_runner/mock/mock_timers:655:25)
      at TestContext.<anonymous> (file:///Users/tronic/tmp/apollo-next/packages/client-react-streaming/src/AccumulateMultipartResponsesLink.test.ts:22:793)
      at Test.runInAsyncScope (node:async_hooks:206:9)
      at Test.run (node:internal/test_runner/test:641:25)
      at Test.start (node:internal/test_runner/test:550:17)
      at startSubtest (node:internal/test_runner/harness:218:17)
      at async file:///Users/tronic/tmp/apollo-next/packages/client-react-streaming/src/AccumulateMultipartResponsesLink.test.ts:12:543

Additional information

I'm running these tests through tsx, but I believe this doesn't play a role here.

This is the run command:

NODE_OPTIONS="$NODE_OPTIONS --conditions=browser" TSX_TSCONFIG_PATH=./tsconfig.tests.json node --import tsx/esm --no-warnings --test src/AccumulateMultipartResponsesLink.test.ts

You can also observe this happening in our CI here so I believe this problem is not Darwin-specific.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 2, 2024

This test repeatedly fails in Node v20.12.0 and v21.7.0, but works in v21.11.1 and v21.6.1

There were two changes to mock timers in those new releases: #51809 and #51673. My guess would be that #51673 is the culprit.

As an aside, I don't believe the extra awaits added in apollographql/apollo-client-nextjs#266 are necessary. As a general rule, you should only need to await tests if you are inside of another test.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 2, 2024

I haven't worked on the mock timers code before, so cc @Benricheson101 and @benjamingr, but the problem seems to be that this line assumes that afterCallback always exists. When it doesn't exist, we get the error in the OP. Fixing that line seems straightforward, but we should include a test for it as well.

@phryneas
Copy link
Author

phryneas commented Apr 2, 2024

Thanks for looking into this!

As an aside, I don't believe the extra awaits added in apollographql/apollo-client-nextjs#266 are necessary. As a general rule, you should only need to await tests if you are inside of another test.

And thanks for this as well, good to know!

@Benricheson101
Copy link
Contributor

I haven't worked on the mock timers code before, so cc @Benricheson101 and @benjamingr, but the problem seems to be that this line assumes that afterCallback always exists. When it doesn't exist, we get the error in the OP. Fixing that line seems straightforward, but we should include a test for it as well.

Good catch. I see what's happening, this would definitely be a pretty easy fix. I can go ahead and open a fix for it.

@VoltrexKeyva VoltrexKeyva added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. test_runner Issues and PRs related to the test runner subsystem. labels Apr 3, 2024
@MoLow MoLow closed this as completed in 3f8cc88 Apr 11, 2024
marco-ippolito pushed a commit that referenced this issue May 2, 2024
PR-URL: #52332
Fixes: #52325
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
PR-URL: #52332
Fixes: #52325
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants