sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
793 stars 103 forks source link

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

Closed DiegoAndai closed 9 months ago

DiegoAndai commented 1 year ago

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 commented 1 year ago

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: https://github.com/mui/material-ui/pull/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 commented 1 year ago

Just a rough summary to what has been done (see https://github.com/mui/material-ui/pull/37430 for details):

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: https://github.com/sinonjs/sinon/pull/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 commented 9 months ago

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