test: increase usage of assert.ifError() #10543

cjihrig · 2016-12-30T15:56:25Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes

Affected core subsystem(s)

test

Do this in one commit instead of one commit per test.

gibfahn · 2016-12-30T16:06:52Z

test/parallel/test-repl-persistent-history.js

@@ -247,10 +247,7 @@ function runTest(assertCleaned) {
    useColors: false,
    terminal: true
  }, function(err, repl) {
-    if (err) {
-      console.error(`Failed test # ${numtests - tests.length}`);


Is the failed test number useful here?

I wrote this. It is useful. Please keep it.

It's a pity that assert.ifError() doesn't take an optional message argument, unlike every other assert method...

Restored that one.

Adding a comment indicating that the console.log should not be removed would be helpfuil

gibfahn · 2016-12-30T16:14:01Z

@Trott Is this something we could lint for (at least within ./test)? It looks like we currently check assert.deepEqual, assert.fail, and assert.throws, but not the others.

cjihrig · 2016-12-30T16:59:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5640/

Trott · 2016-12-31T05:24:43Z

@Trott Is this something we could lint for (at least within ./test)? It looks like we currently check assert.deepEqual, assert.fail, and assert.throws, but not the others.

You mean can we lint for if (err) throw err type constructs and suggest they get replaced with assert.ifError(err) instead?

Probably but it would be a bit more complicated than those other existing rules you list and probably more prone to error. /cc @not-an-aardvark for the real expert opinion on such things, though...

Trott · 2016-12-31T05:27:11Z

and probably more prone to error

Actually, the more I'm thinking about it, the more I'm thinking it's do-able. You wouldn't want to flag things like this anyway:

if (err) {
  // do a bunch of stuff here
  throw err;
}

The // do a bunch of stuff here precludes subbing in assert.ifError(err) so the fact that a rule might miss flagging that would be a feature rather than a bug.

not-an-aardvark · 2016-12-31T06:12:35Z

Yes, that seems doable. Would we want to report arbitrary expressions there too, or just identifiers/variables like err?

if (foo.bar.baz()) {
  throw foo.bar.baz();
}

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

gibfahn · 2017-01-02T18:51:07Z

@not-an-aardvark

Yes, that seems doable. Would we want to report arbitrary expressions there too, or just identifiers/variables like err?

If it's not much harder to do then yes, if the if condition and the throw expression are the same then assert.ifError seems like a better way (IMHO).

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. Refs: nodejs#10543

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. PR-URL: #10671 Ref: #10543 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com>

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. PR-URL: nodejs#10671 Ref: nodejs#10543 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com>

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. PR-URL: #10671 Ref: #10543 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com>

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. PR-URL: nodejs#10671 Ref: nodejs#10543 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com>

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. PR-URL: nodejs#10671 Ref: nodejs#10543 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com>

MylesBorins · 2017-03-08T09:48:20Z

doesn't land cleanly on v4 and causes failures on v6... would be nice to see backported to v6

MylesBorins · 2017-05-08T15:15:13Z

ping re: backport

nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Dec 30, 2016

gibfahn reviewed Dec 30, 2016

View reviewed changes

gibfahn approved these changes Dec 30, 2016

View reviewed changes

cjihrig force-pushed the throws branch from 285707c to a132419 Compare December 30, 2016 16:57

nodejs-github-bot added the dont-land-on-v7.x label Dec 30, 2016

jasnell approved these changes Dec 30, 2016

View reviewed changes

Trott approved these changes Dec 31, 2016

View reviewed changes

lpinca approved these changes Dec 31, 2016

View reviewed changes

cjihrig force-pushed the throws branch from a132419 to cd17612 Compare January 2, 2017 14:48

nodejs-github-bot added the dont-land-on-v7.x label Jan 2, 2017

test: increase usage of assert.ifError()

746a46e

PR-URL: nodejs#10543 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

cjihrig force-pushed the throws branch from cd17612 to 746a46e Compare January 2, 2017 14:49

nodejs-github-bot added the dont-land-on-v7.x label Jan 2, 2017

cjihrig merged commit 746a46e into nodejs:master Jan 2, 2017

cjihrig deleted the throws branch January 2, 2017 14:50

not-an-aardvark added a commit to not-an-aardvark/node that referenced this pull request Jan 7, 2017

tools,test: enforce assert.ifError with lint rule

ef5b150

This adds an ESLint rule to enforce the use of `assert.ifError(err)` instead of `if (err) throw err;` in tests. Refs: nodejs#10543

not-an-aardvark mentioned this pull request Jan 7, 2017

tools,test: enforce assert.ifError with lint rule #10671

Closed

2 tasks

mscdex removed the dont-land-on-v7.x label Jan 13, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added dont-land-on-v4.x labels Mar 8, 2017

MylesBorins added lts-watch-v6.x and removed dont-land-on-v6.x labels Mar 8, 2017

MylesBorins added the backport-requested-v6.x label May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: increase usage of assert.ifError() #10543

test: increase usage of assert.ifError() #10543

cjihrig commented Dec 30, 2016

gibfahn Dec 30, 2016

Fishrock123 Dec 30, 2016

gibfahn Dec 30, 2016

cjihrig Dec 30, 2016

jasnell Dec 30, 2016

gibfahn commented Dec 30, 2016

cjihrig commented Dec 30, 2016

Trott commented Dec 31, 2016

Trott commented Dec 31, 2016

not-an-aardvark commented Dec 31, 2016 •

edited

Loading

gibfahn commented Jan 2, 2017 •

edited

Loading

MylesBorins commented Mar 8, 2017

MylesBorins commented May 8, 2017

test: increase usage of assert.ifError() #10543

test: increase usage of assert.ifError() #10543

Conversation

cjihrig commented Dec 30, 2016

Checklist

Affected core subsystem(s)

gibfahn Dec 30, 2016

Choose a reason for hiding this comment

Fishrock123 Dec 30, 2016

Choose a reason for hiding this comment

gibfahn Dec 30, 2016

Choose a reason for hiding this comment

cjihrig Dec 30, 2016

Choose a reason for hiding this comment

jasnell Dec 30, 2016

Choose a reason for hiding this comment

gibfahn commented Dec 30, 2016

cjihrig commented Dec 30, 2016

Trott commented Dec 31, 2016

Trott commented Dec 31, 2016

not-an-aardvark commented Dec 31, 2016 • edited Loading

gibfahn commented Jan 2, 2017 • edited Loading

MylesBorins commented Mar 8, 2017

MylesBorins commented May 8, 2017

not-an-aardvark commented Dec 31, 2016 •

edited

Loading

gibfahn commented Jan 2, 2017 •

edited

Loading