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

Iterating over Readable streams never resolves with fake timers #2619

Closed
mattdean-digicatapult opened this issue Sep 13, 2024 · 5 comments
Closed

Comments

@mattdean-digicatapult
Copy link

Describe the bug

With sinon v19 and using fake timers Readable streams will never resolve. This causes tests that need to evaluate the contents of Readable streams to timeout. Now it might be I shouldn't expect this to work as I am doing async stuff, but this did work with sinon v18 so for now I'd at least consider this a bug.

To Reproduce
Steps to reproduce the behavior:

  1. setup a fake timer using sinon.useFakeTimers or similar
  2. try to read all the data from a Readable
  3. The final chunk of the Readable will never resolve

I've created a reproduction repo of this https://github.com/mattdean-digicatapult/usefaketimers-bug-reproduction if you'd like to test it out. The test code is as follows:

import { Readable } from "node:stream";

import { expect } from "chai";
import { before, describe, it } from "mocha";

import sinon from "sinon";

describe("sinon bug reproduction", function () {
  let result;

  before(async function () {
    const clock = sinon.useFakeTimers({ now: 100 });
    const stream = Readable.from(Buffer.from("test"));
    const chunks = [];
    for await (const chunk of stream) {
      chunks.push(chunk);
    }
    result = Buffer.concat(chunks).toString("utf8");
    clock.restore();
  });

  it("should run successfully", function () {
    expect(result).to.equal("test");
  });
});

Expected behavior

The stream to finish reading as occurred in sinon v18

Screenshots

Screenshot 2024-09-13 at 10 15 38

Context (please complete the following information):

Additional context

None

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

I am guessing this is related to us now faking all timers by default in version 13 of Fake Timers, which is a breaking change (hence the new major version of Sinon): https://github.com/sinonjs/fake-timers/blob/main/CHANGELOG.md#1300--2024-08-25

Somehow the underlying code you are using might depend on nextTick() or queueMicroTask(). See what happens if you explicitly list the timers to fake toFake: [...] .

Ref sinonjs/fake-timers#323

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

Great testcase. Made it very easy to reproduce and look into. Was exactly what I assumed. This fixes/avoids your issue:

-    const clock = sinon.useFakeTimers({ now: 100 });
+    const clock = sinon.useFakeTimers({ now: 100, toFake: ["setTimeout"] });

EDIT: while it avoids the issue, I find it much better to keep this as an async function and I suggest an alternative below.

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

@SimenB This might be a side-effect you can be aware of in Jest. Not sure how we should document this.

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

Just to add some info. It's the nextTick() timer that never resolves. Instead of explicitly saying you don't want to fake that, you could also of course use runToLastAsync() and use the fact that this is dealing with async work.

I pushed a "fix" to your example: fatso83/usefaketimers-bug-reproduction@54c812d

Hope that helps!

@fatso83 fatso83 closed this as completed Sep 13, 2024
@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

I'll add this as a note to the migration guide: https://sinonjs.org/guides/migration-guide. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants