Promisification #39

vadimdemedes · 2015-09-06T12:29:09Z

This PR is related to #38. After this PR is complete and callbacks are removed to the maximum extent, AVA's code should be much cleaner.

Overview

As stated in #38, pinkie-promise and pify modules are used in this PR.

Changes

Promisify Runner.prototype.serial
Promisify Runner.prototype.concurrent
Promisify Runner.prototype.run
Promisify Test.prototype.run
Use promises in tests

As usual, let me know what you think and let's discuss this!

sindresorhus · 2015-09-06T12:50:25Z

Use mz module when promises are needed for core modules (e.g. fs)

You can just use pify for that. See pify.all().

vadimdemedes · 2015-09-06T12:50:55Z

Don't know why, but I knew you would say that, haha

vadimdemedes · 2015-09-06T14:28:36Z

@sindresorhus this PR is ready for you to review, let me know what you think ;)

sindresorhus · 2015-09-06T17:36:16Z

lib/runner.js

@@ -1,8 +1,10 @@
 'use strict';
+var Promise = require('pinkie-promise');


Actually. I think it's worth using https://github.com/petkaantonov/bluebird here for performance reasons.

sindresorhus · 2015-09-06T17:43:21Z

lib/test.js

-	this._timeStart = Date.now();
+	return new Promise(function (resolve, reject) {
+		this.promise.resolve = resolve;
+		this.promise.reject = reject;


I can't really think of a better way right now, but this feels weird.

Yeah, it does. My goal was to switch to promises with a minimum code changed.

Ok. Can you at least add a TODO in there so it's not forgotten that it needs improving?

sindresorhus · 2015-09-06T18:01:18Z

lib/runner.js

-		}.bind(this));
-	}.bind(this), cb);
+Runner.prototype.serial = function (tests) {
+	return Promise.resolve(tests).each(function (test) {


This inner function can be DRYed up as the Runner.prototype.concurrent has the exact same function content.

Yeah, thought of that too

Just wanted to get a quick feedback on the PR as a whole

sindresorhus · 2015-09-06T18:03:00Z

You can also remove pinkie-promise from the tests and devDependency and use Bluebird there too.

vadimdemedes · 2015-09-06T18:03:40Z

@sindresorhus Right, good catch, missed that

sindresorhus · 2015-09-06T18:05:28Z

The code base is so much cleaner with promises. Excellent work @vdemedes :)

Gonna give it a more thorough review tomorrow. Need sleep now.

vadimdemedes · 2015-09-06T18:05:58Z

Thank you @sindresorhus ;)

sindresorhus · 2015-09-07T08:14:34Z

I tried running this on pageres and got an error:

Unhandled rejection TypeError: Cannot read property 'testCount' of undefined
    at exit (/Users/sindresorhus/dev/ava/index.js:52:11)
    at tryCatcher (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/util.js:26:23)
    at Promise._settlePromiseFromHandler (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/promise.js:503:31)
    at Promise._settlePromiseAt (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/promise.js:577:18)
    at Promise._settlePromises (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/promise.js:693:14)
    at Async._drainQueue (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/async.js:123:16)
    at Async._drainQueues (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/async.js:133:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/Users/sindresorhus/dev/ava/node_modules/bluebird/js/main/async.js:15:14)
    at processImmediate [as _immediateCallback] (timers.js:371:17)

It seems the stats and results arguments in the exit function are undefined.

(Can you add a regression test for this when fixed?)

sindresorhus · 2015-09-07T08:18:37Z

lib/runner.js

-			if (err) {
-				this.stats.failCount++;
-			}
+Runner.prototype.addTestResult = function (test) {


This should be a private method, meaning prefixed with _: Runner.prototype._addTestResult

vadimdemedes · 2015-09-07T10:26:18Z

@sindresorhus Trying to figure out the right way to test index.js. It is a bit problematic, since it exposes only functions to add tests, so I can't test it without child_process.exec. What do you think?

sindresorhus · 2015-09-07T10:30:41Z

Using exec is fine.

vadimdemedes · 2015-09-07T10:51:42Z

@sindresorhus Pushed critical fixes and updated es6 test to use index.js, instead of lib/test.js directly. Also, pageres tests succeed now:

But now we have another problem. There's no continuous output, meaning that all output is displayed at the end, when all tests finish. This happens, because Promise.all (https://github.com/sindresorhus/ava/pull/39/files#diff-b3b53682a18f203ac8d29b0e277cad26R56) resolves only when all promises are resolved.

vadimdemedes · 2015-09-07T10:52:53Z

First thing that I came up with to fix that is to use each-async again, instead of Promise.all. Maybe you have some other solution in mind?

vadimdemedes · 2015-09-07T10:53:58Z

@sindresorhus Oh my God, I know how to fix this, never mind ;)

sindresorhus · 2015-09-07T10:54:48Z

Yeah, let's use each-async for now. Add a TODO about improving it though.

vadimdemedes · 2015-09-07T10:56:16Z

I came up with a much better solution and without use of each-async ;) Just a few seconds and you'll see the commit!

