Skip to content
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

Closed
wants to merge 10 commits into from
97 changes: 53 additions & 44 deletions lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand All @@ -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) {
Expand Down Expand Up @@ -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;

Copy link
Member

Choose a reason for hiding this comment

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

remove this empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sindresorhus: Fixed

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 () {
Expand Down Expand Up @@ -230,33 +249,23 @@ Test.prototype._publicApi = function () {
api.plan = this.plan.bind(this);

function skipFn() {
self._assert();
self._assert(Promise.resolve());
Copy link
Contributor

Choose a reason for hiding this comment

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

self._assert() should work fine here. Promise.all converts array members to promise using Promise.resolve anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 this.assertions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I wrote this before I saw how you optimized the onAssertion callback. You can disregard.

}

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({
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
},
"devDependencies": {
"coveralls": "^2.11.4",
"delay": "^1.3.0",
"signal-exit": "^2.1.2",
"sinon": "^1.17.2",
"source-map-fixtures": "^0.4.0",
Expand Down
118 changes: 118 additions & 0 deletions test/test.js
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');
Copy link
Contributor

Choose a reason for hiding this comment

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

var Promise = require('bluebird');

Promise is not natively available in Node 0.10


function ava() {
Expand Down Expand Up @@ -351,3 +353,119 @@ test('skipped assertions count towards the plan', function (t) {
t.end();
});
});

test('throws and doesNotThrow work with promises', function (t) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
});
});