Skip to content

Commit

Permalink
cli: Don't throw strings when there are errors
Browse files Browse the repository at this point in the history 
It makes for confusing output that tells the user something is wrong
with the program, directing them to dig into istanbul's guts to 'fix the
problem', because the file and line number are printed there.

Instead, if we get a string, just output it to stderr, and exit with
status code 1.

If an actual (non-string) error is raised, still throw it, since that
does genuinely indicate that something is probably wrong.

Fix #518
  • Loading branch information
@isaacs
isaacs committed Jan 10, 2016
1 parent 8a678fa commit bd518ae
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 2 deletions.
7 changes: 6 additions & 1 deletion lib/cli.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ function exit(ex, code) {
stream.write('', done);
}, function () {
if (ex) {
throw ex; // turn it into an uncaught exception
if (typeof ex === 'string') {
console.error(ex);
exitProcess(1);
} else {
throw ex; // turn it into an uncaught exception
}
} else {
exitProcess(code);
}
Expand Down
16 changes: 15 additions & 1 deletion test/cli/test-check-coverage-command.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--statements', '72' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for statements .* global/));
test.done();
});
Expand All @@ -51,6 +52,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--branches', '72' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for branches .* global/));
test.done();
});
Expand All @@ -59,6 +61,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--functions', '72' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for functions .* global/));
test.done();
});
Expand All @@ -67,14 +70,16 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--lines', '72' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for lines .* global/));
test.done();
});
},
},
"should fail with multiple reasons when multiple thresholds violated": function (test) {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--statements=72', '--functions=50', '--branches=72', '--lines=72' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for statements .* global/));
test.ok(results.grepError(/Coverage for branches .* global/));
Expand All @@ -87,6 +92,7 @@ module.exports = {
// YML equivalent to: '--statements=72', '--functions=50', '--branches=72', '--lines=72'
run([ '--config', 'config-check-global.istanbul.yml' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for statements .* global/));
test.ok(results.grepError(/Coverage for branches .* global/));
Expand All @@ -99,6 +105,7 @@ module.exports = {
// YML equivalent to: '--statements=72', '--functions=50', '--branches=72', '--lines=72'
run([ '--statements=10', '--config', 'config-check-global.istanbul.yml' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Coverage for lines .* global/));
test.ok(!results.grepError(/Coverage for statements .* global/));
test.ok(results.grepError(/Coverage for branches .* global/));
Expand All @@ -110,6 +117,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--statements=-3', '--functions=-10', '--branches=-1', '--lines=-3' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/Uncovered count for lines .* global/));
test.ok(results.grepError(/Uncovered count for statements .* global/));
test.ok(results.grepError(/Uncovered count for branches .* global/));
Expand All @@ -129,6 +137,7 @@ module.exports = {
test.ok(!existsSync(path.resolve(OUTPUT_DIR, 'no-matching-coverage.json')));
run([ 'no-matching-coverage.json' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(results.grepError(/No coverage files found./));
test.done();
});
Expand All @@ -141,6 +150,7 @@ module.exports = {
// vendor/dummy_vendor_lib.js (statements 66.67% vs. 72%)
// vendor/dummy_vendor_lib.js (lines 66.67% vs. 72%)
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(!results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for lines .* per-file/));
test.ok(results.grepError(/Coverage for statements .* per-file/));
Expand All @@ -156,6 +166,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'relative.json')));
run([ '--config', 'config-check-each.istanbul.yml', 'coverage/relative.json' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(!results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for lines .* per-file/));
test.ok(results.grepError(/Coverage for statements .* per-file/));
Expand All @@ -171,6 +182,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'relative-dot-slash.json')));
run([ '--config', 'config-check-each.istanbul.yml', 'coverage/relative-dot-slash.json' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(!results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for lines .* per-file/));
test.ok(results.grepError(/Coverage for statements .* per-file/));
Expand All @@ -186,6 +198,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--branches=100', '--functions=100', '--config', 'config-check-each.istanbul.yml' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(!results.grepError(/Coverage for lines .* global/));
test.ok(!results.grepError(/Coverage for statements .* global/));
test.ok(results.grepError(/Coverage for branches .* global/));
Expand All @@ -201,6 +214,7 @@ module.exports = {
test.ok(existsSync(path.resolve(OUTPUT_DIR, 'coverage.json')));
run([ '--config', 'config-check-mixed.istanbul.yml' ], function (results) {
test.ok(!results.succeeded());
test.ok(!results.grepError(/lib[\\\/]cli.js:/));
test.ok(!results.grepError(/Coverage for lines .* global/));
test.ok(results.grepError(/Coverage for statements .* global/));
test.ok(!results.grepError(/Coverage for branches .* global/));
Expand Down

0 comments on commit bd518ae

Please sign in to comment.