Async: Implement new assert.async mechanism #653

JamesMGreene · 2014-09-04T17:15:05Z

Fixes #534.

Typical Usage Example:

QUnit.test( "x", function( assert ) {
  assert.expect( 1 );
  var done = assert.async();
  setTimeout(function() {
    assert.ok( true );
    done();
  }, 250);
});

This PR should now be merged before #634 (which I will then rebase).

JamesMGreene · 2014-09-04T17:16:41Z

For QUnit v2.x, I think we should change QUnit.start/QUnit.stop to be straight-forward block/unblock without a semaphore (or rename them altogether to avoid confusion with the v1.x behavior) but they still need access to the semaphore for backward compatibility right now.

leobalter · 2014-09-04T17:37:32Z

src/core.js

+					config.current.semaphore = 1;
+					config.semaphore = 0;
+				} else {
+					QUnit.assert.ok( false, "Test timed out" );


use pushFailure instead, if there's no current test it's the best option here

OK, just a holdover from the existing code.

leobalter · 2014-09-04T17:47:28Z

I'm missing also the deprecation messages on asyncTest and QUnit.config.semaphore.

leobalter · 2014-09-04T17:57:33Z

I also vote for deprecating QUnit.stop() and keeping QUnit.start() to only handle the global config.autostart. Tests' semaphores should be handled only by assert.async, that shouldn't care about global semaphore.

For 2.0.0, I would like to make all tests assert.async() by default, making them to fill expected assertions or calling assert.done, as previously mentioned by @jzaefferer.

JamesMGreene · 2014-09-04T18:05:12Z

To make tests async by default, we would need to either:

provide the initially created done function somewhere, e.g. as an additional argument passed to the function callback

QUnit.test( "x", function( assert, done ) {
  /* ... */
  done();
});

or as a contextual property (e.g. this.done();), which may have conflicts when migrating from v1.x to v2.x.

leobalter · 2014-09-04T18:19:33Z

also as assert.done();, no?

anyway, let's bring this 2.0.0 discussion to another issue so here we can focus on the changes on this PR.

JamesMGreene · 2014-09-04T18:44:42Z

also as assert.done();, no?

assert.done is not planned.

JamesMGreene · 2014-09-04T18:45:53Z

I'm missing also the deprecation messages on asyncTest and QUnit.config.semaphore.

Added deprecation warnings for QUnit.start, QUnit.stop, QUnit.asyncTest, and QUnit.config.semaphore.

jzaefferer · 2014-09-04T18:53:58Z

For 2.0.0, I would like to make all tests assert.async() by default, making them to fill expected assertions or calling assert.done, as previously mentioned by @jzaefferer.

That's not really what I suggested (I hope). With the two approaches we're going for now, I don't think we really need something else.

The third approach would be to make assert.expect() trigger an async test, then running the specified of assertions ending the test. That wouldn't work for expect(0), which we currently support. There's probably more reasons why this isn't great, at least in the context of our existing APIs.

jzaefferer · 2014-09-04T18:56:39Z

Please ignore style issues with lines that are too long until #654 is addressed, which will help fix these. That kind of issue is just distracting from the important parts here.

gibson042 · 2014-09-04T19:00:04Z

Other than that, \o/ \o/ \o/

This is great.

JamesMGreene · 2014-09-05T01:28:59Z

Thanks, @gibson042!

@jzaefferer @leobalter @Krinkle: Did you guys want to review this PR further? I'm not sure I have anyone's stamp of approval yet.

leobalter · 2014-09-05T15:43:37Z

test/async.js

+		setTimeout(function() {
+			currentTest.state = "beforeEach";
+			done();
+		}, 50);


missing space

leobalter · 2014-09-05T15:44:29Z

LGTM, so I reported only few style issues I saw

JamesMGreene · 2014-09-05T15:48:19Z

@leobalter: All fixed, and a few more.

JamesMGreene · 2014-09-05T15:54:02Z

So @leobalter and @gibson042 have approved now.

@jzaefferer @Krinkle @scottgonzalez, did any of you want to review this further before it gets merged?

Forewarning: The Promise PR (#634) will be ready for rebasing for further review and/or merging immediately after this one is merged, so you should also go review that one (which contains this PR as well) before I have to pester you. 😜

jzaefferer · 2014-09-05T19:57:53Z

reporter/html.js

@@ -15,6 +15,8 @@ QUnit.init = function() {
 	config.autorun = false;
 	config.filter = "";
 	config.queue = [];
+
+	// DEPRECATED: QUnit.config.semaphore will be removed in QUnit 2.0.


This should be fine, but since this property was never in the public API, it's not really necessary. I also see no reason why something outside QUnit should ever fiddle with this, except as workarounds.

Removed, per implementation of #653 (comment)

jzaefferer · 2014-09-05T20:21:04Z

test/autostart.js

@@ -0,0 +1,6 @@
+QUnit.start();


Calling start() again here doesn't cause any failure. It should, and it does in master.

I added a test for it locally and it does cause a failure for me, so...?

Added many tests for the various start/stop error scenarios mentioned in #653 (comment)

jzaefferer · 2014-09-05T20:22:19Z

Overall looking very promising. No game-breaker, just some details that we need to iron out. No need to discuss setTimeout support here, I'll open a separate ticket for that.

JamesMGreene · 2014-09-09T20:00:37Z

The new commit implemented all of the error scenario rules for QUnit.start/QUnit.stop per my comment #653 (comment), which I am still currently assuming are what @jzaefferer was implying in his comment #653 (comment).

jzaefferer · 2014-09-11T14:15:18Z

src/core.js

-				QUnit.start();
-			}, config.testTimeout );
+
+		// If there isn't a test running, don't allow QUni.stop() to be called


What's QUni?

Whoops. Fixed. :)

