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

Add a how-to guide for speeding up time with lolex #1931

Merged
merged 8 commits into from
Jun 28, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ coverage/
.idea
.sass_cache
_site
*.swp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.
How is it different from .idea though?😼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it makes sense to either allow these or remove them all but no strong feelings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feelings either. If .swp doesn't belong here, neither does .idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.idea -> IntelliJ IDEA

128 changes: 128 additions & 0 deletions docs/_howto/lolex-async-promises.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
---
layout: page
title: How to speed up time with lolex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the title be "How to test async functions with fake timers" perhaps?

---

With lolex, testing code that depends on timers is easier, as it sometimes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With lolex -> "With fake timers (lolex)"?

becomes possible to skip the waiting part and trigger scheduled callbacks
synchronously. Consider the following function of a maker module (a module
that makes things):

```js
// maker.js
module.exports.callAfterOneSecond = callback => {
setTimeout(callback, 1000);
};
```

We can use lolex to verify that `callAfterOneSecond` works as expected, but
skipping that part where the test takes one second:

```js
// test.js
it('should call after one second', () => {
const spy = sinon.spy();
maker.callAfterOneSecond(spy);

// callback is not called immediately
assert.ok(!spy.called);

// but it is called synchronously after the clock is fast forwarded
clock.tick(1000);
assert.ok(spy.called); // PASS
});
```

We could expect similar behavior from a promisified timeout:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could - "one might"?

Also - maybe "One might expect to be able to use a similar approach to test an async function using promises:"?


```js
// maker.js
const util = require('util');
const setTimeoutPromise = util.promisify(setTimeout);

module.exports.asyncReturnAfterOneSecond = async () => {
await setTimeoutPromise(1000);
return 42;
};
```

Trying a naive approach:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about "naive", maybe "One may try"?


```js
// test.js
it('should return 42 after one second', () => {
// result will hold a Promise after the next call
const result = maker.asyncReturnAfterOneSecond();

clock.tick(1000);

// the Promise was not resolved,
// even though we moved the time forward
assert.equal(result, 42); // FAIL
});
```

The above test fails, since `asyncReturnAfterOneSecond` is an `async` function
that returns a Promise, and it is currently impossible to control the Promises'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Promise's (definite singular) or promises' (indefinite plural)

forced resolution. **Their `then()` function always runs asynchronously**.

It doesn't mean that `async` functions and Promises cannot be tested though.
The most intuitive way to test the above `asyncReturnAfterOneSecond` is to
simply wait for one second:

```js
// test.js

// using async await
it('should return 42 after one second', async () => {
const result = await maker.asyncReturnAfterOneSecond();
assert.equal(result, 42); // PASS
});

// or using Mocha's promises
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Or without using an async function" since both versions are using Mocha's promise return value support?

it('should return 42 after one second', () => {
const promise = maker.asyncReturnAfterOneSecond();

// this call does not really speed up anything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clock.tick(1000);

return promise.then(result => assert.equal(result, 42)); // PASS
});
```

Although `async` functions cannot be tested synchronously, we can test Promises
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "(yet)" after "cannot"? We're talking about it :)

that are resolved in `setTimeout`. Consider the following function, that has the
same functionality as `asyncReturnAfterOneSecond`:

```js
// maker.js
module.exports.fulfillAfterOneSecond = () => {
return new Promise(fulfill => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is resolve and not fulfill

Copy link
Contributor

@fatso83 fatso83 Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

setTimeout(() => fulfill(42), 1000);
});
};
```

`fulfillAfterOneSecond` resolves to 42 after one second, just like
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

`asyncReturnAfterOneSecond`, but it is testable in a synchronous way:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


```js
// test.js
it('should be fulfilled after one second', () => {
orlangure marked this conversation as resolved.
Show resolved Hide resolved
const promise = maker.fulfillAfterOneSecond();

// this call actually makes the resolution quicker
clock.tick(1000);
return promise.then(result => assert.equal(result, 42)); // PASS
});
```

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed about "built-in support for promises"

Copy link
Member

@benjamingr benjamingr Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"we do not try to" "while we do not"?

Promises, while speeding up their resolution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


Although it is currently impossible to speed up `async` functions, or even
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Promises that are not resolved directly in `setTimeout`'s callback, there is a
discussion around this limitation. There is a chance that it will become
possible to force `then()` execution in the future, meaning that the things that
are impossible today may become possible at some point.