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: Introduce timeout to set per-test timeout durations #1165

Merged
merged 4 commits into from
Jul 7, 2017
Merged
Show file tree
Hide file tree
Changes from all 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 Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ module.exports = function( grunt ) {
"test/main/test",
"test/main/assert",
"test/main/assert/step",
"test/main/assert/timeout",
"test/main/async",
"test/main/promise",
"test/main/modules",
Expand Down
46 changes: 46 additions & 0 deletions docs/assert/timeout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
layout: default
title: timeout
description: Sets the length of time to wait for async operations before failing the test.
categories:
- assert
- async
---

## `timeout( duration )`

Sets the length of time to wait for async operations before failing the test.

| name | description |
|------|-------------|
| `duration` (Number) | The length of time, in milliseconds, to wait for async operations. |

### Description

`assert.timeout()` sets the length of time, in milliseconds, to wait for async operations in the current test. This is equivalent to setting `config.testTimeout` on a per-test basis. The timeout length only applies when performing async operations.

If `0` is passed, then the test will be assumed to be completely synchronous. If a non-numeric value is passed as an argument, the function will throw an error.
Copy link
Member

@Krinkle Krinkle Jul 4, 2017

Choose a reason for hiding this comment

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

I don't think we currently handle this. I haven't manually confirmed, but from looking at the code and the test, this isn't ensured as far as I can see. The current test verifies the error message, not the behaviour.

"Completely synchronous" is quite a bold statement, one I'd love to have in our documentation, but it does set high expectations. We wouldn't want to have this with low-confidence of it actually being so. It should mean that, by the end of the test() callback, the test is over.

setTimeout(, 0) tends to be rounded up to at least ~4ms on most platforms. And even without that aspect, "the next (available) tick" can still have many (small) things in-between. Especially the micro-task queue in JavaScript (containing Promise callbacks, event handlers, and various other things), and things like requestAnimationFrame, setImmediate, and other features that offer a shorter "timeout" than setTimeout.

Apart from that, even we'd have a way to enqueue before the micro-task queue, that means the user's code could as well, and it tends to be FIFO, so our handler might still be after the user's handler that calls done() which still violates the fine "Completely synchronous" guarantee.

I'd like to see the following fail (with timeout=0):

  • returning Promise.resolve().
  • Manually, via Promise.resolve().then( assert.async() )`.
  • Manually, via setTimeout( assert.sync(), 0 );.

As for how to make these fail, I think we may have to special-case 0 and actually validate it right after calling test(), which means a more tight integration, but I think it's worth for this feature.

Copy link
Member Author

@trentmwillis trentmwillis Jul 4, 2017

Choose a reason for hiding this comment

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

To summarize (to ensure I'm understanding everything):

  • setTimeout(, 0) doesn't guarantee synchronicity (agreed)
  • However, we do want to guarantee synchronicity when using a timeout of 0
  • Therefore, we should special case assert.timeout(0)
  • And, add tests handling edge cases that likely fail with the current implementation

Does that seem correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's right.


### Examples

```js
QUnit.test( "Waiting for focus event", function( assert ) {
assert.timeout( 1000 ); // Timeout of 1 second
var done = assert.async();
var input = $( "#test-input" ).focus();
setTimeout(function() {
assert.equal( document.activeElement, input[0], "Input was focused" );
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a non-DOM example?

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @gibson042 here. Note: Since the above comment was added, a non-DOM example was also added below, which is great.

I don't particularly mind having an example involving DOM in someway, but as unimportant as it might seem, the specific example we have above seems like an anti-pattern I'd rather not promote in the documentation.

I'd be okay with having just the below example, although in that case I'd rather have the main example showcase assert.async() which is easier to understand when learning QUnit. Having two examples, where one shows compat with Promise still would be nice too, though.

done();
});
});
```

```js
QUnit.test( "Waiting for async function", function( assert ) {
assert.timeout( 500 ); // Timeout of .5 seconds
var promise = asyncFunction();
return promise.then( function( result ) {
assert.ok( result );
} );
});
```
8 changes: 8 additions & 0 deletions src/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ class Assert {

// Assert helpers

timeout( duration ) {
if ( typeof duration !== "number" ) {
throw new Error( "You must pass a number as the duration to assert.timeout" );
}

this.test.timeout = duration;
}

// Documents a "step", which is a string value, in a test as a passing assertion
step( message ) {
const result = !!message;
Expand Down
39 changes: 31 additions & 8 deletions src/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export default function Test( settings ) {
this.module = config.currentModule;
this.stack = sourceFromStacktrace( 3 );
this.steps = [];
this.timeout = undefined;

// If a module is skipped, all its tests and the tests of the child suites
// should be treated as skipped even if they are defined as `only` or `todo`.
Expand Down Expand Up @@ -164,6 +165,15 @@ Test.prototype = {
function runTest( test ) {
promise = test.callback.call( test.testEnvironment, test.assert );
test.resolvePromise( promise );

// If the test has a "lock" on it, but the timeout is 0, then we push a
// failure as the test should be synchronous.
if ( test.timeout === 0 && test.semaphore !== 0 ) {
Copy link
Member

@Krinkle Krinkle Jul 6, 2017

Choose a reason for hiding this comment

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

I keep staring at test.semaphore !== 0 here. I can't quite put my finger on it... We need this because... Because, semaphore controls whether or not we'll wait before starting the next test, and is incremented whenever a Promise is returned and/or async was used. Got it.

pushFailure(
"Test did not finish synchronously even though assert.timeout( 0 ) was used.",
sourceFromStacktrace( 2 )
);
}
}
},

Expand Down Expand Up @@ -677,20 +687,33 @@ export function only( testName, callback ) {

// Put a hold on processing and return a function that will release it.
export function internalStop( test ) {
var released = false;

test.semaphore += 1;
config.blocking = true;

// Set a recovery timeout, if so configured.
if ( config.testTimeout && defined.setTimeout ) {
clearTimeout( config.timeout );
config.timeout = setTimeout( function() {
pushFailure( "Test timed out", sourceFromStacktrace( 2 ) );
internalRecover( test );
}, config.testTimeout );
if ( defined.setTimeout ) {
let timeoutDuration;

if ( typeof test.timeout === "number" ) {
timeoutDuration = test.timeout;
} else if ( typeof config.testTimeout === "number" ) {
timeoutDuration = config.testTimeout;
}

if ( typeof timeoutDuration === "number" && timeoutDuration > 0 ) {
clearTimeout( config.timeout );
config.timeout = setTimeout( function() {
pushFailure(
`Test took longer than ${timeoutDuration}ms; test timed out.`,
sourceFromStacktrace( 2 )
);
internalRecover( test );
}, timeoutDuration );
}

}

let released = false;
return function resume() {
if ( released ) {
return;
Expand Down
3 changes: 3 additions & 0 deletions test/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@
"globals": {
"QUnit": false,
"console": false
},
"rules": {
"max-len": "off"
}
}
1 change: 1 addition & 0 deletions test/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<script src="main/test.js"></script>
<script src="main/assert.js"></script>
<script src="main/assert/step.js"></script>
<script src="main/assert/timeout.js"></script>
<script src="main/async.js"></script>
<script src="main/promise.js"></script>
<script src="main/dump.js"></script>
Expand Down
101 changes: 101 additions & 0 deletions test/main/assert/timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
QUnit.module( "assert.timeout", function() {
QUnit.test( "pushes a failure if test times out when using async", function( assert ) {
assert.timeout( 10 );
assert.expect( 1 );

var originalPushFailure = QUnit.config.current.pushFailure;
QUnit.config.current.pushFailure = function pushFailureStub( message ) {
QUnit.config.current.pushFailure = originalPushFailure;

assert.equal( message, "Test took longer than 10ms; test timed out." );
};

assert.async();
} );

QUnit.test( "pushes a failure if test times out when using a promise", function( assert ) {
assert.timeout( 10 );
assert.expect( 1 );

var originalPushFailure = QUnit.config.current.pushFailure;
QUnit.config.current.pushFailure = function pushFailureStub( message ) {
QUnit.config.current.pushFailure = originalPushFailure;

assert.equal( message, "Test took longer than 10ms; test timed out." );
};

// Return a "thenable" to serve as a mock Promise
return {
then: function() {}
};
} );

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 ) {}

assert.ok( true );
} );

QUnit.test( "throws an error if a non-numeric value is passed as duration", function( assert ) {
assert.throws( function() {
assert.timeout( null );
}, /You must pass a number as the duration to assert.timeout/ );
} );

QUnit.module( "a value of zero", function() {
function stubPushFailure( assert ) {
var originalPushFailure = QUnit.config.current.pushFailure;
QUnit.config.current.pushFailure = function pushFailureStub( message ) {
QUnit.config.current.pushFailure = originalPushFailure;

assert.equal(
message,
"Test did not finish synchronously even though assert.timeout( 0 ) was used."
);
};
}

QUnit.test( "does not fail a synchronous test using assert.async", function( assert ) {
assert.timeout( 0 );
var done = assert.async();
assert.ok( true );
done();
} );

QUnit.test( "fails a test using assert.async and a setTimeout of 0", function( assert ) {
assert.timeout( 0 );
assert.expect( 1 );

stubPushFailure( assert );

var done = assert.async();
setTimeout( done, 0 );
} );

if ( typeof Promise !== "undefined" ) {
QUnit.test( "fails a test returning an immediately resolved Promise", function( assert ) {
assert.timeout( 0 );
assert.expect( 1 );

stubPushFailure( assert );

return Promise.resolve();
} );

QUnit.test( "fails a test using assert.async and an immediately resolved Promise", function( assert ) {
assert.timeout( 0 );
assert.expect( 1 );

stubPushFailure( assert );

var done = assert.async();
Promise.resolve().then( done );
} );
}
} );
} );