Skip to content

Stop hiding source of syntax errors #325

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

Closed
wants to merge 9 commits into from
Closed

Stop hiding source of syntax errors #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c3f17c3
Stop hiding source of syntax errors
Dec 12, 2015
ca72cf4
nonexisting -> nonexistent
Dec 12, 2015
fa400b0
Remove redundant nonexistent file test
Dec 12, 2015
4fcca81
Update fork test name
Dec 12, 2015
67574d9
Use tap.match(), use \d in regex instead of number
Dec 12, 2015
db512f1
Use t.match()
Dec 12, 2015
5d885e0
Use t.match()
Dec 12, 2015
2d0e355
Merge remote-tracking branch 'upstream/master'
Dec 12, 2015
24f4ca7
Fix test style inconsistencies
Dec 12, 2015
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
9 changes: 5 additions & 4 deletions lib/babel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var sourceMapCache = Object.create(null);

var sourceMapSupport = require('source-map-support');
sourceMapSupport.install({
handleUncaughtExceptions: false,
retrieveSourceMap: function (source) {
if (sourceMapCache[source]) {
return {
Expand Down Expand Up @@ -62,10 +63,6 @@ var options = {
// check if test files required ava and show error, when they didn't
exports.avaRequired = false;

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

// try to load an input source map for the test file, in case the file was
// already compiled once by the user
var inputSourceMap = sourceMapSupport.retrieveSourceMap(testPath);
Expand All @@ -82,6 +79,10 @@ requireFromString(transpiled.code, testPath, {
appendPaths: module.paths
});

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

// if ava was not required, show an error
if (!exports.avaRequired) {
send('no-tests', {avaRequired: false});
Expand Down
28 changes: 12 additions & 16 deletions test/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,22 +107,6 @@ test('change process.cwd() to a test\'s directory', function (t) {
});
});

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

var api = new Api([path.join(__dirname, 'fixture/babel-hook.js')]);

api.on('error', function (data) {
t.is(data.name, 'SyntaxError');
t.true(/Unexpected token/.test(data.message));
});

api.run()
.catch(function (err) {
t.ok(err);
});
});

test('unhandled promises will throw an error', function (t) {
t.plan(3);

Expand Down Expand Up @@ -249,6 +233,18 @@ test('test file that immediately exits with 0 exit code ', function (t) {
});
});

test('testing nonexistent files rejects', function (t) {
t.plan(2);

var api = new Api([path.join(__dirname, 'fixture/broken.js')]);

api.run()
.catch(function (err) {
t.ok(err);
t.match(err.message, /Couldn't find any files to test/);
});
});

test('test file in node_modules is ignored', function (t) {
t.plan(2);

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

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

execCli('fixture/babel-hook.js', function (err, stdout, stderr) {
t.ok(err);
t.is(err.code, 1);
t.match(stderr, /Unexpected token/);
t.end();
});
});

test('throwing a anonymous function will report the function to the console', function (t) {
execCli('fixture/throw-anonymous-function.js', function (err, stdout, stderr) {
t.ok(err);
Expand Down
8 changes: 3 additions & 5 deletions test/fork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,9 @@ test('rejects on error and streams output', function (t) {

fork(fixture('broken.js'))
.run()
.on('uncaughtException', function (data) {
t.true(/no such file or directory/.test(data.exception.message));
})
.catch(function () {
t.pass();
.catch(function (err) {
t.ok(err);
Copy link
Contributor

@jamestalmage jamestalmage Dec 12, 2015

Choose a reason for hiding this comment

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

Are you talking about this test here? If this test passes, that seems fine to me. We are getting a failure. Users of the CLI or API will get "can't find any files to test", like they have. Anybody using lib/fork from outside AVA is already asking for trouble.

We need to investigate why it fails on Node 0.10.

Copy link
Contributor Author

@sotojuan sotojuan Dec 12, 2015

Choose a reason for hiding this comment

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

We need to investigate why it fails on Node 0.10

It seems to fail in the regex test for the error message, so maybe the error message changed from 0.10 to others. I can give it a try with nvm.

If this test passes, that seems fine to me.

Yep it works and passes but I was just curious about the fact that a stack is printed to the screen during that error. Maybe it's not a big deal.

Copy link
Contributor

@jamestalmage jamestalmage Dec 12, 2015

Choose a reason for hiding this comment

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

That is because we have to let Node print it to stderr natively. We could clean up the output of our own tests by allowing fork to take stdio options like child_process.fork does (and pipe them to something else besides stderr for that particular test). That's a future PR, IMO.

t.match(err.message, /exited with a non-zero exit code: \d/);
t.end();
});
});
Expand Down