Skip to content

Promisification #39

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.

Sign up for GitHub

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

Closed
wants to merge 19 commits into from
Closed

Promisification #39

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cb35ea0
promisify Runner#serial and Runner#concurrent
Sep 6, 2015
4952ba0
pass Promise to `pify` module to work, when Promise is missing
Sep 6, 2015
49cdda6
promisify Runner#run
Sep 6, 2015
6248fbc
update tests to use promise-based Runner#run
Sep 6, 2015
4fc926b
promisify Test#run
Sep 6, 2015
9334bc2
resolve Test#run with a test instance
Sep 6, 2015
ccd9702
fix error handling
Sep 6, 2015
40c0271
make tests use promises
Sep 6, 2015
49f49a6
fix es2015 test to work with promises
Sep 6, 2015
52377b4
use bluebird instead of pinkie-promise
Sep 6, 2015
9fca658
use bluebird for control flow
Sep 6, 2015
f438b81
separate test result handling into a function to DRY stuff up
Sep 6, 2015
a132e9f
use bluebird in tests
Sep 6, 2015
74c6854
make Runner#addTestResult() a private method
Sep 7, 2015
b854336
fix Runner#concurrent method
Sep 7, 2015
7b54e17
fix exit function to get stats & results from runner
Sep 7, 2015
ec01943
fix Runner#concurrent to return a test instance in case of error
Sep 7, 2015
5033b91
use main entrypoint in es6 test, instead of Test directly
Sep 7, 2015
dedc6fb
continuous output of test results in Runner#concurrent
Sep 7, 2015
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
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function exit(stats, results) {

setImmediate(function () {
runner.on('test', test);
runner.run(exit);
runner.run().then(exit);
});

module.exports = runner.addTest.bind(runner);
Expand Down
129 changes: 74 additions & 55 deletions lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
'use strict';
var Promise = require('pinkie-promise');
Copy link
Member

Choose a reason for hiding this comment

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

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

var promisify = require('pify');
var util = require('util');
var EventEmitter = require('events').EventEmitter;
var each = require('each-async');
var eachSerial = require('async-each-series');
var each = promisify(require('each-async'), Promise);
Copy link
Member

Choose a reason for hiding this comment

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

Just use Promise.all()

var eachSerial = promisify(require('async-each-series'), Promise);
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/petkaantonov/bluebird/blob/master/API.md#eachfunction-iterator---promise

var Test = require('./test');

function Runner(opts) {
Expand Down Expand Up @@ -44,67 +46,84 @@ Runner.prototype.addAfterHook = function (title, cb) {
this.tests.after.push(new Test(title, cb));
};

Runner.prototype.concurrent = function (tests, cb) {
each(tests, function (test, i, next) {
test.run(function (err, duration) {
if (err) {
this.stats.failCount++;
}

this.results.push({
duration: duration,
title: test.title,
error: err
});

this.emit('test', err, test.title, duration);
next();
}.bind(this));
}.bind(this), cb);
Runner.prototype.concurrent = function (tests) {
return each(tests, function (test, i, next) {
test.run()
.catch(function () {
return;
})
.then(function () {
if (test.assertError) {
this.stats.failCount++;
}

this.results.push({
duration: test.duration,
title: test.title,
error: test.assertError
});

this.emit('test', test.assertError, test.title, test.duration);
next();
}.bind(this));
}.bind(this));
};

Runner.prototype.serial = function (tests, cb) {
eachSerial(tests, function (test, next) {
test.run(function (err, duration) {
if (err) {
this.stats.failCount++;
}

this.results.push({
duration: duration,
title: test.title,
error: err
});

this.emit('test', err, test.title, duration);
next();
}.bind(this));
}.bind(this), cb);
Runner.prototype.serial = function (tests) {
return eachSerial(tests, function (test, next) {
test.run()
.catch(function () {
return;
})
.then(function () {
if (test.assertError) {
this.stats.failCount++;
}

this.results.push({
duration: test.duration,
title: test.title,
error: test.assertError
});

this.emit('test', test.assertError, test.title, test.duration);
next();
}.bind(this));
}.bind(this));
};

Runner.prototype.run = function (cb) {
Runner.prototype.run = function () {
var concurrent = this.tests.concurrent;
var serial = this.tests.serial;
var before = this.tests.before;
var after = this.tests.after;

// TODO: refactor this bullshit
this.serial(before, function () {
if (this.stats.failCount > 0) {
return this.end(cb);
}

this.serial(serial, function () {
this.concurrent(concurrent, function () {
this.serial(after, function () {
this.end(cb);
}.bind(this));
}.bind(this));
}.bind(this));
}.bind(this));
};
var self = this;

Runner.prototype.end = function (cb) {
this.stats.passCount = this.stats.testCount - this.stats.failCount;
cb(this.stats, this.results);
return this.serial(before)
.then(function () {
if (self.stats.failCount > 0) {
return Promise.reject();
Copy link
Member

Choose a reason for hiding this comment

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

Should we reject with an error with a error message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no error message before, so I did not insert it too. Also, there wouldn't be a meaningful message, only smth like "Error happened during tests" and that's it. The goal of this line is just to prevent execution of the following tests.

}
})
.then(function () {
return self.serial(serial);
})
.then(function () {
return self.concurrent(concurrent);
})
.then(function () {
return self.serial(after);
})
.catch(function () {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you prefer .then(this.concurrent.bind(this, concurrent))?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he means why just return from 119 and not doing anything on the catch.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I should have been clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right, I am just checking notes from the PR, not from the files themselves.

Because previously, when error happened, this.end executed and that's it. So that's exactly what I did here. In case of an error, just exit.

})
.then(function () {
self.stats.passCount = self.stats.testCount - self.stats.failCount;
});
};

// Runner.prototype.end = function (cb) {
// this.stats.passCount = this.stats.testCount - this.stats.failCount;
// cb(this.stats, this.results);
// };
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove that

58 changes: 34 additions & 24 deletions lib/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict';
var Promise = require('pinkie-promise');
var setImmediate = require('set-immediate-shim');
var util = require('util');
var assert = require('assert');
Expand Down Expand Up @@ -66,34 +67,39 @@ Test.prototype.skip = function () {
this.skipTest = true;
};

Test.prototype.run = function (cb) {
this.cb = cb;
Test.prototype.run = function () {
this.promise = {};

if (!this.fn || this.skipTest) {
this.exit();
}

this._timeStart = Date.now();
return new Promise(function (resolve, reject) {
this.promise.resolve = resolve;
this.promise.reject = reject;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure


try {
var ret = this.fn(this);
if (!this.fn || this.skipTest) {
return this.exit();
}

if (ret && typeof ret.then === 'function') {
ret.then(this.exit.bind(this)).catch(function (err) {
this.assertError = new assert.AssertionError({
actual: err,
message: 'Promise rejected → ' + err,
operator: 'promise',
stackStartFunction: this
});
this._timeStart = Date.now();

this.exit();
}.bind(this));
try {
var ret = this.fn(this);

if (ret && typeof ret.then === 'function') {
ret.then(this.exit.bind(this)).catch(function (err) {
this.assertError = new assert.AssertionError({
actual: err,
message: 'Promise rejected → ' + err,
operator: 'promise',
stackStartFunction: this
});

this.exit();
}.bind(this));
}
} catch (err) {
this.assertError = err;
this.exit();
}
} catch (err) {
this.assertError = err;
this.exit();
}
}.bind(this));
};

Test.prototype.end = function () {
Expand Down Expand Up @@ -124,7 +130,11 @@ Test.prototype.exit = function () {
this.ended = true;

setImmediate(function () {
this.cb(this.assertError, this.duration);
if (this.assertError) {
return this.promise.reject(this.assertError);
}

this.promise.resolve(this);
}.bind(this));
}
};
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
"fn-name": "^2.0.0",
"globby": "^3.0.1",
"meow": "^3.3.0",
"pify": "^2.0.0",
"pinkie-promise": "^1.0.0",
"plur": "^2.0.0",
"pretty-ms": "^2.0.0",
"resolve-from": "^1.0.0",
Expand Down
2 changes: 1 addition & 1 deletion test/es2015.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ test('run test', t => {
ava('foo', a => {
a.true(false);
a.end();
}).run(err => {
}).run().catch(err => {
t.true(err);
t.end();
});
Expand Down
Loading