-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Promisification #39
Changes from all commits
cb35ea0
4952ba0
49cdda6
6248fbc
4fc926b
9334bc2
ccd9702
40c0271
49f49a6
52377b4
9fca658
f438b81
a132e9f
74c6854
b854336
7b54e17
ec01943
5033b91
dedc6fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
'use strict'; | ||
var Promise = require('bluebird'); | ||
var util = require('util'); | ||
var EventEmitter = require('events').EventEmitter; | ||
var each = require('each-async'); | ||
var eachSerial = require('async-each-series'); | ||
var Test = require('./test'); | ||
|
||
function Runner(opts) { | ||
|
@@ -44,67 +43,73 @@ 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++; | ||
} | ||
Runner.prototype.concurrent = function (tests) { | ||
// run all tests | ||
tests = tests.map(function (test) { | ||
// in case of error, don't reject a promise | ||
return test.run().catch(function () { | ||
return; | ||
}).then(function () { | ||
this._addTestResult(test); | ||
}.bind(this)); | ||
}, this); | ||
|
||
this.results.push({ | ||
duration: duration, | ||
title: test.title, | ||
error: err | ||
}); | ||
return Promise.all(tests); | ||
}; | ||
|
||
this.emit('test', err, test.title, duration); | ||
next(); | ||
}.bind(this)); | ||
}.bind(this), cb); | ||
Runner.prototype.serial = function (tests) { | ||
return Promise.resolve(tests).each(function (test) { | ||
return test.run() | ||
.catch(function () { | ||
return; | ||
}) | ||
.then(function () { | ||
this._addTestResult(test); | ||
}.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++; | ||
} | ||
Runner.prototype._addTestResult = function (test) { | ||
if (test.assertError) { | ||
this.stats.failCount++; | ||
} | ||
|
||
this.results.push({ | ||
duration: duration, | ||
title: test.title, | ||
error: err | ||
}); | ||
this.results.push({ | ||
duration: test.duration, | ||
title: test.title, | ||
error: test.assertError | ||
}); | ||
|
||
this.emit('test', err, test.title, duration); | ||
next(); | ||
}.bind(this)); | ||
}.bind(this), cb); | ||
this.emit('test', test.assertError, test.title, test.duration); | ||
}; | ||
|
||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we reject with an error with a error message here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, I should have been clearer. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, |
||
}) | ||
.then(function () { | ||
self.stats.passCount = self.stats.testCount - self.stats.failCount; | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
'use strict'; | ||
var Promise = require('bluebird'); | ||
var setImmediate = require('set-immediate-shim'); | ||
var util = require('util'); | ||
var assert = require('assert'); | ||
|
@@ -66,34 +67,40 @@ 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(); | ||
// TODO: refactor this to avoid storing the promise | ||
return new Promise(function (resolve, reject) { | ||
this.promise.resolve = resolve; | ||
this.promise.reject = reject; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Can you at least add a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () { | ||
|
@@ -124,7 +131,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)); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,6 @@ | ||
import test from 'tape'; | ||
import ava from '../lib/test'; | ||
import test from '../'; | ||
|
||
test('run test', t => { | ||
ava('foo', a => { | ||
a.true(false); | ||
a.end(); | ||
}).run(err => { | ||
t.true(err); | ||
t.end(); | ||
}); | ||
t.true(false); | ||
t.end(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inner function can be DRYed up as the
Runner.prototype.concurrent
has the exact same function content.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thought of that too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to get a quick feedback on the PR as a whole