sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
806 stars 107 forks source link

Upgrade lolex with async versions of all timer-executing calls (2019) #237

Closed dominykas closed 5 years ago

dominykas commented 5 years ago

This is a rebased and updated version of #105.

I still have to do a full review and test in our code base, probably might need to update some documentation, but just wanted to open this up. I probably also need to re-read all the threads on this... But hopefully this is now closer to getting merged than it was before :)

quasicomputational commented 5 years ago

I've just tried this out, dropping this branch in instead of some local code that implemented async timers, and I hit some snags along the way - but, once I overcame them, I can report it worked fine. I do have one problem to report. Specifically, sequential calls to runAllAsync can fail to be idempotent, which is quite contrary to my expectations - since, if it runs the queue to exhaustion, how can a second call find any more timers to run?

If I don't flush the timer queue at the end of every test, my tests stall and time out. OK, so I need to write my tests like this:

const itAsync = (name, fn) => {
    it(name, async () => {
        fn();
        await clock.runAllAsync();
        // D
    });
};
itAsync("flarbs the blah", async () => {
    // A
    await clock.runAllAsync();
    // B
    await clock.runAllAsync();
    // C
});

Unfortunately, concurrent runAllAsync didn't behave like I wanted it to. The order of execution I observed was ABDC, as opposed to the hoped-for ABCD. This is quite problematic for me, since then the code at C runs after Jest considers the test to be over and done with.

If, however, itAsync is written like this:

const itAsync = (name, fn) => {
    it(name, async () => {
        let running = true;
        Promise.resolve(fn()).finally(() => {
            running = false;
        });
        while (running) {
            await clock.runAllAsync();
        }
    });
};

Then it works, and the code at C gets run before the end of the test. But this is clearly demonstrating that runAllAsync isn't idempontent. I think it'd be better if, even with multiple concurrent invocations, runAllAsync was guaranteed to be idempotent.

quasicomputational commented 5 years ago

Actually, upon reflection, concurrent runAllAsync can't guarantee idempotency, because the code run after any one of them completes could schedule further tasks. So I think that what this warrants is a big scary warning in the docs, saying that this is not something you can rely on despite what a naive mental model might suggest.

benjamingr commented 5 years ago

I don't think this will work with async/await. I think the way to make this work is to use the devtools protocol.

I am not opposed to landing a stop-gap though.

benjamingr commented 5 years ago

I will also ping my employer and ask for a full day of work on lolex to close all the stuff I need. Sorry! I was hoping to do this in May but then all the conferences happened.

dominykas commented 5 years ago

Rebased this on top of master and tested it out on an internal project - await clock.tickAsync() works like a charm.

Will try to add some docs to this.

dominykas commented 5 years ago

@quasicomputational in your example, if you add an await fn() (line 3) - does it work correctly?

dominykas commented 5 years ago

I didn't perform as thorough a review as I would at my day job, but @kylefleming's #105 looks very well thought out, so I hope we can merge this :)

I know there's comments about different approaches on how this could have been implemented differently, but I don't think the required functionality is present in node nor in the browsers, and this hasn't changed in the last 2 years, so quite possibly this is as good as it will ever get...

fatso83 commented 5 years ago

@dominykas Can you add some docs that explain the limits of the runAllAsync feature? "Using it in simple examples like A will work, but B needs this (*) workaround", where we document @quasicomputational 's workaround and a link to https://github.com/sinonjs/lolex/pull/237#issuecomment-498825901 for what a proper fix entails?

I'm inline with you on this with regards to having a stopgap solution that works in 80% of the cases, while waiting for The Perfect Fix :tm:.

As an alternative to trying to fix bigger examples into the API docs, I could add a Lolex how-to for doing this on the SinonJS how-to page, but I'd still need a somewhat useful template. To not stop this from expanding, I could split that into a separate issue.

quasicomputational commented 5 years ago

FWIW I think that this PR is as good as can be done, and that there's not even a theoretical 'better fix' that merging should block on. The bug is in user expectations, not library code.

voxpelli commented 5 years ago

Wanted to chime in with a 👍 on this. I'm using a quick wrapper (built by my colleague @digli) for this functionality in some current projects and it has shown a very real value.

The wrapper:

const tick = async (clock, ms) => { clock.tick(ms); };
// ...
await tick(clock, 300);
dominykas commented 5 years ago

