Assert: Introduce timeout to set per-test timeout durations #1165

trentmwillis · 2017-04-20T04:19:45Z

As discussed in #1132.

This makes it easy to set per-test timeouts, instead of the globally configured QUnit.config.testTimeout.

Krinkle · 2017-04-20T04:27:11Z

docs/assert/timeout.md

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


Missing space between "timeout()" and "sets".

Krinkle · 2017-04-20T04:28:40Z

src/assert.js

@@ -14,6 +14,14 @@ class Assert {

 	// Assert helpers

+	timeout( duration ) {
+		if ( objectType( duration ) !== "number" ) {


I don't think we need objectType here. typeof should suffices, right?

For 99% of cases it's probably fine, but if someone were to pass a new Number(1000), we'd miss it, even though it is a number.

I don't think we should support that implicitly. I'd rather be strict and throw an exception from a well-defined and stable interface.

Krinkle · 2017-04-20T04:34:56Z

src/test.js

@@ -36,6 +36,7 @@ export default function Test( settings ) {
 	this.module = config.currentModule;
 	this.stack = sourceFromStacktrace( 3 );
 	this.steps = [];
+	this.timeout = 0;


config.testTimeout is undefined by default. Having this one be 0 feels inconsistent. On the other hand, I don't like undefined as default for config.testTimeout. false is probably a better fit here. 0 could also work although my first-impression expectation based on other software would be that 0 either means infinite waiting or that no waiting is allowed (e.g. any async will be considered an error). Whereas in our case it means no waiting. Although I suppose infinite waiting and no waiting are very similar in the case of qunit - except that issue #1132 exposes a difference, albeit through a bug we should fix separately.

If we go with 0 we should document that for this and for config.testTimeout and which ever value we pick, we should file an issue to also change config.testTimeout's default in the next major release.

I agree that 0 is a poor default (it should describe the lowest meaningful timeout value), but I don't mind undefined for "no test-specific timeout".

I didn't put much thought into this when I was implementing it originally. undefined seems reasonable and is consistent with the current testTimeout, so I will go that route.

platinumazure · 2017-04-20T16:48:36Z

@trentmwillis I'm going to wait to see what shakes out of Timo's review before reviewing this myself. Feel free to ping me when you've committed any changes.

gibson042 · 2017-04-21T01:38:21Z

docs/assert/timeout.md

+  var done = assert.async();
+  var input = $( "#test-input" ).focus();
+  setTimeout(function() {
+    assert.equal( document.activeElement, input[0], "Input was focused" );


Could we have a non-DOM example?

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.

gibson042 · 2017-04-21T01:40:50Z

src/test.js

@@ -36,6 +36,7 @@ export default function Test( settings ) {
 	this.module = config.currentModule;
 	this.stack = sourceFromStacktrace( 3 );
 	this.steps = [];
+	this.timeout = 0;


I agree that 0 is a poor default (it should describe the lowest meaningful timeout value), but I don't mind undefined for "no test-specific timeout".

gibson042 · 2017-04-21T01:58:02Z

src/test.js

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

 	// Set a recovery timeout, if so configured.
-	if ( config.testTimeout && defined.setTimeout ) {
+	const timeoutDuration = test.timeout || config.testTimeout;
+	if ( timeoutDuration && defined.setTimeout ) {


Per the above, this should be more explicit. There's also a question of whether the global value is a fallback or a maximum. I'm personally inclined towards the latter, e.g.

const specificTimeout = typeof test.timeout === "number" ? test.timeout : Infinity; const globalTimeout = typeof config.testTimeout === "number" ? config.testTimeout : Infinity; const resolvedTimeout = Math.min( explicitTimeout, globalTimeout ); if ( resolvedTimeout < Infinity && defined.setTimeout ) { … }

I prefer the fallback approach. It is conceivable to want the majority of your tests to use a short timeout and then override that for a specific, slow test. If we go with the global maximum approach, we force the default for all tests to be the longer value.

trentmwillis · 2017-05-28T04:17:11Z

@Krinkle @gibson042 I've updated and believe I have addressed all comments. Another review would be appreciated 😄

trentmwillis · 2017-07-04T04:21:31Z

@qunitjs/qunit-team can I get someone to review this? I'd like to land this before releasing 2.4. Thanks!

Krinkle · 2017-07-04T04:48:51Z

test/main/assert/timeout.js

+QUnit.test( "does not push a failure if test is synchronous", function( assert ) {
+	assert.timeout( 1 );
+
+	var wait = Date.now() + 50;


Let's use 10 here as with the other one. Will make the test run a bit faster, too.

Rebased + updated.

Krinkle · 2017-07-04T04:59:04Z

docs/assert/timeout.md

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


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.

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?

Yep, that's right.

platinumazure

LGTM, aside from concerns already raised in other reviews.

trentmwillis · 2017-07-06T02:59:29Z

I have updated and addressed @Krinkle's concerns.

@platinumazure I've disabled the max-len ESLint rule for the test directory since it is not uncommon for test descriptions to be longer than it and I feel it adds negative value to split them on multiple lines.

Krinkle · 2017-07-06T03:06:57Z

test/main/assert/timeout.js

+			stubPushFailure( assert );
+
+			var done = assert.async();
+			setTimeout( () => done(), 0 );


(Causes CI failure) Too new syntax for the web browser we use for our Travis tests, and presumably for some of our other browsers as well.

Bah, a side-effect of too much Babel usage...

trentmwillis · 2017-07-06T03:27:17Z

CI passing again.

Krinkle · 2017-07-06T03:45:07Z

src/test.js

+
+			// 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 ) {


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.

platinumazure · 2017-07-06T03:46:50Z

@trentmwillis re: max-len Sounds good to me.

Prior comments have been addressed

Krinkle requested changes Apr 20, 2017

View reviewed changes

gibson042 previously requested changes Apr 21, 2017

View reviewed changes

trentmwillis force-pushed the assert-timeout branch from b3ae777 to 32022c9 Compare May 28, 2017 04:16

Krinkle reviewed Jul 4, 2017

View reviewed changes

trentmwillis force-pushed the assert-timeout branch from 32022c9 to 42a5db9 Compare July 4, 2017 04:54

Krinkle requested changes Jul 4, 2017

View reviewed changes

platinumazure approved these changes Jul 4, 2017

View reviewed changes

Assert: Introduce timeout to set per-test timeout durations

46b1446

trentmwillis force-pushed the assert-timeout branch from 42a5db9 to 29cc20a Compare July 6, 2017 02:57

Krinkle reviewed Jul 6, 2017

View reviewed changes

Trent Willis and others added 3 commits July 5, 2017 20:15

Tests: Add tests for assert.timeout

47f9b1b

Docs: Document assert.timeout

498d0bf

Build: Disable max-len ESLint rule for tests

07916ad

trentmwillis force-pushed the assert-timeout branch from 29cc20a to 07916ad Compare July 6, 2017 03:16

Krinkle reviewed Jul 6, 2017

View reviewed changes

Krinkle approved these changes Jul 6, 2017

View reviewed changes

trentmwillis merged commit 6fc5541 into qunitjs:master Jul 7, 2017

trentmwillis deleted the assert-timeout branch July 7, 2017 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert: Introduce timeout to set per-test timeout durations #1165

Assert: Introduce timeout to set per-test timeout durations #1165

trentmwillis commented Apr 20, 2017

Krinkle Apr 20, 2017

Krinkle Apr 20, 2017

trentmwillis Apr 20, 2017

Krinkle Apr 20, 2017 •

edited

Loading

Krinkle Apr 20, 2017

gibson042 Apr 21, 2017

trentmwillis May 28, 2017

platinumazure commented Apr 20, 2017

gibson042 Apr 21, 2017

Krinkle Jul 6, 2017

gibson042 Apr 21, 2017

gibson042 Apr 21, 2017

trentmwillis May 28, 2017

trentmwillis commented May 28, 2017

trentmwillis commented Jul 4, 2017

Krinkle Jul 4, 2017

trentmwillis Jul 4, 2017

Krinkle Jul 4, 2017 •

edited

Loading

trentmwillis Jul 4, 2017 •

edited

Loading

Krinkle Jul 5, 2017

platinumazure left a comment

trentmwillis commented Jul 6, 2017

Krinkle Jul 6, 2017

trentmwillis Jul 6, 2017

trentmwillis commented Jul 6, 2017

Krinkle Jul 6, 2017 •

edited

Loading

platinumazure commented Jul 6, 2017


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


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

Assert: Introduce timeout to set per-test timeout durations #1165

Assert: Introduce timeout to set per-test timeout durations #1165

Conversation

trentmwillis commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krinkle Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentmwillis commented May 28, 2017

trentmwillis commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krinkle Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

trentmwillis Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

trentmwillis commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentmwillis commented Jul 6, 2017

Krinkle Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

platinumazure commented Jul 6, 2017

Krinkle Apr 20, 2017 •

edited

Loading

Krinkle Jul 4, 2017 •

edited

Loading

trentmwillis Jul 4, 2017 •

edited

Loading

Krinkle Jul 6, 2017 •

edited

Loading