vadimdemedes · 2015-09-07T10:58:57Z

Here's that change: https://github.com/sindresorhus/ava/pull/39/files#diff-b3b53682a18f203ac8d29b0e277cad26R52.

The logic here is to output test results immediately, as they are available. But, use Promise.all to return from Runner#concurrent, when all the promises are resolved (tests are completed).

Close #39 PR Fixes #38

sindresorhus · 2015-09-08T02:07:23Z

Awesome! I'm really happy about this change. Super work @vdemedes :D

Would you happen to be interested in joining the project? I think you have a lot of good to bring to AVA.

Qix- · 2015-09-08T02:19:23Z

👍 for @vdemedes on the project. Lots of great stuff there.

vadimdemedes · 2015-09-08T04:17:02Z

@sindresorhus @Qix- Thank you, guys!

Yes, I am definitely interested in joining AVA! I really like it and would love to contribute more!

Close #39 PR Fixes #38

vdemedes added 2 commits September 6, 2015 15:20

promisify Runner#serial and Runner#concurrent

cb35ea0

pass Promise to pify module to work, when Promise is missing

4952ba0

vdemedes added 7 commits September 6, 2015 15:56

promisify Runner#run

49cdda6

update tests to use promise-based Runner#run

6248fbc

promisify Test#run

4fc926b

resolve Test#run with a test instance

9334bc2

fix error handling

ccd9702

make tests use promises

40c0271

fix es2015 test to work with promises

49f49a6

sindresorhus reviewed Sep 6, 2015
View reviewed changes

use bluebird instead of pinkie-promise

52377b4

sindresorhus reviewed Sep 6, 2015
View reviewed changes

use bluebird for control flow

9fca658

sindresorhus reviewed Sep 6, 2015
View reviewed changes

vdemedes added 2 commits September 6, 2015 21:06

separate test result handling into a function to DRY stuff up

f438b81

use bluebird in tests

a132e9f

sindresorhus reviewed Sep 7, 2015
View reviewed changes

vdemedes added 3 commits September 7, 2015 13:04

make Runner#addTestResult() a private method

74c6854

fix Runner#concurrent method

b854336

fix exit function to get stats & results from runner

7b54e17

vdemedes added 2 commits September 7, 2015 13:43

fix Runner#concurrent to return a test instance in case of error

ec01943

use main entrypoint in es6 test, instead of Test directly

5033b91

continuous output of test results in Runner#concurrent

dedc6fb

sindresorhus closed this in e47e85f Sep 8, 2015

sindresorhus pushed a commit that referenced this pull request Sep 8, 2015

Promisification

7357b69

Close #39 PR Fixes #38

sindresorhus pushed a commit that referenced this pull request Sep 8, 2015

Promisification

fa3ca81

Close #39 PR Fixes #38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promisification #39

Promisification #39

vadimdemedes commented Sep 6, 2015

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

sindresorhus Sep 6, 2015

sindresorhus Sep 6, 2015

vadimdemedes Sep 6, 2015

sindresorhus Sep 6, 2015

vadimdemedes Sep 6, 2015

sindresorhus Sep 6, 2015

vadimdemedes Sep 6, 2015

vadimdemedes Sep 6, 2015

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

sindresorhus commented Sep 7, 2015

sindresorhus Sep 7, 2015

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 8, 2015

Qix- commented Sep 8, 2015

vadimdemedes commented Sep 8, 2015

		@@ -1,8 +1,10 @@
		'use strict';
		var Promise = require('pinkie-promise');

Promisification #39

Promisification #39

Conversation

vadimdemedes commented Sep 6, 2015

Overview

Changes

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

sindresorhus commented Sep 6, 2015

vadimdemedes commented Sep 6, 2015

sindresorhus commented Sep 7, 2015

Choose a reason for hiding this comment

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

vadimdemedes commented Sep 7, 2015

sindresorhus commented Sep 8, 2015

Qix- commented Sep 8, 2015

vadimdemedes commented Sep 8, 2015