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

Unable to use shouldAdvanceTime and advanceTimeDelta options. #122

Closed
royalpinto opened this issue Jul 18, 2017 · 9 comments · Fixed by #123
Closed

Unable to use shouldAdvanceTime and advanceTimeDelta options. #122

royalpinto opened this issue Jul 18, 2017 · 9 comments · Fixed by #123

Comments

@royalpinto
Copy link

Env Details:
lolex: 2.1.0
node: v6.9.2

Following script throws an error:

var lolex = require('lolex');
var clock = lolex.install({shouldAdvanceTime: true, advanceTimeDelta: 40});
setTimeout(() => console.log(1), 4000);

Error:

1
_stream_writable.js:396
  if (state.length === 0 && state.needDrain) {
           ^

TypeError: Cannot read property 'length' of undefined
    at onwriteDrain (_stream_writable.js:396:12)
    at afterWrite (_stream_writable.js:386:5)
    at runJobs (/Users/lohith.pinto/Development/stark/node_modules/lolex/src/lolex-src.js:183:18)
    at Object.tick (/Users/lohith.pinto/Development/stark/node_modules/lolex/src/lolex-src.js:550:9)
    at doIntervalTick (/Users/lohith.pinto/Development/stark/node_modules/lolex/src/lolex-src.js:411:11)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)

What could be the issue ?

@fatso83
Copy link
Contributor

fatso83 commented Jul 18, 2017

Thanks for your bug report. This was quite unfortunate.

@elad-nach, any ideas?

@acud
Copy link
Contributor

acud commented Jul 18, 2017

@fatso83 I'm just looking into this as I'm trying to integrate 2.1.0 into sinon.

I'm also getting sporadic errors there - regardless to whether lolex is installed with shouldAdvanceTime = true. Errors are shown on mocha in the XHR test section on sinon.
I'm investigating in any case. will update once I nail it.

@acud
Copy link
Contributor

acud commented Jul 18, 2017

@fatso83 @benjamingr
could you also please have a second look at the nextTick() feature that was merged? The runJobs was added on that PR.
see here and here.

@benjamingr
Copy link
Member

The error makes sense - the user overrode process.nextTick and did not uninstall the timer - Node was trying to flush the ticks and lolex was there.

I'll see if I can hack around it (by mimicing nextTick better), the obvious fix is to advance the clock synchronously and then uninstall.

A general way to reproduce would be:

lolex.install({}); // 
startSomeAsyncNodeAction((err, data) => {
 // startSomeAsyncNodeAction might call nextTick
});

@benjamingr
Copy link
Member

It's because nextTick can take extra parameters, but we never documented it - I'll fix.

@benjamingr
Copy link
Member

benjamingr commented Jul 18, 2017

Sorry @royalpinto , this is my bad - I meant to have it work but I made a silly mistake:

-            args: Array.prototype.slice.call(1)
+            args: Array.prototype.slice.call(arguments, 1)

Submitting a PR now.

@royalpinto
Copy link
Author

@fatso83 @elad-nach @benjamingr
Thank you all for the quick resolution 👍
Is this is available in the new npm release now ? I did a npm install lolex (it installed same 2.1.0) and error is still repeating.

@fatso83
Copy link
Contributor

fatso83 commented Jul 19, 2017

@royalpinto No new patch release has been made. I'll see to it today.

@fatso83
Copy link
Contributor

fatso83 commented Jul 19, 2017

This is now published as 2.1.1.

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

Successfully merging a pull request may close this issue.

4 participants