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

Mixed usage of setTimeout and timers.setTimeout #677

Closed
taylor1791 opened this issue Sep 8, 2016 · 10 comments
Closed

Mixed usage of setTimeout and timers.setTimeout #677

taylor1791 opened this issue Sep 8, 2016 · 10 comments

Comments

@taylor1791
Copy link
Contributor

nock/lib/request_overrider.js uses both setTimeout and timers.setTimeout. Should the project use one or the other for the sake of consistency?

Using setTimeout is nice because if a someone is mocking the clock, then nock will use a mocked clock as well. They can still do it, but have to do additional work to mock timers.

@stale
Copy link

stale bot commented Sep 17, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

@stale stale bot added the stale label Sep 17, 2018
@gr2m gr2m added the pinned label Sep 17, 2018
@stale stale bot removed the stale label Sep 17, 2018
@gr2m
Copy link
Member

gr2m commented Sep 17, 2018

That’s interesting, I didn’t even know about Node’s timers module. I agree we should get rid of using the module. This would make a very nice first-timer issue

@gr2m gr2m added the refactor label Sep 17, 2018
gr2m pushed a commit that referenced this issue Dec 14, 2018
…ediate

When unit testing, it is not uncommon to mock the clock. This includes
controlling when setImmediate fires. By using global.setImmediate, we
adhere to this expectation. This also avoids consumers having to mock
the timers module.

Resolves #677
@nockbot
Copy link
Collaborator

nockbot commented Dec 18, 2018

🎉 This issue has been resolved in version 11.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nockbot
Copy link
Collaborator

nockbot commented Dec 23, 2018

🎉 This issue has been resolved in version 10.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Chengxuan
Copy link
Contributor

Chengxuan commented Dec 31, 2018

This could have been communicated better, our tests started failing after picked up 10.0.5 because most of them use fakeTimer which didn't impact nock. Now they all timing out and it took me a while to figure out why. I agree to give more control is better, but I don't think most of existing users know it uses a separate timer due to existing behaviour. They'll need to go through the same debugging process as I did now.

Maybe update your readme with an example would help? This is a change of behaviour for all the existing tests uses fakeTimer. Those tests will start failing with no obvious error other than timing out and it would take a while to track down to the nock library.

@RichardLitt
Copy link
Member

@Chengxuan Well articulated! I think an example would be great. We're sorry about how this inconvenienced you.

@Chengxuan
Copy link
Contributor

FYI @RichardLitt more discussion have happened in #1334.

@RichardLitt
Copy link
Member

Ah, sweet. Closing this.

@lock
Copy link

lock bot commented Jan 16, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 2019
@nockbot
Copy link
Collaborator

nockbot commented Aug 13, 2019

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

5 participants