Skip to content

Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests. #206

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

Merged
merged 5 commits into from
Nov 16, 2015
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
7 changes: 4 additions & 3 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ environment:
install:
- ps: Install-Product node $env:nodejs_version
- set CI=true
- npm -g install npm@latest
- set AVA_APPVEYOR=true
- npm -g install npm@latest || (timeout 30 && npm -g install npm@latest)
- set PATH=%APPDATA%\npm;%PATH%
- npm install
- npm install || (timeout 30 && npm install)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about just: timeout 30 && npm install?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this tries to reinstall after 30 seconds if the first npm install fails (which it does about every 10 times or so on AppVeyor).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this timeout shorter?

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 chose 30 seconds pretty arbitrarily, so I"m not opposed to shortening it and seeing what happens.
Why do you want it shorter?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it matters to be honest. AppVeyor is sooooo slow regardless. But we can try with 10 seconds to see if it still works.

Copy link
Member

Choose a reason for hiding this comment

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

@jamestalmage Probably because it's annoying to always have to wait for AppVeyor before merging a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want it shorter?

So that tests are executed sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably because it's annoying to always have to wait for AppVeyor before merging a PR.

Yes. That.

I got a lot of TV watched this weekend. Push a commit, watch 20 minutes... Still broken... Push a commit, watch 20 minutes...

Copy link
Member

Choose a reason for hiding this comment

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

Haha :p

Sounds like me this weekend. I was bumping dependencies on a gadzillion modules on a super slow connection. $ david update, wait 10 minutes, commit, push, npm publish, wait 5 minutes, then next module...

