Sitelet https://github.com/nodejs/node/issues/52325
Skip to content

Mock timers broken with 20.12.0 / 21.7.0 #52325

Closed
Closed
Mock timers broken with 20.12.0 / 21.7.0#52325
Labels
test_runnerIssues and PRs related to the test runner subsystem.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

Description

@phryneas
phryneas
opened on 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.

Activity

  1. cjihrig commented on Apr 2, 2024

    @cjihrig
    cjihrig
    on Apr 2, 2024
    Contributor

    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-integrations#266 are necessary. As a general rule, you should only need to await tests if you are inside of another test.

  2. cjihrig commented on Apr 2, 2024

    @cjihrig
    cjihrig
    on Apr 2, 2024
    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.

  3. phryneas commented on Apr 2, 2024

    @phryneas
    phryneas
    on Apr 2, 2024
    ContributorAuthor

    Thanks for looking into this!

    As an aside, I don't believe the extra awaits added in apollographql/apollo-client-integrations#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!

  4. Benricheson101 commented on Apr 2, 2024

    @Benricheson101
    Benricheson101
    on Apr 2, 2024
    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.

  5. VoltrexKeyva
    added
    timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().
    test_runnerIssues and PRs related to the test runner subsystem.
    on Apr 3, 2024
  6. MoLow
    closed this as completedin 3f8cc88on Apr 11, 2024
  7. pull
    added a commit that references this issue on Apr 11, 2024

    test_runner: fix clearing final timeout in own callback

    3f8cc88
  8. marco-ippolito
    added 2 commits that reference this issue on May 2, 2024

    test_runner: fix clearing final timeout in own callback

    048bf79

    test_runner: fix clearing final timeout in own callback

    9497097
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    test_runnerIssues and PRs related to the test runner subsystem.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions