Navigation Menu

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

assert.timeout() can fail on simple synchronous tests #1539

Closed
Krinkle opened this issue Jan 12, 2021 · 1 comment · Fixed by #1540
Closed

assert.timeout() can fail on simple synchronous tests #1539

Krinkle opened this issue Jan 12, 2021 · 1 comment · Fixed by #1540
Assignees
Labels
Component: Core For module, test, hooks, and runner. Type: Bug

Comments

@Krinkle
Copy link
Member

Krinkle commented Jan 12, 2021

Tell us about your runtime:

  • QUnit version: 2.13.0-dev (latest git)
  • What environment are you running QUnit in?: Browser

What are you trying to do?

Code that reproduces the problem:

The following test fails intermittently in IE Opera (Chromium-based) and IE 10 on Windows (probably other browsers as well). However, it passes consistently in Firefox, Chrome, and Safari on macOS:

	QUnit.test( "does not push a failure if test is synchronous", function( assert ) {
		assert.timeout( 1 );

		var wait = Date.now() + 10;
		while ( Date.now() < wait ) {} // eslint-disable-line no-empty

		assert.true( true );
	} );

Failure

Test took longer than 1ms; test timed out.

Screenshot in IE 10

@Krinkle Krinkle added Type: Bug Component: Core For module, test, hooks, and runner. labels Jan 12, 2021
@Krinkle Krinkle self-assigned this Jan 12, 2021
@Krinkle
Copy link
Member Author

Krinkle commented Jan 12, 2021

I suspect the issue might be with this assumption in assert.timeout() which is meant to catch cases where assert.timeout() is called twice in the same test, but that is not actually happening in our reproduction case:

qunit/src/assert.js

Lines 18 to 33 in 9f16eed

timeout( duration ) {
if ( typeof duration !== "number" ) {
throw new Error( "You must pass a number as the duration to assert.timeout" );
}
this.test.timeout = duration;
// If a timeout has been set, clear it and reset with the new duration
if ( config.timeout ) {
clearTimeout( config.timeout );
if ( config.timeoutHandler && this.test.timeout > 0 ) {
resetTestTimeout( this.test.timeout );
}
}
}

I added the following to confirm the issue:

  	QUnit.test( "does not push a failure if test is synchronous", function( assert ) {
+		if ( QUnit.config.timeout ) {
+			throw new Error( "QUnit.config.timeout: " + QUnit.config.timeout );
+		}
 		assert.timeout( 1 );
 

With this patch in place, I confirmed that indeed this condition is being intemittently. About every other test run for me. And while it doesn't cause a test failure normally for me in Firefox or Chrome, I do find this condition being met met half the time in those browsers.

So it would seem we have a race condition where about half the time, config.timeout ends up retaining some kind of value. And only on Windows does it cause the test to fail as a result of that.

Krinkle added a commit that referenced this issue Jan 12, 2021
For an async test, we normally remember the assigned timeout amount,
and the actual scheduling and enforcement of the timeout doesn't happen
until after the test callback returns.

If the test doesn't happen to be asynchronous, then the amount is
discarded and we simply move on to the next test.

Sometimes, the timeout ID stored in `config.timeout` for a previous
async test would remain and thus caused `assert.timeout()` to wrongly
think it was called twice in the same test, and tries clear and
reschedule it, when actually it is clearing nothing and scheduling
the first timeout and doing so before the test callback has returned.

Thus when the test is over and we "ignore" the timeout, QUnit then
got confused thinking it's an async test and thus waits until it
times out.

Fix this by making sure `config.timeout` is always explicitly emptied
(or re-assigned) when a timeout is cancelled or has completed.

Fixes #1539.
Krinkle added a commit that referenced this issue Jan 12, 2021
For an async test, we normally remember the assigned timeout amount,
and the actual scheduling and enforcement of the timeout doesn't happen
until after the test callback returns.

If the test doesn't happen to be asynchronous, then the amount is
discarded and we simply move on to the next test.

Sometimes, the timeout ID stored in `config.timeout` for a previous
async test would remain and thus caused `assert.timeout()` to wrongly
think it was called twice in the same test, and tries clear and
reschedule it, when actually it is clearing nothing and scheduling
the first timeout and doing so before the test callback has returned.

Thus when the test is over and we "ignore" the timeout, QUnit then
got confused thinking it's an async test and thus waits until it
times out.

Fix this by making sure `config.timeout` is always explicitly emptied
(or re-assigned) when a timeout is cancelled or has completed.

Fixes #1539.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core For module, test, hooks, and runner. Type: Bug
Development

Successfully merging a pull request may close this issue.

1 participant