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

detect improper use of t.throws #742

Merged
merged 8 commits into from
Apr 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/recipes/babelrc.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,6 @@ AVA *always* adds a few custom Babel plugins when transpiling your plugins. They

* Enable `power-assert` support.
* Rewrite require paths internal AVA dependencies like `babel-runtime` (important if you are still using `npm@2`).
* [`ava-throws-helper`](https://github.com/jamestalmage/babel-plugin-ava-throws-helper) helps AVA [detect and report](https://github.com/sindresorhus/ava/pull/742) improper use of the `t.throws` assertion.
* Generate test metadata to determine which files should be run first (*future*).
* Static analysis of dependencies for precompilation (*future*).
2 changes: 2 additions & 0 deletions lib/caching-precompiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,13 @@ CachingPrecompiler.prototype._init = function () {
];

var transformRuntime = require('babel-plugin-transform-runtime');
var throwsHelper = require('babel-plugin-ava-throws-helper');
var rewriteBabelPaths = this._createRewritePlugin();
var powerAssert = this._createEspowerPlugin();

this.defaultPlugins = [
powerAssert,
throwsHelper,
rewriteBabelPaths,
transformRuntime
];
Expand Down
1 change: 1 addition & 0 deletions lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ if (env.NODE_PATH) {
module.exports = function (file, opts) {
opts = objectAssign({
file: file,
baseDir: process.cwd(),
tty: process.stdout.isTTY ? {
columns: process.stdout.columns,
rows: process.stdout.rows
Expand Down
4 changes: 4 additions & 0 deletions lib/test-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ sourceMapSupport.install({
var loudRejection = require('loud-rejection/api')(process); // eslint-disable-line
var serializeError = require('./serialize-error');
var send = require('./send');
var throwsHelper = require('./throws-helper');
var installPrecompiler = require('require-precompiled'); // eslint-disable-line
var cacheDir = opts.cacheDir;

Expand Down Expand Up @@ -92,7 +93,10 @@ Object.keys(require.extensions).forEach(function (ext) {

require(testPath);

process.on('unhandledRejection', throwsHelper);

process.on('uncaughtException', function (exception) {
throwsHelper(exception);
send('uncaughtException', {exception: serializeError(exception)});
});

Expand Down
2 changes: 2 additions & 0 deletions lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ var plur = require('plur');
var assert = require('./assert');
var enhanceAssert = require('./enhance-assert');
var globals = require('./globals');
var throwsHelper = require('./throws-helper');

function Test(title, fn, contextRef, report) {
if (!(this instanceof Test)) {
Expand Down Expand Up @@ -68,6 +69,7 @@ Test.prototype._assert = function (promise) {
};

Test.prototype._setAssertError = function (err) {
throwsHelper(err);
if (this.assertError !== undefined) {
return;
}
Expand Down
35 changes: 35 additions & 0 deletions lib/throws-helper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';
var fs = require('fs');
var path = require('path');
var chalk = require('chalk');
var globals = require('./globals');

module.exports = function throwsHelper(error) {
if (!error || !error._avaThrowsHelperData) {
return;
}
var data = error._avaThrowsHelperData;
var codeFrame = require('babel-code-frame');
var frame = '';
try {
var rawLines = fs.readFileSync(data.filename, 'utf8');
frame = codeFrame(rawLines, data.line, data.column, {highlightCode: true});
} catch (e) {
console.warn(e);
console.warn(e);
}
console.error(
[
'Improper usage of t.throws detected at ' + chalk.bold.yellow('%s (%d:%d)') + ':',
frame,
'The first argument to t.throws should be wrapped in a function:',
chalk.cyan(' t.throws(function() {\n %s\n })'),
'Visit the following URL for more details:',
' ' + chalk.blue.underline('https://github.com/sindresorhus/ava#throwsfunctionpromise-error-message')
].join('\n\n'),
path.relative(globals.options.baseDir, data.filename),
data.line,
data.column,
data.source
);
};
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@
"array-uniq": "^1.0.2",
"arrify": "^1.0.0",
"ava-init": "^0.1.0",
"babel-code-frame": "^6.7.5",
"babel-core": "^6.3.21",
"babel-plugin-ava-throws-helper": "0.0.4",
"babel-plugin-detective": "^1.0.2",
"babel-plugin-espower": "^2.1.0",
"babel-plugin-transform-runtime": "^6.3.13",
Expand Down Expand Up @@ -138,6 +140,7 @@
"update-notifier": "^0.6.0"
},
"devDependencies": {
"babel-code-frame": "^6.7.5",
"cli-table2": "^0.2.0",
"coveralls": "^2.11.4",
"delay": "^1.3.0",
Expand Down
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ You can also use the special `"inherit"` keyword. This makes AVA defer to the Ba

See AVA's [`.babelrc` recipe](docs/recipes/babelrc.md) for further examples and a more detailed explanation of configuration options.

Note that AVA will *always* apply the [`espower`](https://github.com/power-assert-js/babel-plugin-espower) and [`transform-runtime`](https://babeljs.io/docs/plugins/transform-runtime/) plugins.
Note that AVA will *always* apply [a few internal plugins](docs/recipes/babelrc.md#notes) regardless of configuration, but they should not impact the behavior of your code.

### TypeScript support

Expand Down
3 changes: 2 additions & 1 deletion test/caching-precompiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var uniqueTempDir = require('unique-temp-dir');
var sinon = require('sinon');
var babel = require('babel-core');
var transformRuntime = require('babel-plugin-transform-runtime');
var throwsHelper = require('babel-plugin-ava-throws-helper');
var fromMapFileSource = require('convert-source-map').fromMapFileSource;

var CachingPrecompiler = require('../lib/caching-precompiler');
Expand Down Expand Up @@ -145,7 +146,7 @@ test('uses babelConfig for babel options when babelConfig is an object', functio
t.true('inputSourceMap' in options);
t.false(options.babelrc);
t.same(options.presets, ['stage-2', 'es2015']);
t.same(options.plugins, [customPlugin, powerAssert, rewrite, transformRuntime]);
t.same(options.plugins, [customPlugin, powerAssert, throwsHelper, rewrite, transformRuntime]);
Copy link
Member

Choose a reason for hiding this comment

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

Without checking the power-assert transformations, are there cases the Babel plugin can no longer catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be. The babel-throws-helper wrapper is totally transparent to the code it wraps.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but power-assert might rewrite the t.throws() argument expression such that it is computed outside of t.throws(). Again, I don't know if it does, but if it did then you wouldn't be able to catch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh - Understood. Still not a problem though. power-assert wraps inline, just like ava-throws-helper.

t.true(_rec._expr(_rec._capt(_rec._capt(a, 'arguments/0/left') === 'bar', 'arguments/0'), {
    content: 't.true(a === \'bar\')',
    filepath: 'test/fixture/power-assert.js',
    line: 6
}));

Also, we don't enhance t.throws with power-assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't enhance t.throws with power-assert.

Right. t.throws is listed as NON_ENHANCED_PATTERNS here. power-assert doesn't touch its arguments.

t.end();
});

Expand Down
24 changes: 24 additions & 0 deletions test/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,30 @@ test('throwing a named function will report the to the console', function (t) {
});
});

test('improper use of t.throws will be reported to the console', function (t) {
execCli('fixture/improper-t-throws.js', function (err, stdout, stderr) {
t.ok(err);
t.match(stderr, /Improper usage of t\.throws detected at .*improper-t-throws.js \(4:10\)/);
t.end();
});
});

test('improper use of t.throws from within a Promise will be reported to the console', function (t) {
execCli('fixture/improper-t-throws-promise.js', function (err, stdout, stderr) {
t.ok(err);
t.match(stderr, /Improper usage of t\.throws detected at .*improper-t-throws-promise.js \(5:11\)/);
t.end();
});
});

test('improper use of t.throws from within an async callback will be reported to the console', function (t) {
execCli('fixture/improper-t-throws-async-callback.js', function (err, stdout, stderr) {
t.ok(err);
t.match(stderr, /Improper usage of t\.throws detected at .*improper-t-throws-async-callback.js \(5:11\)/);
t.end();
});
});

test('babel require hook only applies to the test file', function (t) {
t.plan(3);

Expand Down
11 changes: 11 additions & 0 deletions test/fixture/improper-t-throws-async-callback.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../';

test.cb(t => {
setTimeout(() => {
t.throws(throwSync());
});
});

function throwSync() {
throw new Error('should be detected');
}
11 changes: 11 additions & 0 deletions test/fixture/improper-t-throws-promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import test from '../../';

test(t => {
return Promise.resolve().then(() => {
t.throws(throwSync());
});
});

function throwSync() {
throw new Error('should be detected');
}
13 changes: 13 additions & 0 deletions test/fixture/improper-t-throws-unhandled-rejection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import test from '../../';

test.cb(t => {
Promise.resolve().then(() => {
t.throws(throwSync());
});

setTimeout(t.end, 20);
});

function throwSync() {
throw new Error('should be detected');
}
9 changes: 9 additions & 0 deletions test/fixture/improper-t-throws.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import test from '../../';

test(t => {
t.throws(throwSync());
});

function throwSync() {
throw new Error('should be detected');
}