Skip to content

Allow new assertions while waiting for pending ones #1331

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

Merged
merged 2 commits into from
Apr 3, 2017
Merged

Allow new assertions while waiting for pending ones #1331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e56fbc
Allow new assertions while waiting for pending ones
novemberborn Apr 1, 2017
d4bc502
fixup! Allow new assertions while waiting for pending ones
novemberborn Apr 2, 2017
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
24 changes: 13 additions & 11 deletions lib/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class Test {
this.endCallbackFinisher = null;
this.finishDueToAttributedError = null;
this.finishDueToInactivity = null;
this.finishing = false;
this.finished = false;
this.pendingAssertions = [];
this.pendingThrowsAssertion = null;
this.planCount = null;
Expand Down Expand Up @@ -152,15 +152,15 @@ class Test {
}

countPassedAssertion() {
if (this.finishing) {
if (this.finished) {
this.saveFirstError(new Error('Assertion passed, but test has already ended'));
}

this.assertCount++;
}

addPendingAssertion(promise) {
if (this.finishing) {
if (this.finished) {
this.saveFirstError(new Error('Assertion passed, but test has already ended'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is not from this PR, but a pending assertion technically hasn't passed yet. This message should be different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that when I first wrote it, but the nuance seemed superfluous. And since #1330 it's not even shown anymore.

}

Expand All @@ -169,7 +169,7 @@ class Test {
}

addFailedAssertion(error) {
if (this.finishing) {
if (this.finished) {
this.saveFirstError(new Error('Assertion failed, but test has already ended'));
}

Expand Down Expand Up @@ -365,21 +365,19 @@ class Test {
}

finish() {
this.finishing = true;

if (!this.assertError && this.pendingThrowsAssertion) {
return this.waitForPendingThrowsAssertion();
}

this.verifyPlan();
this.verifyAssertions();

if (this.assertError || this.pendingAssertions.length === 0) {
return this.completeFinish();
}

// Finish after potential errors from pending assertions have been consumed.
return Promise.all(this.pendingAssertions).then(() => this.completeFinish());
// Retry finishing after potential errors from pending assertions have been consumed.
const consumedErrors = Promise.all(this.pendingAssertions);
// Note that more assertions may be added in the meantime.
this.pendingAssertions = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test to validate the works it happens twice (pending resolve, but more pending have been added, and yet another sync will be added while you wait for those to resolve).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return consumedErrors.then(() => this.finish());
}

finishPromised() {
Expand All @@ -389,6 +387,10 @@ class Test {
}

completeFinish() {
this.verifyPlan();
this.verifyAssertions();

this.finished = true;
this.duration = globals.now() - this.startedAt;

let reason = this.assertError;
Expand Down
73 changes: 58 additions & 15 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,8 @@ test('number of assertions matches t.plan when the test exits, but before all pe
result = r;
}).run().then(passed => {
t.is(passed, false);
t.match(result.reason.message, /Assertion passed, but test has already ended/);
t.is(result.reason.name, 'Error');
t.is(result.reason.assertion, 'plan');
t.is(result.reason.operator, '===');
t.end();
});
});
Expand All @@ -604,35 +604,30 @@ test('number of assertions matches t.plan when the test exits, but before all pe
a.throws(delay.reject(10, new Error('foo')), 'foo');
a.notThrows(delay(10), 'foo');
setTimeout(() => {
a.fail();
a.pass();
}, 5);
}, null, r => {
result = r;
}).run().then(passed => {
t.is(passed, false);
t.match(result.reason.message, /Assertion failed, but test has already ended/);
t.is(result.reason.name, 'Error');
t.is(result.reason.assertion, 'plan');
t.is(result.reason.operator, '===');
t.end();
});
});

test('number of assertions doesn\'t match plan when the test exits, but before all promises resolve another is added', t => {
let result;
const passed = ava(a => {
ava(a => {
a.plan(3);
a.throws(delay.reject(10, new Error('foo')), 'foo');
a.notThrows(delay(10), 'foo');
setTimeout(() => {
a.throws(Promise.reject(new Error('foo')), 'foo');
}, 5);
}, null, r => {
result = r;
}).run();

t.is(passed, false);
t.is(result.reason.assertion, 'plan');
t.is(result.reason.operator, '===');
t.end();
}).run().then(passed => {
t.is(passed, true);
t.end();
});
});

test('assertions return promises', t => {
Expand All @@ -646,6 +641,54 @@ test('assertions return promises', t => {
});
});

// https://github.com/avajs/ava/issues/1330
test('no crash when adding assertions after the test has ended', t => {
t.plan(3);

ava(a => {
a.pass();
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I really don't think this should be ok. Many people do the mistake of using setTimeout inside a sync test and it silently passing. If users need to do this they should either use an async function or test.cb().

Copy link
Member Author

Choose a reason for hiding this comment

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

@sindresorhus It's not OK, that's what #1330 is about. This merely verifies that the internal handling doesn't cause a crash, since the error cannot currently be communicated.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok.

t.doesNotThrow(() => a.pass());
}, 5);
}).run();

ava(a => {
a.pass();
setTimeout(() => {
t.doesNotThrow(() => a.fail());
}, 5);
}).run();

ava(a => {
a.pass();
setTimeout(() => {
t.doesNotThrow(() => a.notThrows(Promise.resolve()));
}, 5);
}).run();
});

test('can add more pending assertions while waiting for earlier assertions to complete', t => {
return ava(a => {
a.plan(4);

a.notThrows(new Promise(resolve => {
setImmediate(() => {
resolve();

a.pass();
a.throws(new Promise(resolve => {
setImmediate(() => {
a.pass();
resolve();
});
}));
});
}));
}).run().then(passed => {
t.is(passed, false);
});
});

test('contextRef', t => {
new Test({
contextRef: {context: {foo: 'bar'}},
Expand Down