matrix:
fast_finish: true
build: off
Expand All @@ -19,4 +20,4 @@ clone_depth: 1
test_script:
- node --version
- npm --version
- npm run test-win
- npm run test-win || (timeout 30 && npm run test-win)
34 changes: 29 additions & 5 deletions cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@ var cli = meow({

var testCount = 0;
var fileCount = 0;
var unhandledRejectionCount = 0;
var uncaughtExceptionCount = 0;
var errors = [];

function error(err) {
console.error(err.stack);
process.exit(1);
flushIoAndExit(1);
}

function prefixTitle(file) {
Expand Down Expand Up @@ -116,11 +118,24 @@ function run(file) {
return fork(args)
.on('stats', stats)
.on('test', test)
.on('unhandledRejections', rejections)
.on('uncaughtException', uncaughtException)
.on('data', function (data) {
process.stdout.write(data);
});
}

function rejections(data) {
var unhandled = data.unhandledRejections;
log.unhandledRejections(data.file, unhandled);
unhandledRejectionCount += unhandled.length;
}

function uncaughtException(data) {
uncaughtExceptionCount++;
log.uncaughtException(data.file, data.uncaughtException);
}

function sum(arr, key) {
var result = 0;

Expand All @@ -145,21 +160,30 @@ function exit(results) {
var failed = sum(stats, 'failCount');

log.write();
log.report(passed, failed);
log.report(passed, failed, unhandledRejectionCount, uncaughtExceptionCount);
log.write();

if (failed > 0) {
log.errors(flatten(tests));
}

process.stdout.write('');

flushIoAndExit(
failed > 0 || unhandledRejectionCount > 0 || uncaughtExceptionCount > 0 ? 1 : 0
);
}

function flushIoAndExit(code) {
// TODO: figure out why this needs to be here to
// correctly flush the output when multiple test files
process.stdout.write('');
process.stderr.write('');
Copy link
Member

Choose a reason for hiding this comment

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

Wut, another one? :p

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there's 3 of process.stdout.write('') now for no explained reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - I meant to delete that other one.

There should be one each stderr and stdout. This belongs in the "it seamed to help" pile. Definitely room to experiment with removing it down the road.


// timeout required to correctly flush stderr on Node 0.10 Windows
// timeout required to correctly flush io on Node 0.10 Windows
setTimeout(function () {
process.exit(failed > 0 ? 1 : 0);
}, 0);
process.exit(code);
}, process.env.AVA_APPVEYOR ? 500 : 0);
}

function init(files) {
Expand Down
2 changes: 1 addition & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ var setImmediate = require('set-immediate-shim');
var hasFlag = require('has-flag');
var chalk = require('chalk');
var relative = require('path').relative;
var serializeError = require('destroy-circular');
var serializeError = require('./lib/serialize-value');
var Runner = require('./lib/runner');
var log = require('./lib/logger');
var runner = new Runner();
Expand Down
38 changes: 33 additions & 5 deletions lib/babel.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict';
var loudRejection = require('loud-rejection/api')(process);
var resolveFrom = require('resolve-from');
var createEspowerPlugin = require('babel-plugin-espower/create');
var requireFromString = require('require-from-string');
var serializeValue = require('./serialize-value');

var hasGenerators = parseInt(process.version.slice(1), 10) > 0;
var testPath = process.argv[2];
Expand Down Expand Up @@ -32,20 +34,46 @@ module.exports = {
}
};

function send(name, data) {
process.send({name: name, data: data});
}

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

var transpiled = babel.transformFileSync(testPath, options);
requireFromString(transpiled.code, testPath, {
appendPaths: module.paths
});

if (!avaRequired) {
console.error('No tests found in ' + testPath + ', make sure to import "ava" at the top of your test file');
setImmediate(function () {
process.exit(1);
});
throw new Error('No tests found in ' + testPath + ', make sure to import "ava" at the top of your test file');
}

process.on('message', function (message) {
if (message['ava-kill-command']) {
var command = message['ava-child-process-command'];
if (command) {
process.emit('ava-' + command, message.data);
}
});

process.on('ava-kill', function () {
setTimeout(function () {
process.exit(0);
}, process.env.AVA_APPVEYOR ? 100 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

AppVeyor doesn't set env variables like Travis to identify itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, in that case we don't need our own AVA_APPVEYOR.

Copy link
Member

Choose a reason for hiding this comment

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

@vdemedes I think the intention is to only do it when we run AVA's tests on AppVeyor, not when the user run their tests there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sindresorhus Hmm, don't those fixes also needed for user's tests?

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 got the intent right.
I originally had it as APPVEYOR, until a conversation with @BarryThePenguin

The only tests this affects is ones that parse the stdout / stderr from our CLI.
There is a chance by running the tests fast, that they miss the last line or two (a summary or the failed stack trace). It's highly unlikely they will be grepping our code CLI output to get a pass/fail on their own tests.

They will still always get the correct exit code - they might just be missing the last "1 test failed".
If users report it as a problem it is easy enough to correct down the road.

});

process.on('ava-cleanup', function () {
var unhandled = loudRejection.currentlyUnhandled();
if (unhandled.length) {
unhandled = unhandled.map(function (entry) {
return serializeValue(entry.reason);
});
send('unhandledRejections', {unhandledRejections: unhandled});
}

setTimeout(function () {
send('cleaned-up', {});
}, 100);
});
14 changes: 13 additions & 1 deletion lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ module.exports = function (args) {

var ps = childProcess.fork(babel, args, options);

function send(command, data) {
ps.send({'ava-child-process-command': command, 'data': data});
Copy link
Contributor

Choose a reason for hiding this comment

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

can just be:

ps.send({
  name: command,
  data: data
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only thought there was wether or not we wanted to monkeypatch process.on on the child process, so users never saw our communication.

I can imagine a scenario where you are writing some code that might be a child process (or might be top level), and so they would will listen to process.on('message'). By choosing a unique property name here we could monkeypatch on and filter out these messages. I reemit these on the child process as specific ava- prefixed events(process.on('ava-kill', ...), process.on('ava-cleanup'), etc) - that it is really unlikely users will listen to.

None of that is implemented. Just leaving the door open.

}

var promise = new Promise(function (resolve, reject) {
var testResults;

Expand All @@ -26,7 +30,15 @@ module.exports = function (args) {

// after all tests are finished and results received
// kill the forked process, so AVA can exit safely
ps.send({'ava-kill-command': true});
send('cleanup', true);
});

ps.on('cleaned-up', function () {
send('kill', true);
});

ps.on('uncaughtException', function () {
send('cleanup', true);
});

ps.on('error', reject);
Expand Down
33 changes: 32 additions & 1 deletion lib/logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,41 @@ x.errors = function (results) {
});
};

x.report = function (passed, failed) {
x.report = function (passed, failed, unhandled, uncaught) {
if (failed > 0) {
log.writelpad(chalk.red(failed, plur('test', failed), 'failed'));
} else {
log.writelpad(chalk.green(passed, plur('test', passed), 'passed'));
}
if (unhandled > 0) {
log.writelpad(chalk.red(unhandled, 'unhandled', plur('rejection', unhandled)));
}
if (uncaught > 0) {
log.writelpad(chalk.red(uncaught, 'uncaught', plur('exception', uncaught)));
}
};

x.unhandledRejections = function (file, rejections) {
if (!(rejections && rejections.length)) {
return;
}
rejections.forEach(function (rejection) {
log.write(chalk.red('Unhandled Rejection: ', file));
if (rejection.stack) {
log.writelpad(chalk.red(beautifyStack(rejection.stack)));
} else {
log.writelpad(chalk.red(JSON.stringify(rejection)));
}
log.write();
});
};

x.uncaughtException = function (file, error) {
log.write(chalk.red('Uncaught Exception: ', file));
if (error.stack) {
log.writelpad(chalk.red(beautifyStack(error.stack)));
} else {
log.writelpad(chalk.red(JSON.stringify(error)));
}
log.write();
};
16 changes: 16 additions & 0 deletions lib/serialize-value.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict';
var destroyCircular = require('destroy-circular');

// Make a value ready for JSON.stringify() / process.send()

module.exports = function serializeValue(value) {
if (typeof value === 'object') {
return destroyCircular(value);
}
if (typeof value === 'function') {
// JSON.stringify discards functions, leaving no context information once we serialize and send across.
// We replace thrown functions with a string to provide as much information to the user as possible.
return '[Function: ' + (value.name || 'anonymous') + ']';
}
return value;
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"globby": "^3.0.1",
"has-flag": "^1.0.0",
"is-generator-fn": "^1.0.0",
"loud-rejection": "^1.2.0",
"max-timeout": "^1.0.0",
"meow": "^3.3.0",
"plur": "^2.0.0",
Expand Down
9 changes: 9 additions & 0 deletions test/fixture/loud-rejection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const test = require('../../');

test('creates an unhandled rejection', t => {
Promise.reject(new Error(`You can't handle this!`));

setTimeout(function () {
t.end();
});
});
7 changes: 7 additions & 0 deletions test/fixture/throw-anonymous-function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const test = require('../../');

test('throw an uncaught exception', t => {
setImmediate(() => {
throw function () {};
});
});
9 changes: 9 additions & 0 deletions test/fixture/throw-named-function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const test = require('../../');

function fooFn() {}

test('throw an uncaught exception', t => {
setImmediate(() => {
throw fooFn
});
});
7 changes: 7 additions & 0 deletions test/fixture/uncaught-exception.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const test = require('../../');

test('throw an uncaught exception', t => {
setImmediate(() => {
throw new Error(`Can't catch me!`)
});
});
10 changes: 5 additions & 5 deletions test/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ test('resolves promise with tests info', function (t) {
});

test('rejects on error and streams output', function (t) {
var buffer = '';

t.plan(2);
fork(fixture('broken.js'))
.on('data', function (data) {
buffer += data;
.on('uncaughtException', function (data) {
var exception = data.uncaughtException;
t.ok(/no such file or directory/.test(exception.message));
})
.catch(function () {
t.ok(/no such file or directory/.test(buffer));
t.pass();
t.end();
});
});
Expand Down
44 changes: 40 additions & 4 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1058,14 +1058,50 @@ test('change process.cwd() to a test\'s directory', function (t) {

test('Babel require hook only applies to the test file', function (t) {
execCli('fixture/babel-hook.js', function (err, stdout, stderr) {
t.ok(/exited with a non-zero exit code/.test(stderr));
t.ok(/Unexpected token/.test(stdout));
t.ok(/Unexpected token/.test(stderr));
t.ok(err);
t.is(err.code, 1);
t.end();
});
});

test('Unhandled promises will be reported to console', function (t) {
execCli('fixture/loud-rejection.js', function (err, stdout, stderr) {
t.ok(err);
t.ok(/You can't handle this/.test(stderr));
t.ok(/1 unhandled rejection[^s]/.test(stderr));
t.end();
});
});

test('uncaught exception will be reported to console', function (t) {
execCli('fixture/uncaught-exception.js', function (err, stdout, stderr) {
t.ok(err);
t.ok(/Can't catch me!/.test(stderr));
// TODO: This should get printed, but we reject the promise (ending all tests) instead of just ending that one test and reporting.
Copy link
Member

Choose a reason for hiding this comment

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

For TODO's I prefer to include the username so we know who added it without having to git blame.

TODO(jamestalmage):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, I'm assuming you took care of it in the merge? Or do you want me to go handle it now?

Copy link
Member

Choose a reason for hiding this comment

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

I'll do it.

// t.ok(/1 uncaught exception[^s]/.test(stdout));
t.end();
});
});

test('throwing a named function will report the to the console', function (t) {
execCli('fixture/throw-named-function.js', function (err, stdout, stderr) {
t.ok(err);
t.ok(/\[Function: fooFn]/.test(stderr));
// t.ok(/1 uncaught exception[^s]/.test(stdout));
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);
t.ok(/\[Function: anonymous]/.test(stderr));
// t.ok(/1 uncaught exception[^s]/.test(stdout));
t.end();
});
});

test('absolute paths in CLI', function (t) {
t.plan(2);

Expand All @@ -1091,9 +1127,9 @@ test('titles of both passing and failing tests and AssertionErrors are displayed
test('empty test files creates a failure with a helpful warning', function (t) {
t.plan(2);

execCli('fixture/empty.js', function (err, stdout) {
execCli('fixture/empty.js', function (err, stdout, stderr) {
t.ok(err);
t.ok(/No tests found.*?import "ava"/.test(stdout));
t.ok(/No tests found.*?import "ava"/.test(stderr));
t.end();
});
});
Expand Down