Async: Make tests Promise-aware #634

JamesMGreene · 2014-08-25T16:09:28Z

Fixes #632.
Ref #534.

~~I'm not loving how un-DRY the Test#run function is... but that's not new, only exacerbated by this PR.~~

Updated: This makes the setup, teardown, and test methods all aware that their return value may be a Promise in need of asynchronous resolution (resolve-ing).

Criticism or other suggestions welcomed, as usual.

cc: @domenic @stefanpenner

scottgonzalez · 2014-08-25T16:12:31Z

Why does this always add an assertion?

JamesMGreene · 2014-08-25T16:20:52Z

What would you propose as an alternative? We need to know if the Promise was fulfilled or rejected and reflect such in the test results.

JamesMGreene · 2014-08-25T16:22:14Z

Are you thinking more like a fulfilled Promise doesn't show up at all but a rejected Promise calls this.pushFailure rather than this.assert.whatever?

I'm not a huge fan of assertions that only show up in the UI conditionally (e.g. assert.expect). 👎

domenic · 2014-08-25T16:22:36Z

src/test.js

+						context.assert.ok( true, "Promise should be fulfilled" );
+						QUnit.start();
+					},
+					function() {


It seems unfortunate to ignore the rejection reason. When QUnit tests throw, do you ignore the exception?

Good point. I'll update that.

scottgonzalez · 2014-08-25T16:28:13Z

You could push a failure or only add the assertion for a rejected promise. The important part being that expect() should only include the assertions the user is implementing. For the failure case, I don't think it's terrible if the count is wrong because we've added an assertion, but if it's not necessary then we shouldn't add it.

JamesMGreene · 2014-08-25T16:29:16Z

Hmm... thoughts on @scottgonzalez's comments, @stefanpenner / @domenic?

stefanpenner · 2014-08-25T16:32:45Z

@JamesMGreene I agree with @scottgonzalez's suggestion, as a rejection detected here is the same as a unexpected throw in the test body of synchronous code.

stefanpenner · 2014-08-25T16:36:11Z

src/test.js

@@ -72,13 +74,38 @@ Test.prototype = {
 		this.callbackStarted = now();

 		if ( config.notrycatch ) {
-			this.callback.call( this.testEnvironment, this.assert );
-			this.callbackRuntime = now() - this.callbackStarted;
+			promise = this.callback.call( this.testEnvironment, this.assert );


if we can to be pedantic (with little cost) we can make the promise usage more spec aligned by not accessing then more then once.

if (promise !==null && typeof promise == 'object') { var then = promise.then; if (type then === 'function') { then.call(promise, ....); } }

Sure, can do.

Should the fulfillment callback be executed with the Promise as the context (this) as well? Not important for the QUnit code but was just curious for the unit test.

Can you point me to the spec that describes the check you posted? I've looked at this site, but that says that the promise itself can be an object and a function, which doesn't match your suggested approach.

That said, I'd like to simplify this, since we expect tests to only return undefined and promises, nothing else:

promise = this.callback.call( this.testEnvironment, this.assert ); if ( promise && typeof promise.then === "function" ) { [...] }

JamesMGreene · 2014-08-25T16:48:15Z

Updated:

No longer adding an assert
Including rejection reason in the failure output

JamesMGreene · 2014-08-25T17:00:36Z

Updated:

Pedantic Promise handling per @stefanpenner's suggestion

JamesMGreene · 2014-08-25T17:02:47Z

Rebased.

JamesMGreene · 2014-08-25T17:42:33Z

BTW, @stefanpenner: does Ember have any need for the setup and teardown methods to be Promise-aware as well?

jQuery/QUnit team folks: do you think setup and teardown should be Promise-aware as well?

jzaefferer · 2014-08-25T17:44:23Z

src/test.js

@@ -72,13 +74,50 @@ Test.prototype = {
 		this.callbackStarted = now();

 		if ( config.notrycatch ) {
-			this.callback.call( this.testEnvironment, this.assert );
-			this.callbackRuntime = now() - this.callbackStarted;


Why did you remove this line? Afaik its still needed.

Accidental, thanks for catching that.

JamesMGreene · 2014-08-25T17:51:17Z

Updated to restore the accidentally deleted line that @jzaefferer pointed out.

jzaefferer · 2014-08-25T17:53:18Z

src/test.js

+						promise,
+						QUnit.start,
+						function( e ) {
+							context.pushFailure( "Promise rejected on test #" + ( context.assertions.length + 1 ) + " " + context.stack + ": " + ( e.message || e ), extractStacktrace( e, 0 ) );


First of all, context.stack should be e.stack, right?

Even then, outputting both e.stack and the result of extractStacktrace( e, 0 ) looks like a bad idea to me - I don't think two stacktraces at once are useful. Though I would like to see an example of the actual output to confirm that suspicion.

This is just a duplicate of the standard line (with this changed to context) from the existing catch block a few lines further down.

this.pushFailure( "Died on test #" + ( this.assertions.length + 1 ) + " " + this.stack + ": " + ( e.message || e ), extractStacktrace( e, 0 ) );

Test#stack is from src/core.js#L111

Thanks, that makes sense. Should've looked at the full file.

JamesMGreene · 2014-08-25T18:02:52Z

Outstanding Question: Should the setup/teardown methods be Promise-aware as well?

I'm thinking yes.

jzaefferer · 2014-08-25T19:09:20Z

src/test.js

 			this.callbackRuntime = now() - this.callbackStarted;
+			if ( promise !== null && typeof promise === "object" ) {


I posted this before, but it got lost in an update:

Can someone point me to the spec that describes the check @stefanpenner posted? I've looked at this site, but that says that the promise itself can be an object or a function, which doesn't match the suggested approach.

That said, I'd like to simplify this, since we expect tests to only return undefined and promises, nothing else:

promise = this.callback.call( this.testEnvironment, this.assert ); if ( promise && typeof promise.then === "function" ) { [...] }

the only part of the spec I'm trying to follow is not access then property more then once. Your example once again violates this.

That being said, your check does correctly check if promise itself is a function which my example fails to add, good catch!

Thanks for clarifying. The reasoning is explained in this note, which seems odd, but okay to follow.

Since we have to check if promise is defined first, this still needs to be a bit more complicated then I outlined above.

in theory people can implement a custom "then" that has side-effects when invoked. This can be malicious, in-error, or some other funkyness.

JamesMGreene · 2014-08-25T19:28:40Z

TODO:

Extract code around the new Promise-aware code into a central, DRY location
Update the Test#setup and Test#teardown methods to also be Promise-aware

leobalter · 2014-08-27T19:08:02Z

Should the setup/teardown methods be Promise-aware as well?

yes

JamesMGreene · 2014-08-28T14:04:10Z

Updated. Please re-review, thanks!

JamesMGreene · 2014-08-28T14:06:04Z

Sorry, rebased.

jzaefferer · 2014-08-28T14:23:51Z

test/test.js

+	// Return a mock self-fulfilling Promise ("thenable")
+	var thenable = {
+		then: function( fulfilledCallback /*, rejectedCallback */ ) {
+			assert.ok( true, "`then` function was called" );


Let's drop this one, since there's an assertion here anyway. This kind of marker ("I was here!") is only useful when there's nothing else.

OK, will do.

jzaefferer · 2014-08-28T14:30:10Z

Overall this is looking good. I'd like to reduce the amount of seemingly repetitive tests. Any of those that don't actually increase code coverage should be deleted.

We should also figure out how to do some "real world testing" with this. Build the dist file and put it into some Ember tests or whatever might already be using something similar. The current tests don't even use "real" Promises, so its hard to tell what we might be missing.

JamesMGreene · 2014-08-28T14:30:19Z

src/test.js

+				test = this;
+		if ( promise != null ) {
+			then = promise.then;
+			if ( typeof then === "function" ) {


Should I be preferring QUnit.objectType( then ) === "function" here rather than using the raw JS? I don't see any benefit to that [when checking for a function] other than consistency with other parts of the library but we're pretty inconsistent across the board right now.

leobalter · 2014-09-03T01:22:00Z

test/promise.js

@@ -0,0 +1,74 @@
+// NOTE: Adds 1 assertion
+function createMockPromise( assert ) {
+	// Return a mock self-fulfilling Promise ("thenable")


missing empty line

JamesMGreene · 2014-09-03T13:10:27Z

So I think one potential issue may be that the global beforeEach/afterEach should really be allowed to fully synchronize before the module-level beforeEach/afterEach is called, otherwise race conditions may be introduced.

JamesMGreene · 2014-09-03T13:47:17Z

Added a 2nd separate commit to deal with the synchronization of the global beforeEach/afterEach callbacks before running the module-level beforeEach/afterEach calls but I'm not too fond of it... especially as it actually would still be an issue for async scenarios other than Promises.

I think I'd rather see them extracted to separate synchronize calls in the run method rather than lumped together into the test.before/test.after method via test.hooks, which is a realistic bug not covered by PR #635.

JamesMGreene · 2014-09-03T13:54:29Z

@stefanpenner: This one is taking much more persistence than initially expected! You're going to owe me. 😉

JamesMGreene · 2014-09-03T14:18:28Z

Created Issue #647 to keep track of getting that synchronization issue resolved. After that, I should be able to undo/clean this 2nd commit.

Ref qunitjs#634 (comment)

JamesMGreene · 2014-09-03T21:19:10Z

Waiting on the resolution of PR #650, then I'll update this PR.

JamesMGreene · 2014-09-04T17:32:32Z

Now deferring for PR #653 to be merged first as I think it may resolve some of the new test failures I'm getting on this branch as well.

JamesMGreene · 2014-09-04T17:36:56Z

This PR is now built on top of pending/unmerged PR #653. Doing so does indeed eliminate the 3 unexpected test failures that recently popped up in this branch.

You can isolate just the changes necessary for this particular PR (making tests Promise-aware) by viewing this comparison:
async-done...promise-aware

Fixes #632 Ref #534 Closes #634

JamesMGreene · 2014-09-11T21:09:42Z

Rebased after merging PR #653.

leobalter · 2014-09-11T21:20:44Z

LGTM, merging

JamesMGreene · 2014-09-11T21:44:14Z

At long last... victory! ✌️ 😀

domenic reviewed Aug 25, 2014
View reviewed changes

stefanpenner reviewed Aug 25, 2014
View reviewed changes

jzaefferer reviewed Aug 25, 2014
View reviewed changes

Krinkle changed the title ~~Async: Tests are now Promise-aware~~ Async: Make tests Promise-aware Aug 25, 2014

JamesMGreene mentioned this pull request Aug 26, 2014

Should use Error#stack without throwing when available #636

Closed

jzaefferer reviewed Aug 28, 2014
View reviewed changes

JamesMGreene reviewed Aug 28, 2014
View reviewed changes

leobalter reviewed Sep 3, 2014
View reviewed changes

JamesMGreene mentioned this pull request Sep 3, 2014

Global beforeEach/afterEach should synchronize before module beforeEach/afterEach run #647

Closed

leobalter added a commit to leobalter/qunit that referenced this pull request Sep 3, 2014

Test: Remove unused code

4612063

Ref qunitjs#634 (comment)

leobalter mentioned this pull request Sep 3, 2014

Test: Remove unused code #649

Merged

JamesMGreene mentioned this pull request Sep 4, 2014

Test: Fix test hooks order #650

Closed

JamesMGreene force-pushed the master branch from 103c75a to 76ff48c Compare September 4, 2014 15:57

JamesMGreene mentioned this pull request Sep 4, 2014

Async: Implement new assert.async mechanism #653

Merged

Async: Tests are now Promise-aware

be3dce3

Fixes #632 Ref #534 Closes #634

leobalter merged commit be3dce3 into qunitjs:master Sep 11, 2014

JamesMGreene deleted the promise-aware branch September 11, 2014 21:55

JamesMGreene mentioned this pull request Sep 12, 2014

Document QUnit.test's support for Promises qunitjs/api.qunitjs.com#80

Closed

		this.callbackRuntime = now() - this.callbackStarted;
		if ( promise !== null && typeof promise === "object" ) {

Async: Make tests Promise-aware #634

Async: Make tests Promise-aware #634

Conversation

JamesMGreene commented Aug 25, 2014

scottgonzalez commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottgonzalez commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

stefanpenner commented Aug 25, 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

JamesMGreene commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

JamesMGreene commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesMGreene commented Aug 25, 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

JamesMGreene commented Aug 25, 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

JamesMGreene commented Aug 25, 2014

leobalter commented Aug 27, 2014

JamesMGreene commented Aug 28, 2014

JamesMGreene commented Aug 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzaefferer commented Aug 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesMGreene commented Sep 3, 2014

JamesMGreene commented Sep 3, 2014

JamesMGreene commented Sep 3, 2014

JamesMGreene commented Sep 3, 2014

JamesMGreene commented Sep 3, 2014

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 4, 2014

JamesMGreene commented Sep 11, 2014

leobalter commented Sep 11, 2014

JamesMGreene commented Sep 11, 2014