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

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

Merged
merged 5 commits into from
Oct 14, 2019

Conversation

dominykas
Copy link
Contributor

@dominykas dominykas commented Apr 19, 2019

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

@dominykas dominykas changed the title Upgrade lolex with async versions of all timer-executing calls Upgrade lolex with async versions of all timer-executing calls (2019) Apr 19, 2019
@quasicomputational
Copy link

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

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.

@fatso83 fatso83 requested a review from benjamingr June 4, 2019 18:15
@benjamingr
Copy link
Member

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

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

dominykas commented Jul 3, 2019

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

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

@dominykas
Copy link
Contributor Author

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

fatso83 commented Aug 4, 2019

@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 #237 (comment) 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 ™️.

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

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

voxpelli commented Aug 9, 2019

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

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

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

I think this is good to go. Sorry for keeping you and @kylefleming out dry by leaving this hanging for so long. As you stated, it would be nice to have some additional docs explaining the limits of runAllAsync, but it can be added later on. Preferably in lolex, with either a duplicate comment in the Sinon API docs or with a link to lolex for more insight (like you did with lolex.install).

I'll merge this today, unless there's more input.

@mroderick
Copy link
Member

@fatso83 go ahead and merge this

@dominykas
Copy link
Contributor Author

I ran eslint . --fix after resolving the conflict... best viewed with whitespace ignore: ccbefa2

@dominykas
Copy link
Contributor Author

🎉 ✅

@mroderick
Copy link
Member

I think this needs a rebase

@dominykas
Copy link
Contributor Author

I think this needs a rebase

Is that to keep the commit history clean?

I can do that tomorrow, I suppose.

@mroderick
Copy link
Member

Is that to keep the commit history clean?

I can do that tomorrow, I suppose.

2019-10-10 at 15 48

@fatso83
Copy link
Contributor

fatso83 commented Oct 10, 2019

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

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

failed in Firefox

@dominykas
Copy link
Contributor Author

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

Added tickAsync(), nextAsync(), runAllAsync(), runToLastAsync()
@dominykas
Copy link
Contributor Author

dominykas commented Oct 11, 2019

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

Promises are slow, they result in many browser ticks, making the tests timeout on Firefox.
@dominykas
Copy link
Contributor Author

dominykas commented Oct 11, 2019

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

@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?

@fatso83 fatso83 merged commit 5c0f0f8 into sinonjs:master Oct 14, 2019
@dominykas dominykas deleted the async-upgrade-2019 branch October 14, 2019 09:32
@dominykas
Copy link
Contributor Author

🎉 thanks!

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 this pull request may close these issues.

7 participants