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

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

Merged
merged 1 commit into from
Jan 2, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 30, 2016

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.

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Dec 30, 2016
@@ -247,10 +247,7 @@ function runTest(assertCleaned) {
useColors: false,
terminal: true
}, function(err, repl) {
if (err) {
console.error(`Failed test # ${numtests - tests.length}`);
Copy link
Member

Choose a reason for hiding this comment

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

Is the failed test number useful here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored that one.

Copy link
Member

Choose a reason for hiding this comment

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

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

@gibfahn
Copy link
Member

gibfahn commented Dec 30, 2016

@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
Copy link
Contributor Author

cjihrig commented Dec 30, 2016

@Trott
Copy link
Member

Trott commented Dec 31, 2016

@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
Copy link
Member

Trott commented Dec 31, 2016

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
Copy link
Contributor

not-an-aardvark commented Dec 31, 2016

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Jan 2, 2017

@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).

not-an-aardvark added a commit to not-an-aardvark/node that referenced this pull request Jan 7, 2017
This adds an ESLint rule to enforce the use of `assert.ifError(err)`
instead of `if (err) throw err;` in tests.

Refs: nodejs#10543
jasnell pushed a commit that referenced this pull request Jan 9, 2017
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10543
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michal Zasso <[email protected]>
@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

ping re: backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants