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

NodeJS timers module mock causing tests to timeout in JSDom #470

Closed
DiegoAndai opened this issue Jun 8, 2023 · 3 comments
Closed

NodeJS timers module mock causing tests to timeout in JSDom #470

DiegoAndai opened this issue Jun 8, 2023 · 3 comments

Comments

@DiegoAndai
Copy link

We have several tests timing out when updating sinon to 15.1.0. After an initial investigation, the identified cause was fake-timers version 10.2.0, specifically the timers module mock change, but don’t know why it caused the tests to fail.

You can check the PRs for sinon update here: MUI PR, MUI X PR. The failed tests are listed there.

What did you expect to happen?

Tests were running ok before fake-timers version 10.2.0, version 10.1.0 works fine.

What actually happens

Several tests listed are timing out, for example, the ones listed above.

How to reproduce

A minimal test for our codebase that passes before the upgrade and fails after:

it('test', async () => {
  await act(() => {
    return new Promise((resolve) => {
      setTimeout(resolve, 100);
      clock.runAll();
    });
  });
  expect(true).to.equal(true);
});

Fails with the following error:

Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  at listOnTimeout (internal/timers.js:557:17)
  at processTimers (internal/timers.js:500:7)

The other examples listed above are failing with the same error.

Thanks in advance for checking this out! 😊

@fatso83
Copy link
Contributor

fatso83 commented Jun 12, 2023

Seems like the new native Node timers feature broke some installs.

cc @swenzel-arc and @benjamingr

I think we might want to deprecate this version and re-release as a new major to avoid this while we figure out the details of what is happening.

@DiegoAndai I wrote you directly in one of the PRs you referenced: mui/material-ui#37430 I analyzed the situation a bit to figure out the likely suspects. I do not have a fix without looking more into it, but suggested something to look at.

@fatso83
Copy link
Contributor

fatso83 commented Jun 22, 2023

Just a rough summary to what has been done (see mui/material-ui#37430 for details):

  • verified that the MUI issue was reproducible (does not happen in 10.1, but does happen in 10.2)
  • did not see what causes the issue specifically (requires work to be put in)
  • deprecated NPM package version 10.2
  • released a new major version 11 - no functional changes, but to signal a breaking change
  • released 10.3 as a re-tagged 10.1 to fix Sinon installs that still chose to install 10.2
  • released new version of Sinon 15.2 that explicitly relies on 10.3 (to avoid the 10.2 issue)

If we depend on version 11 in Sinon, that will be a breaking change.

The reason the linked PR still fails is due to an unrelated change that ironically was to improve webpack compatibility: sinonjs/sinon#2519

I don't think this change validates a new major version, as we usually care about API changes, not bundling compatibility, with regards to major releases.

@fatso83
Copy link
Contributor

fatso83 commented Sep 25, 2023

Closing this as I have no idea what is wrong and if something is wrong, and there exists a workaround.

@fatso83 fatso83 closed this as completed Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants