Add a how-to guide for speeding up time with lolex #1931
Conversation
Pull Request Test Coverage Report for Build 2909
💛 - Coveralls |
|
Hey, thank you so much for following up on our email discussion - will take a look promptly :) |
docs/_howto/lolex-async-promises.md
Outdated
| @@ -0,0 +1,128 @@ | |||
| --- | |||
| layout: page | |||
| title: How to speed up time with lolex | |||
There was a problem hiding this comment.
Should the title be "How to test async functions with fake timers" perhaps?
docs/_howto/lolex-async-promises.md
Outdated
| title: How to speed up time with lolex | ||
| --- | ||
|
|
||
| With lolex, testing code that depends on timers is easier, as it sometimes |
There was a problem hiding this comment.
With lolex -> "With fake timers (lolex)"?
docs/_howto/lolex-async-promises.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| We could expect similar behavior from a promisified timeout: |
There was a problem hiding this comment.
We could - "one might"?
Also - maybe "One might expect to be able to use a similar approach to test an async function using promises:"?
docs/_howto/lolex-async-promises.md
Outdated
| }; | ||
| ``` | ||
|
|
||
| Trying a naive approach: |
There was a problem hiding this comment.
Not sure about "naive", maybe "One may try"?
docs/_howto/lolex-async-promises.md
Outdated
| assert.equal(result, 42); // PASS | ||
| }); | ||
|
|
||
| // or using Mocha's promises |
There was a problem hiding this comment.
Maybe "Or without using an async function" since both versions are using Mocha's promise return value support?
docs/_howto/lolex-async-promises.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| Although `async` functions cannot be tested synchronously, we can test Promises |
There was a problem hiding this comment.
Add "(yet)" after "cannot"? We're talking about it :)
docs/_howto/lolex-async-promises.md
Outdated
| ```js | ||
| // maker.js | ||
| module.exports.fulfillAfterOneSecond = () => { | ||
| return new Promise(fulfill => { |
There was a problem hiding this comment.
Nit: this is resolve and not fulfill
There was a problem hiding this comment.
To nitpick further: I think what Benjamin is saying is not that you have to call your callbacks "resolve", but that "fullfill" is a somewhat misleading name for the resolving callback, as a promise is also fulfilled if you reject it :-)
docs/_howto/lolex-async-promises.md
Outdated
| }; | ||
| ``` | ||
|
|
||
| `fulfillAfterOneSecond` resolves to 42 after one second, just like |
There was a problem hiding this comment.
Maybe something like: By returning the promise from mocha's test and making it an async test we can still progress the internal timers using lolex's fake timers - so we only have to wait the time it takes the promise to resolve which is almost instant.
Or something of the sort?
docs/_howto/lolex-async-promises.md
Outdated
| ``` | ||
|
|
||
| The above test passes immediately, without the 1 second delay. This is because | ||
| we do not try to hijack `then()` call, but use Mocha's native ability to test |
There was a problem hiding this comment.
"we do not try to" "while we do not"?
docs/_howto/lolex-async-promises.md
Outdated
| it('should return 42 after one second', () => { | ||
| const promise = maker.asyncReturnAfterOneSecond(); | ||
|
|
||
| // this call does not really speed up anything |
There was a problem hiding this comment.
This just creates confusion, if it is supposed to show a way of doing the same as above, but without ES2015 syntax. Line 86 and 87 can be deleted.
docs/_howto/lolex-async-promises.md
Outdated
| ``` | ||
|
|
||
| The above test fails, since `asyncReturnAfterOneSecond` is an `async` function | ||
| that returns a Promise, and it is currently impossible to control the Promises' |
There was a problem hiding this comment.
the Promise's (definite singular) or promises' (indefinite plural)
docs/_howto/lolex-async-promises.md
Outdated
| ``` | ||
|
|
||
| `fulfillAfterOneSecond` resolves to 42 after one second, just like | ||
| `asyncReturnAfterOneSecond`, but it is testable in a synchronous way: |
There was a problem hiding this comment.
no, it's not synchronous. the test is still asynchronous, as the assert call is done in a microtick after the function exits. The reason fulfillAfterOneSecond resolves (almost) immediately using clock.tick and asyncReturnAfterOneSecond is not affected by clock.tick has to do with how setTimeout is resolved. In asyncReturnAfterOneSecond you bind the original value of setTimeout using util.promisify(). That means it has a reference to the original setTimeout which will not be affected by anything lolex does. async really has nothing to do with it.
There was a problem hiding this comment.
I think there is an assumption lolex.install() was called prior to anything - but I think it makes a ton of sense to make it explicit as part of the test (that is, add a lolex.install() part above)
docs/_howto/lolex-async-promises.md
Outdated
| ``` | ||
|
|
||
| The above test passes immediately, without the 1 second delay. This is because | ||
| we do not try to hijack `then()` call, but use Mocha's native ability to test |
There was a problem hiding this comment.
I don't understand this section. You didn't try to hijack the then call in earlier tests? Mocha's built-in promise support is just syntactic sugar to avoid passing in a callback and there isn't much native stuff going on. I'd replace "native ability to test Promises" with "built-in support for Promises"
There was a problem hiding this comment.
Agreed about "built-in support for promises"
docs/_howto/lolex-async-promises.md
Outdated
|
|
||
| The above test passes immediately, without the 1 second delay. This is because | ||
| we do not try to hijack `then()` call, but use Mocha's native ability to test | ||
| Promises, while speeding up their resolution. |
There was a problem hiding this comment.
Here it seems like it is Mocha that is speeding up their resolution. I think it would be clearer if you changed it to "while using lolex to speed up the test".
docs/_howto/lolex-async-promises.md
Outdated
| we do not try to hijack `then()` call, but use Mocha's native ability to test | ||
| Promises, while speeding up their resolution. | ||
|
|
||
| Although it is currently impossible to speed up `async` functions, or even |
There was a problem hiding this comment.
This isn't correct. You can speed up async functions. It's just syntactic sugar-coating for promises. Anything you can do with promises is possible with async and vice versa. Unless you can inform me otherwise?
There was a problem hiding this comment.
It's easier to speed up promises because you can override the promise library to one that uses a scheduler
Speeding up async functions without invoking the native %RunMicrotasks (which requires an addon) or using a require('binding') with an undocumented internal API requires transpiling your async functions to coroutines.
There was a problem hiding this comment.
True, but I didn't mean overriding the scheduler for the Promises, but rather the statement that implied that one cannot speedup async tests that use timers. It seemed as if the statement said that if you have an async test using setTimeout (for instance), then it would not be affected by clock.tick calls, which would be incorrect.
Just the fact that we are arguing the meaning might be a hint that this sentence should be rephrased somehow :-)
docs/_howto/lolex-async-promises.md
Outdated
| we do not try to hijack `then()` call, but use Mocha's native ability to test | ||
| Promises, while speeding up their resolution. | ||
|
|
||
| Although it is currently impossible to speed up `async` functions, or even |
There was a problem hiding this comment.
It's not currently impossible - just very very hacky :) I would add here the conclusion maybe:
"Although it is currently impossible to speed up the thens async functions run it is still possible to speed up timers and write meaningful sped-up tests with code using async functions." or something of the sort.
Up to you.
|
Thank you for working on this! |
.gitignore
Outdated
| @@ -5,3 +5,4 @@ coverage/ | |||
| .idea | |||
| .sass_cache | |||
| _site | |||
| *.swp | |||
There was a problem hiding this comment.
Please remove this. This is not specific to this project.
Use git's global ignore so you don't need to add vim knowledge to every repo you touch.
There was a problem hiding this comment.
Will do.
How is it different from .idea though?😼
There was a problem hiding this comment.
I also think it makes sense to either allow these or remove them all but no strong feelings
There was a problem hiding this comment.
No strong feelings either. If .swp doesn't belong here, neither does .idea.
There was a problem hiding this comment.
I didn't review the initial commits and I have no idea what .idea is for in the first place. I think it makes sense for the files in a repo to be specific to the repo instead of specific to the people that edited the repo. If .idea is specific to some editor and/or OS, then I think it should be removed as well.
|
Thanks for your comments, I'm in the middle of fixing the issues you guys raised. |
|
@orlangure thanks, we're looking forward to it :) |
|
I ended up changing a lot of text and some of the examples, so not all your suggested changes made it into this draft. But they definitely made things more clear to me and allowed to provide more clear examples. |
docs/_howto/lolex-async-promises.md
Outdated
|
|
||
| ```js | ||
| module.exports.asyncReturnAfterOneSecond = async () => { | ||
| // util.promisify is not used deliberately |
There was a problem hiding this comment.
I need your help here. I think this comment should not be in a guide, but without it it looks like a mistake to not use util.promisify.
When it is used though, the test fails.
There was a problem hiding this comment.
When it is used though, the test fails.
Make it a proxy to util.promisify or call clock.install() before util.promisify is called (because then it holds a reference to the original setTimeout):
module.exports. asyncReturnAfterOneSecond = async () => {
const setTimeoutPromise = util.promisify(setTimeout); //
// ...
}There was a problem hiding this comment.
Nope, even with lolex.install call before util.promisify, tests of this function fail.
What did you mean by proxying somehting to util.promisify? I'll check it as well.
There was a problem hiding this comment.
I think this needs to be fixed in lolex. I'll make an issue.
There was a problem hiding this comment.
For reference, the issue is sinonjs/fake-timers#227
|
Can this guide be reviewed again before the issue is fixed? I guess most of it does not depend on that bug in lolex. |
|
Yeah, this is my bad and I'll fix it in lolex in the coming week (hopefully, it's on my plate). Then I think we can merge this in :) |
|
Still waiting for sinonjs/fake-timers#227 to be reviewed and merged. Bumping for now. |
|
@benjamingr Any chance of looking at the PR blocking getting this in? Yuri has kind of been waiting since October. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
@orlangure I don't see why we should keep this hanging. I know sinonjs/fake-timers#227 has been stalled for some time, but this is just affecting a single line in this whole commit, and the current documented way works. Duplicate efforts have started appearing (#2054), so I'd like to focus the efforts :-)
If you could just update the minor spelling issue I'm all good with this. My suggestion on the comment wording is optional.
docs/_howto/lolex-async-promises.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| While returning a Promise from the Mocha’s test, we can still progress the timers |
There was a problem hiding this comment.
| While returning a Promise from the Mocha’s test, we can still progress the timers | |
| While returning a Promise from Mocha’s test, we can still progress the timers |
docs/_howto/lolex-async-promises.md
Outdated
|
|
||
| ```js | ||
| module.exports.asyncReturnAfterOneSecond = async () => { | ||
| // util.promisify is not used deliberately |
There was a problem hiding this comment.
| // util.promisify is not used deliberately | |
| // Using util.promisify would look nicer, but there is a lolex issue blocking this at the moment: https://github.com/sinonjs/lolex/pull/227 |
|
@fatso83 Done, thanks for pushing this forward. |
|
Thank you! This should go live at https://sinonjs.org/how-to in a few minutes. |
* Add a how-to on speeding up time with lolex * Explain weird setTimeoutPromise usage in comments
Purpose (TL;DR) - mandatory
This PR adds a how-to guide about current ways to control promises/async functions with lolex (#1898).
In a nutshell, there are a few examples: a regular function, an async function, and a Promise (walk into a bar). These things are tested in different ways to demonstrate what we can and cannot control.
How to verify - mandatory