leobalter · 2014-09-11T16:51:19Z

LGTM

Enforce test-bound start/stop calls Fixes #534 Closes #653

Also adds minor test cleanup Fixes #659

JamesMGreene mentioned this pull request Sep 4, 2014

Async: Make tests Promise-aware #634

Merged

leobalter reviewed Sep 4, 2014
View reviewed changes

leobalter reviewed Sep 5, 2014
View reviewed changes

test/async.js

setTimeout(function() {

currentTest.state = "beforeEach";

done();

}, 50);

Copy link

Member

leobalter Sep 5, 2014

Choose a reason for hiding this comment

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

missing space

This was referenced Sep 5, 2014

Remove QUnit.stop, update QUnit.start to only work globally #655

Closed

Remove QUnit.asyncTest #656

Closed

Remove expected argument from test() and asyncTest() #501

Closed

Remove private async parameter from QUnit.test #657

Closed

jzaefferer reviewed Sep 5, 2014
View reviewed changes

Krinkle changed the title ~~Async: Implement new assert.async mechanism~~ Sep 9, 2014

jzaefferer reviewed Sep 11, 2014
View reviewed changes

jzaefferer mentioned this pull request Sep 11, 2014

Defer "begin" callback until test run actually starts #659

Closed

JamesMGreene added 2 commits September 11, 2014 15:44

Async: Implement assert.async

680901a

Enforce test-bound start/stop calls Fixes #534 Closes #653

Logging: Defer begin event till tests actually starts

bcfaf7e

Also adds minor test cleanup Fixes #659

JamesMGreene merged commit bcfaf7e into qunitjs:master Sep 11, 2014

JamesMGreene deleted the async-done branch September 11, 2014 21:44

This was referenced Sep 12, 2014

Document QUnit.assert.async qunitjs/api.qunitjs.com#79

Closed

Remove deprecated asyncTest usage in favor of assert.async for QUnit v1.16.0+ JamesMGreene/qunit-composite#9

Closed

JamesMGreene mentioned this pull request Mar 14, 2015

Lax qunitjs version from "1.10.0" to "^1.10.0" qunitjs/node-qunit#118

Closed

This was referenced Jan 5, 2018

Core: Remove delay from setTimeouts in processing code #1246

Merged

the testing process too slow #1245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async: Implement new assert.async mechanism #653

Async: Implement new assert.async mechanism #653

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

leobalter Sep 4, 2014

JamesMGreene Sep 4, 2014

JamesMGreene Sep 4, 2014

leobalter commented Sep 4, 2014

leobalter commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

leobalter commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

jzaefferer commented Sep 4, 2014

jzaefferer commented Sep 4, 2014

gibson042 commented Sep 4, 2014

JamesMGreene commented Sep 5, 2014

leobalter Sep 5, 2014

leobalter commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

jzaefferer Sep 5, 2014

JamesMGreene Sep 9, 2014

jzaefferer Sep 5, 2014

JamesMGreene Sep 9, 2014

JamesMGreene Sep 9, 2014

jzaefferer commented Sep 5, 2014

JamesMGreene commented Sep 9, 2014

jzaefferer Sep 11, 2014

leobalter Sep 11, 2014

JamesMGreene Sep 11, 2014

leobalter commented Sep 11, 2014

Async: Implement new assert.async mechanism #653

Async: Implement new assert.async mechanism #653

Conversation

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Sep 4, 2014

leobalter commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

leobalter commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

jzaefferer commented Sep 4, 2014

jzaefferer commented Sep 4, 2014

gibson042 commented Sep 4, 2014

JamesMGreene commented Sep 5, 2014

Choose a reason for hiding this comment

leobalter commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

JamesMGreene commented Sep 5, 2014

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

jzaefferer commented Sep 5, 2014

JamesMGreene commented Sep 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Sep 11, 2014