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

Using sinon.useFakeTimers prevents doc from saving (in 5.0) #6074

Closed
paglias opened this issue Jan 31, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@paglias
Copy link
Contributor

commented Jan 31, 2018

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Noticed some tests failing, turns out that after upgrading to 5.0.x using sinon.useFakeTimers (http://sinonjs.org/releases/v4.2.2/fake-timers/) is the cause.

If the current behavior is a bug, please provide the steps to reproduce.
https://gist.github.com/paglias/943c6f3c3efbce095c0e6257eac9b4ae -> Using mongoose 4 it works correctly, with version 5 it doesn't unless you comment out line 12. I've tried with past and future dates and even enabling mongoose debugging but there's no info at all.

From testing I noticed that it arrives up to the pre validate hooks but not further

What is the expected behavior?
It should save docs with sinon.js and useFakeTimers

Please mention your node.js, mongoose and MongoDB version.
node 6 or 8
mongo 3.4
mongoose

@vkarpov15 vkarpov15 added this to the 5.0.3 milestone Feb 1, 2018

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

commented Feb 1, 2018

Thanks for reporting, will fix ASAP 👍

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

commented Feb 1, 2018

Yeah this is a feature of sinon, useFakeTimers() stubs out setImmediate(), so when mongoose uses setImmediate() internally everything goes straight to hell. In my experience, the practice of stubbing out language-specific global state like useFakeTimers() is one to avoid because you never know how a library will react to things like setImmediate() never calling its callback, and it isn't something libraries test for.

Since sinon is so popular, we'll switch back to using process.nextTick() everywhere because sinon doesn't stub that out by default.

vkarpov15 added a commit to vkarpov15/kareem that referenced this issue Feb 1, 2018

@vkarpov15 vkarpov15 closed this in ab59f86 Feb 1, 2018

@paglias

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

Thanks! Unfortunately we have some stuff that relies a lot on dates and sinon is quite easy to simulate them. We could probably switch to passing a date parameter to functions but when you have a lot of nested functions it becomes messy

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2018

@paglias granted. What I usually do is just have a time.js file that exports a now() function that sinon can stub, and place new Date() usage with time.now(). Makes things a little cleaner because tweaking global behavior that other libs might rely on is dangerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.