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

All: Prevent async tests from having multiple resume timeouts #1002

Closed
wants to merge 3 commits into from

Conversation

trentmwillis
Copy link
Member

Addressing #984.

The root issue is that it was possible for a single async test to have multiple resumeProcessing timeouts. This parallel assert.async calls test is probably the easiest case to see why that would be a problem.

  1. done1 would get called and queue up a resumeProcessing timeout.
  2. done2 would get called and queue up another resumeProcessing timeout. test.semaphore is now at 0.
  3. The first timeout executes and hits begin() to resume the test suite
  4. The second timeout executes and hits begin() as well, which causes issues because we're already running

The reason this worked without passing in test before is that config.current would update between the time 3 and 4 happened above which would prevent begin() from getting called a second time.

My solution is to check to see if test is already waiting to resume when passed into resumeProcessing.

@@ -44,7 +44,7 @@ QUnit.assert = Assert.prototype = {

test.semaphore -= 1;
popped = true;
resumeProcessing();
resumeProcessing( test );
Copy link
Member

Choose a reason for hiding this comment

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

👏

@trentmwillis
Copy link
Member Author

@gibson042 updated.

@trentmwillis
Copy link
Member Author

@gibson042 / @leobalter would appreciate a review so I can close this out.

@gibson042
Copy link
Member

Sorry about dropping this. LGTM!

@leobalter
Copy link
Member

:shipit:

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

Successfully merging this pull request may close these issues.

None yet

4 participants