Skip to content

Commit

Permalink
Integrate with improved throws helper
Browse files Browse the repository at this point in the history
Show instructions on how to use `t.throws()` with the test results,
without writing them to stderr. Detect the improper usage even if user
code swallows the error, meaning that tests will definitely fail.

Assume that errors thrown, or emitted from an observable, or if the
returned promise was rejected, that that error is due to the improper
usage of `t.throws()`.

Assume that if a test has a pending throws assertion, and an error leaks
as an uncaught exception or an unhandled rejection, the error was
thrown due to the pending throws assertion. Attribute it to the test.
  • Loading branch information
novemberborn committed Mar 26, 2017
1 parent 5e7ea9a commit d924045
Show file tree
Hide file tree
Showing 19 changed files with 314 additions and 71 deletions.
21 changes: 21 additions & 0 deletions lib/reporters/improper-usage-messages.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';
const chalk = require('chalk');

exports.forError = error => {
if (!error.improperUsage) {
return null;
}

const assertion = error.assertion;
if (assertion !== 'throws' || !assertion === 'notThrows') {
return null;
}

return `Try wrapping the first argument to \`t.${assertion}()\` in a function:
${chalk.cyan(`t.${assertion}(() => { `)}${chalk.grey('/* your code here */')}${chalk.cyan(' })')}
Visit the following URL for more details:
${chalk.blue.underline('https://github.com/avajs/ava#throwsfunctionpromise-error-message')}`;
};
6 changes: 6 additions & 0 deletions lib/reporters/mini.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const formatAssertError = require('../format-assert-error');
const extractStack = require('../extract-stack');
const codeExcerpt = require('../code-excerpt');
const colors = require('../colors');
const improperUsageMessages = require('./improper-usage-messages');

// TODO(@jamestalamge): This should be fixed in log-update and ansi-escapes once we are confident it's a good solution.
const CSI = '\u001B[';
Expand Down Expand Up @@ -200,6 +201,11 @@ class MiniReporter {
if (formatted) {
status += '\n' + indentString(formatted, 2);
}

const message = improperUsageMessages.forError(test.error);
if (message) {
status += '\n' + indentString(message, 2) + '\n';
}
}

if (test.error.stack) {
Expand Down
6 changes: 6 additions & 0 deletions lib/reporters/verbose.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const formatAssertError = require('../format-assert-error');
const extractStack = require('../extract-stack');
const codeExcerpt = require('../code-excerpt');
const colors = require('../colors');
const improperUsageMessages = require('./improper-usage-messages');

class VerboseReporter {
constructor(options) {
Expand Down Expand Up @@ -120,6 +121,11 @@ class VerboseReporter {
if (formatted) {
output += '\n' + indentString(formatted, 2);
}

const message = improperUsageMessages.forError(test.error);
if (message) {
output += '\n' + indentString(message, 2) + '\n';
}
}

if (test.error.stack) {
Expand Down
3 changes: 3 additions & 0 deletions lib/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ class Runner extends EventEmitter {

return Promise.resolve(this.tests.build().run()).then(this._buildStats);
}
attributeLeakedError(err) {
return this.tests.attributeLeakedError(err);
}
}

optionChain(chainableMethods, function (opts, args) {
Expand Down
23 changes: 19 additions & 4 deletions lib/test-collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class TestCollection extends EventEmitter {
afterEachAlways: []
};

this.pendingTestInstances = new Set();

this._emitTestResult = this._emitTestResult.bind(this);
}
add(test) {
Expand Down Expand Up @@ -100,8 +102,9 @@ class TestCollection extends EventEmitter {
}
};
}
_emitTestResult(test) {
this.emit('test', test);
_emitTestResult(result) {
this.pendingTestInstances.delete(result.result);
this.emit('test', result);
}
_buildHooks(hooks, testTitle, context) {
return hooks.map(hook => {
Expand All @@ -125,28 +128,32 @@ class TestCollection extends EventEmitter {
contextRef = null;
}

return new Test({
const test = new Test({
contextRef,
failWithoutAssertions: false,
fn: hook.fn,
metadata: hook.metadata,
onResult: this._emitTestResult,
title
});
this.pendingTestInstances.add(test);
return test;
}
_buildTest(test, contextRef) {
if (!contextRef) {
contextRef = null;
}

return new Test({
test = new Test({
contextRef,
failWithoutAssertions: this.failWithoutAssertions,
fn: test.fn,
metadata: test.metadata,
onResult: this._emitTestResult,
title: test.title
});
this.pendingTestInstances.add(test);
return test;
}
_buildTestWithHooks(test) {
if (test.metadata.skipped || test.metadata.todo) {
Expand Down Expand Up @@ -183,6 +190,14 @@ class TestCollection extends EventEmitter {
}
return finalTests;
}
attributeLeakedError(err) {
for (const test of this.pendingTestInstances) {
if (test.attributeLeakedError(err)) {
return true;
}
}
return false;
}
}

module.exports = TestCollection;
23 changes: 19 additions & 4 deletions lib/test-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const isObj = require('is-obj');
const adapter = require('./process-adapter');
const globals = require('./globals');
const serializeError = require('./serialize-error');
const throwsHelper = require('./throws-helper');

const opts = adapter.opts;
const testPath = opts.file;
Expand Down Expand Up @@ -54,10 +53,25 @@ if (!runner) {
adapter.send('no-tests', {avaRequired: false});
}

process.on('unhandledRejection', throwsHelper);
function attributeLeakedError(err) {
if (!runner) {
return false;
}

return runner.attributeLeakedError(err);
}

const attributedRejections = new Set();
process.on('unhandledRejection', (reason, promise) => {
if (attributeLeakedError(reason)) {
attributedRejections.add(promise);
}
});

process.on('uncaughtException', exception => {
throwsHelper(exception);
if (attributeLeakedError(exception)) {
return;
}

let serialized;
try {
Expand Down Expand Up @@ -88,7 +102,8 @@ process.on('ava-teardown', () => {
}
tearingDown = true;

let rejections = currentlyUnhandled();
let rejections = currentlyUnhandled()
.filter(rejection => !attributedRejections.has(rejection.promise));

if (rejections.length > 0) {
rejections = rejections.map(rejection => {
Expand Down
105 changes: 90 additions & 15 deletions lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const plur = require('plur');
const assert = require('./assert');
const formatAssertError = require('./format-assert-error');
const globals = require('./globals');
const throwsHelper = require('./throws-helper');

class SkipApi {
constructor(test) {
Expand Down Expand Up @@ -60,6 +59,13 @@ class ExecutionContext {

contextRef.context = context;
}

_throwsArgStart(assertion, file, line) {
this._test.trackThrows({assertion, file, line});
}
_throwsArgEnd() {
this._test.trackThrows(null);
}
}
Object.defineProperty(ExecutionContext.prototype, 'context', {enumerable: true});

Expand Down Expand Up @@ -101,9 +107,11 @@ class Test {
this.calledEnd = false;
this.duration = null;
this.endCallbackFinisher = null;
this.finishDueToAttributedError = null;
this.finishDueToInactivity = null;
this.finishing = false;
this.pendingAssertions = [];
this.pendingThrowsAssertion = null;
this.planCount = null;
this.startedAt = 0;
}
Expand Down Expand Up @@ -204,6 +212,60 @@ class Test {
}
}

trackThrows(pending) {
this.pendingThrowsAssertion = pending;
}

detectImproperThrows(err) {
if (!this.pendingThrowsAssertion) {
return false;
}

const pending = this.pendingThrowsAssertion;
this.pendingThrowsAssertion = null;

const values = [];
if (err) {
values.push(formatAssertError.formatWithLabel(`The following error was thrown, possibly before \`t.${pending.assertion}()\` could be called:`, err));
}

this.saveFirstError(new assert.AssertionError({
assertion: pending.assertion,
fixedSource: {file: pending.file, line: pending.line},
improperUsage: true,
message: `Improper usage of \`t.${pending.assertion}()\` detected`,
stack: err instanceof Error && err.stack,
values
}));
return true;
}

waitForPendingThrowsAssertion() {
return new Promise(resolve => {
this.finishDueToAttributedError = () => {
resolve(this.finishPromised());
};

this.finishDueToInactivity = () => {
this.detectImproperThrows();
resolve(this.finishPromised());
};

// Wait up to a second to see if an error can be attributed to the
// pending assertion.
globals.setTimeout(() => this.finishDueToInactivity(), 1000).unref();
});
}

attributeLeakedError(err) {
if (!this.detectImproperThrows(err)) {
return false;
}

this.finishDueToAttributedError();
return true;
}

callFn() {
try {
return {
Expand All @@ -223,13 +285,13 @@ class Test {

const result = this.callFn();
if (!result.ok) {
throwsHelper(result.error);

this.saveFirstError(new assert.AssertionError({
message: 'Error thrown in test',
stack: result.error instanceof Error && result.error.stack,
values: [formatAssertError.formatWithLabel('Error:', result.error)]
}));
if (!this.detectImproperThrows(result.error)) {
this.saveFirstError(new assert.AssertionError({
message: 'Error thrown in test',
stack: result.error instanceof Error && result.error.stack,
values: [formatAssertError.formatWithLabel('Error:', result.error)]
}));
}
return this.finish();
}

Expand Down Expand Up @@ -260,6 +322,10 @@ class Test {
resolve(this.finishPromised());
};

this.finishDueToAttributedError = () => {
resolve(this.finishPromised());
};

this.finishDueToInactivity = () => {
this.saveFirstError(new Error('`t.end()` was never called'));
resolve(this.finishPromised());
Expand All @@ -269,6 +335,10 @@ class Test {

if (promise) {
return new Promise(resolve => {
this.finishDueToAttributedError = () => {
resolve(this.finishPromised());
};

this.finishDueToInactivity = () => {
const err = returnedObservable ?
new Error('Observable returned by test never completed') :
Expand All @@ -279,13 +349,13 @@ class Test {

promise
.catch(err => {
throwsHelper(err);

this.saveFirstError(new assert.AssertionError({
message: 'Rejected promise returned by test',
stack: err instanceof Error && err.stack,
values: [formatAssertError.formatWithLabel('Rejection reason:', err)]
}));
if (!this.detectImproperThrows(err)) {
this.saveFirstError(new assert.AssertionError({
message: 'Rejected promise returned by test',
stack: err instanceof Error && err.stack,
values: [formatAssertError.formatWithLabel('Rejection reason:', err)]
}));
}
})
.then(() => resolve(this.finishPromised()));
});
Expand All @@ -296,6 +366,11 @@ class Test {

finish() {
this.finishing = true;

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

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

Expand Down
37 changes: 0 additions & 37 deletions lib/throws-helper.js

This file was deleted.

Loading

0 comments on commit d924045

Please sign in to comment.