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

Missing information in documentation about shouldAdvanceTime #390

Closed
stefanschenk opened this issue Jun 16, 2021 · 5 comments
Closed

Missing information in documentation about shouldAdvanceTime #390

stefanschenk opened this issue Jun 16, 2021 · 5 comments

Comments

@stefanschenk
Copy link

  • FakeTimers version : 6.0.1

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
Copy link
Contributor

fatso83 commented Jun 18, 2021

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 3775a00, 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 added a commit to fatso83/lolex that referenced this issue Jun 18, 2021
fatso83 added a commit to fatso83/lolex that referenced this issue Jun 18, 2021
@fatso83
Copy link
Contributor

fatso83 commented Jun 18, 2021

Fix in place. PR coming up

@fatso83
Copy link
Contributor

fatso83 commented Jun 18, 2021

See if #391 does not fix your issue.

  • clone this project
  • run npm install && npm link
  • In your own project, do npm link @sinonjs/fake-timers

fatso83 added a commit to fatso83/lolex that referenced this issue Jun 18, 2021
fatso83 added a commit to fatso83/lolex that referenced this issue Jun 18, 2021
fatso83 added a commit that referenced this issue Jun 21, 2021
All in all, this makes the code a lot easier to read/understand. There was unneeded complexity earlier.

* Create verification test for #390
* Fix issue #390: remove dependency on faking setInterval
@fatso83 fatso83 closed this as completed Jun 21, 2021
@fatso83
Copy link
Contributor

fatso83 commented Jun 21, 2021

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

fatso83 referenced this issue Jun 21, 2021
#102)

lolex can now attach itself to the system timers and automatically advance
mocked time at a certain 'real' interval

- changed uninstall signature to carry over configuration flags in cases where this might be necessary
- updated shouldAdvanceTime functionality to support new install method signature
- moved time delta parameter to config
* Updated Readme.md and History.md
@stefanschenk
Copy link
Author

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

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