-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix promise assertions #360
Changes from all commits
3d171ff
c873845
23c619c
796cb0b
913b78b
7a11cb1
a8d622b
5461a64
716b37d
0063076
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 |
---|---|---|
|
@@ -25,11 +25,18 @@ function Test(title, fn) { | |
|
||
this.title = title || fnName(fn) || '[anonymous]'; | ||
this.fn = isGeneratorFn(fn) ? co.wrap(fn) : fn; | ||
this.assertCount = 0; | ||
this.assertions = []; | ||
this.planCount = null; | ||
this.duration = null; | ||
this.assertError = undefined; | ||
|
||
Object.defineProperty(this, 'assertCount', { | ||
enumerable: true, | ||
get: function () { | ||
return this.assertions.length; | ||
} | ||
}); | ||
|
||
// TODO(jamestalmage): make this an optional constructor arg instead of having Runner set it after the fact. | ||
// metadata should just always exist, otherwise it requires a bunch of ugly checks all over the place. | ||
this.metadata = {}; | ||
|
@@ -46,8 +53,8 @@ function Test(title, fn) { | |
|
||
module.exports = Test; | ||
|
||
Test.prototype._assert = function () { | ||
this.assertCount++; | ||
Test.prototype._assert = function (promise) { | ||
this.assertions.push(promise); | ||
}; | ||
|
||
Test.prototype._setAssertError = function (err) { | ||
|
@@ -162,38 +169,50 @@ Test.prototype._end = function (err) { | |
this.exit(); | ||
}; | ||
|
||
Test.prototype.exit = function () { | ||
var self = this; | ||
|
||
// calculate total time spent in test | ||
this.duration = globals.now() - this._timeStart; | ||
|
||
// stop infinite timer | ||
globals.clearTimeout(this._timeout); | ||
|
||
if (this.assertError === undefined && this.planCount !== null && this.planCount !== this.assertCount) { | ||
Test.prototype._checkPlanCount = function () { | ||
if (this.assertError === undefined && this.planCount !== null && this.planCount !== this.assertions.length) { | ||
this._setAssertError(new assert.AssertionError({ | ||
actual: this.assertCount, | ||
actual: this.assertions.length, | ||
expected: this.planCount, | ||
message: 'Assertion count does not match planned', | ||
operator: 'plan' | ||
})); | ||
|
||
this.assertError.stack = this.planStack; | ||
} | ||
}; | ||
|
||
if (!this.ended) { | ||
this.ended = true; | ||
Test.prototype.exit = function () { | ||
var self = this; | ||
|
||
globals.setImmediate(function () { | ||
if (self.assertError !== undefined) { | ||
self.promise.reject(self.assertError); | ||
return; | ||
} | ||
this._checkPlanCount(); | ||
|
||
Promise.all(this.assertions) | ||
.catch(function (err) { | ||
self._setAssertError(err); | ||
}) | ||
.finally(function () { | ||
// calculate total time spent in test | ||
self.duration = globals.now() - self._timeStart; | ||
|
||
self.promise.resolve(self); | ||
// stop infinite timer | ||
globals.clearTimeout(self._timeout); | ||
|
||
self._checkPlanCount(); | ||
|
||
if (!self.ended) { | ||
self.ended = true; | ||
|
||
globals.setImmediate(function () { | ||
if (self.assertError !== undefined) { | ||
self.promise.reject(self.assertError); | ||
return; | ||
} | ||
|
||
self.promise.resolve(self); | ||
}); | ||
} | ||
}); | ||
} | ||
}; | ||
|
||
Test.prototype._publicApi = function () { | ||
|
@@ -230,33 +249,23 @@ Test.prototype._publicApi = function () { | |
api.plan = this.plan.bind(this); | ||
|
||
function skipFn() { | ||
self._assert(); | ||
self._assert(Promise.resolve()); | ||
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 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 we should keep that here, as it makes it more obvious what we're doing, and it makes it a _lot_ easier to reason about 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. Agreed. I wrote this before I saw how you optimized the |
||
} | ||
|
||
function onAssertionEvent(event) { | ||
var promise; | ||
if (event.assertionThrew) { | ||
event.error.powerAssertContext = event.powerAssertContext; | ||
event.error.originalMessage = event.originalMessage; | ||
self._setAssertError(event.error); | ||
self._assert(); | ||
return null; | ||
promise = Promise.reject(event.error); | ||
} else { | ||
promise = Promise.resolve(observableToPromise(event.returnValue)); | ||
} | ||
|
||
var fn = observableToPromise(event.returnValue); | ||
|
||
if (isPromise(fn)) { | ||
return Promise.resolve(fn) | ||
.catch(function (err) { | ||
err.originalMessage = event.originalMessage; | ||
self._setAssertError(err); | ||
}) | ||
.finally(function () { | ||
self._assert(); | ||
}); | ||
} | ||
|
||
self._assert(); | ||
return null; | ||
promise = promise | ||
.catch(function (err) { | ||
err.originalMessage = event.originalMessage; | ||
return Promise.reject(err); | ||
}); | ||
self._assert(promise); | ||
} | ||
|
||
var enhanced = enhanceAssert({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
'use strict'; | ||
var test = require('tap').test; | ||
var Promise = global.Promise = require('bluebird'); | ||
var delay = require('delay'); | ||
var _ava = require('../lib/test'); | ||
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. var Promise = require('bluebird'); Promise is not natively available in Node |
||
|
||
function ava() { | ||
|
@@ -351,3 +353,119 @@ test('skipped assertions count towards the plan', function (t) { | |
t.end(); | ||
}); | ||
}); | ||
|
||
test('throws and doesNotThrow work with promises', function (t) { | ||
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. make two separate tests. |
||
var asyncCalled = false; | ||
ava(function (a) { | ||
a.plan(2); | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
a.doesNotThrow(delay(20).then(function () { | ||
asyncCalled = true; | ||
})); | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 2); | ||
t.is(a.assertCount, 2); | ||
t.is(asyncCalled, true); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('waits for t.throws to resolve after t.end is called', function (t) { | ||
ava.cb(function (a) { | ||
a.plan(1); | ||
a.doesNotThrow(delay(10), 'foo'); | ||
a.end(); | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 1); | ||
t.is(a.assertCount, 1); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('waits for t.throws to reject after t.end is called', function (t) { | ||
ava.cb(function (a) { | ||
a.plan(1); | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
a.end(); | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 1); | ||
t.is(a.assertCount, 1); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('waits for t.throws to resolve after the promise returned from the test resolves', function (t) { | ||
ava(function (a) { | ||
a.plan(1); | ||
a.doesNotThrow(delay(10), 'foo'); | ||
return Promise.resolve(); | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 1); | ||
t.is(a.assertCount, 1); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('waits for t.throws to reject after the promise returned from the test resolves', function (t) { | ||
ava(function (a) { | ||
a.plan(1); | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
return Promise.resolve(); | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 1); | ||
t.is(a.assertCount, 1); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('multiple resolving and rejecting promises passed to t.throws/t.doesNotThrow', function (t) { | ||
ava(function (a) { | ||
a.plan(6); | ||
for (var i = 0; i < 3; ++i) { | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
a.doesNotThrow(delay(10), 'foo'); | ||
} | ||
}).run().then(function (a) { | ||
t.ifError(a.assertError); | ||
t.is(a.planCount, 6); | ||
t.is(a.assertCount, 6); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('number of assertions matches t.plan when the test exits, but before all promises resolve another is added', function (t) { | ||
ava(function (a) { | ||
a.plan(2); | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
a.doesNotThrow(delay(10), 'foo'); | ||
setTimeout(function () { | ||
a.throws(Promise.reject(new Error('foo')), 'foo'); | ||
}, 5); | ||
}).run().catch(function (err) { | ||
t.is(err.operator, 'plan'); | ||
t.is(err.actual, 3); | ||
t.is(err.expected, 2); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
test('number of assertions doesn\'t match plan when the test exits, but before all promises resolve another is added', function (t) { | ||
ava(function (a) { | ||
a.plan(3); | ||
a.throws(delay.reject(10, new Error('foo')), 'foo'); | ||
a.doesNotThrow(delay(10), 'foo'); | ||
setTimeout(function () { | ||
a.throws(Promise.reject(new Error('foo')), 'foo'); | ||
}, 5); | ||
}).run().catch(function (err) { | ||
t.is(err.operator, 'plan'); | ||
t.is(err.actual, 2); | ||
t.is(err.expected, 3); | ||
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.
remove this empty line
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.
@sindresorhus: Fixed