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
797 stars 104 forks source link

Missing information in documentation about shouldAdvanceTime #390

Closed stefanschenk closed 3 years ago

stefanschenk commented 3 years ago

I am using fake-timers for some time now and I wanted to utilize the shouldAdvanceTime option. But I did not get it to work from the start.

This was my install code;

const installFakeClock = (): void => {
  (window as any).fakeClock = install({
    now: new Date(),
    toFake: ['setTimeout', 'clearTimeout'],
    shouldAdvanceTime: true,
  });
};

According to the documentation, this should work. Set shouldAdvanceTime to true and it will use the default time delta of 20ms. However, it would not work. The fake timer was not advancing automatically.

I had to look through the source to see that it actually also expects setInterval and clearInterval to be faked too.

I'm not sure why the advanceTime needs set and clearInterval to be faked too, but it was not expected. If it is needed, could you change the documentation to reflect that the setInterval and clearInterval are needed for this option to work?

fatso83 commented 3 years ago

Spent some time looking into this to understand what was going on. Pasted my runnable experiment here: https://runkit.com/fatso83/60ccaae42e7eab001a381fe7

I merged this feature (#102) 4 years ago in https://github.com/sinonjs/fake-timers/commit/3775a00bf1cc91acc11a195a24b35cf14ab1217d, but I never really used it myself, so I was a bit unfamiliar with it and have long since forgotten how it is implemented, but you are indeed right in that it implicitly relies on stubbing setInterval.

I am not totally sure why, though, as it seems like it only calls through to the real/original version, so the clock.methods[i] === "setInterval" bit should be possible to remove without affecting the functionality. Will have a go at this.

fatso83 commented 3 years ago

Fix in place. PR coming up

fatso83 commented 3 years ago

See if #391 does not fix your issue.

fatso83 commented 3 years ago

Did not hear back, but this should make the actual implementation be in line with the docs, so no doc update needed 😄

stefanschenk commented 3 years ago

@fatso83 Thanks for looking into it and creating a PR in such short time.