@fatso83 sure, I'll see what I can do.

mroderick commented 5 years ago

@fatso83 go ahead and merge this

dominykas commented 5 years ago

I ran eslint . --fix after resolving the conflict... best viewed with whitespace ignore: https://github.com/sinonjs/lolex/pull/237/commits/ccbefa23c99261cdc1eadc95f4269c26836626aa

dominykas commented 5 years ago

🎉 ✅

mroderick commented 5 years ago

I think this needs a rebase

dominykas commented 5 years ago

I think this needs a rebase

Is that to keep the commit history clean?

I can do that tomorrow, I suppose.

mroderick commented 5 years ago

Is that to keep the commit history clean?

I can do that tomorrow, I suppose.

2019-10-10 at 15 48

fatso83 commented 5 years ago

It needs a little bit more work, @dominykas, as I just ran the npm run test-cloud command and it failed in Firefox:

 285 passing (44s)
  43 pending
  3 failing

  1) lolex
       tickAsync
         passes 2 hours, 34 minutes and 10 seconds:
     Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  require<[187]</<[32]</</Runnable.prototype._timeoutError@about:blank line 2 > scriptElement:46839:10
  require<[187]</<[32]</</Runnable.prototype.resetTimeout/this.timer<@about:blank line 2 > scriptElement:46650:24

  2) lolex
       runAllAsync
         throws before allowing infinite recursion:
     Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  require<[187]</<[32]</</Runnable.prototype._timeoutError@about:blank line 2 > scriptElement:46839:10
  require<[187]</<[32]</</Runnable.prototype.resetTimeout/this.timer<@about:blank line 2 > scriptElement:46650:24

  3) lolex
       runAllAsync
         throws before allowing infinite recursion from promises:
     Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  require<[187]</<[32]</</Runnable.prototype._timeoutError@about:blank line 2 > scriptElement:46839:10
  require<[187]</<[32]</</Runnable.prototype.resetTimeout/this.timer<@about:blank line 2 > scriptElement:46650:24

Are you able to modify and run the test in Firefox yourself? We can provide you with sauce labs credentials (privately in a dm; my email is public).

BTW, I rebased and pushed to a branch of my own: https://github.com/fatso83/lolex/tree/async-upgrade-2019. You can just checkout that branch and reset your own branch pointer to that, to save you some work (added a small fix).

dominykas commented 5 years ago

Cheers, I'll take a look at the Firefox issue.

dominykas commented 5 years ago

To make reviewing easier:

Force push 1: reset my head to @fatso83's head - diff provided by GH

Force push 2: rebased that on top of master (5.0.1) - I have a branch with the same code as before, but master merged in - if you were to diff that against async-upgrade-2019 (this PR) - you'd see 0 changes.

i.e. no change in the final result since last reviews.

Now, onto Firefox...

dominykas commented 5 years ago

Are you able to modify and run the test in Firefox yourself? We can provide you with sauce labs credentials (privately in a dm; my email is public).

I was not able to start that up locally... Seems mochify needs some love? Or maybe webdriver/selenium... Sigh.

I used Saucelabs credentials from one of my other OSS projects. Please don't tell anyone ;)

Anyways, it seems the issue was due to the fact that promises (new ticks?) seem to be some 3x slower than Chrome - decreased the number of loops and got this to pass on Saucelabs - @fatso83 can you give it a spin?

dominykas commented 5 years ago

@dominykas Can you add some docs that explain the limits of the runAllAsync feature?

Had another look. TBH, I'm not entirely sure I understand the problem - but I also posted a suggestion of where I think @quasicomputational's code itself might be problematic (or maybe I misunderstood the intent there).

The other linked comment is this:

I don't think this will work with async/await. I think the way to make this work is to use the devtools protocol.

I'm not entirely sure what is the problem here either. async / await, to my understanding, should just work as this is just promises. I haven't encountered any issues in my testing (using async/await), but ofc I didn't try to stretch it - and I didn't analyze the code too much (I only rebased it to make sure its mergeable... and then forgot the details).

I'm thinking that maybe it's best to get some feedback on this from the users? Feel free to tag me on the issues that are raised on sinon/lolex repos that you think may be related and we'll see whether the code needs an update or the docs?

dominykas commented 5 years ago

🎉 thanks!