test: remove assert.doesNotThrow() #18669

BridgeAR · 2018-02-09T01:45:00Z

There is actually no reason to use assert.doesNotThrow() in the tests. If a test threw, just let the error bubble up right away instead of first catching it and then rethrowing it. That way we get the best stack trace and normally also the best possible error message.

Someone might argue that there are rare cases (I guess about 10) where it would actually make sense to keep the current behavior. The reason would be that a loop is used to execute something and a generated error message would be thrown that is individual per entry. Otherwise it might be hard to know what entry failed.

Nevertheless, I think a better way to deal with those would be to add a console.log. And we can add those as soon as something fails.

I can not think of any other reason to keep assert.doesNotThrow(). I personally feel we might want to deprecate that function actually.

I kept the calls in the assertion test file because I work on that right now in a different PR and those mainly test assert.doesNotThrow() on its own.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

BridgeAR · 2018-02-09T01:45:44Z

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

bnoordhuis · 2018-02-09T13:42:16Z

test/addons/symlinked-module/test.js

@@ -30,5 +30,5 @@ const sub = require('./submodule');
  const mod = require(path.join(i, 'binding.node'));
  assert.notStrictEqual(mod, null);
  assert.strictEqual(mod.hello(), 'world');
-  assert.doesNotThrow(() => sub.test(i));
+  sub.test(i);


In a couple of places (like here) the doesNotThrow() is really a comment in disguise.

Someone reading the new code might be confused and think the sub.test(i) is a bug or dead code because the return value isn't checked, not realizing it's there to check a side effect (throwing an exception.)

I would suggest rewriting lines like these to sub.test(i); // Should not throw.. Apart from that getting rid of doesNotThrow() is (IMO) a good move.

I went through the code and added comments were I felt it made sense. Some times I also improved a few tests along the way and added punctuation and caps and things like that.

BridgeAR · 2018-02-10T01:37:44Z

I went through the code and added comments were I felt it made sense that it should not throw. Some times I also improved a few tests along the way and added punctuation and caps and things like that. I also removed one that was outdated.

bnoordhuis

Thanks, still LGTM at a quick glance.

BridgeAR · 2018-02-10T16:16:47Z

Rebased due to conflicts.

Light-CI https://ci.nodejs.org/job/node-test-commit-light/252/

lpinca

Rubber stamp LGTM

BridgeAR · 2018-02-12T11:36:22Z

I added two more commits: the first one cleans up the rest of the doesNotThrow statements that should not be there (I did not do most of those before because of a otherwise conflicting PR that landed in the meanwhile).

The second commit adds a eslint rule to prohibit the usage of assert.doesNotThrow. I could of course also remove that commit again and open a separate PR for it after this lands. I just thought it can probably also be included here. Is everyone fine with that?

BridgeAR · 2018-02-12T11:37:19Z

Light-CI https://ci.nodejs.org/job/node-test-commit-light/257/

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throw, just let the error bubble up right away instead of first catching it and then rethrowing it.

Add punctuation and comments about code that should not throw.

Prohibit the usage of `assert.doesNotThrow()`.

BridgeAR · 2018-02-12T14:55:44Z

Rebased due to conflicts.

BridgeAR · 2018-02-12T14:56:22Z

Light-CI https://ci.nodejs.org/job/node-test-commit-light/258/

BridgeAR · 2018-02-13T13:18:10Z

New Light-CI https://ci.nodejs.org/job/node-test-commit-light/264/

mcollina

LGTM

BridgeAR · 2018-02-16T15:56:16Z

Landed in 4d3c3f0...15bb843

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throws, just let the error bubble up right away instead of first catching it and then rethrowing it. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

BridgeAR · 2018-03-10T00:34:01Z

Backported in #19244

@MylesBorins I am not certain in what way this correlates to the mentioned PR?

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throws, just let the error bubble up right away instead of first catching it and then rethrowing it. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Add punctuation and comments about code that should not throw. Also remove a obsolete test and refactor some tests. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Prohibit the usage of `assert.doesNotThrow()`. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throws, just let the error bubble up right away instead of first catching it and then rethrowing it. Backport-PR-URL: #19244 PR-URL: #18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Add punctuation and comments about code that should not throw. Also remove a obsolete test and refactor some tests. Backport-PR-URL: #19244 PR-URL: #18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Prohibit the usage of `assert.doesNotThrow()`. Backport-PR-URL: #19244 PR-URL: #18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throws, just let the error bubble up right away instead of first catching it and then rethrowing it. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Add punctuation and comments about code that should not throw. Also remove a obsolete test and refactor some tests. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Prohibit the usage of `assert.doesNotThrow()`. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

MylesBorins · 2018-07-31T18:40:17Z

Backport requested in #19244

nodejs-github-bot added addons Issues and PRs related to native addons. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Feb 9, 2018

bnoordhuis approved these changes Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018

bnoordhuis approved these changes Feb 10, 2018

View reviewed changes

cjihrig approved these changes Feb 10, 2018

View reviewed changes

BridgeAR mentioned this pull request Feb 10, 2018

assert: adds rejects() and doesNotReject() #18023

Closed

4 tasks

BridgeAR force-pushed the remove-does-not-throw branch from 954cc07 to 6ee92a9 Compare February 10, 2018 16:16

lpinca approved these changes Feb 11, 2018

View reviewed changes

BridgeAR added 3 commits February 12, 2018 15:55

test: remove assert.doesNotThrow()

e080569

There is actually no reason to use `assert.doesNotThrow()` in the tests. If a test throw, just let the error bubble up right away instead of first catching it and then rethrowing it.

test: minor refactoring

43851aa

Add punctuation and comments about code that should not throw.

tools: add assert.doesNotThrow eslint rule

f3570c8

Prohibit the usage of `assert.doesNotThrow()`.

BridgeAR force-pushed the remove-does-not-throw branch from b354a9b to f3570c8 Compare February 12, 2018 14:55

fixup - remove doesNotThrow

17b6061

mcollina approved these changes Feb 16, 2018

View reviewed changes

BridgeAR closed this Feb 16, 2018

BridgeAR mentioned this pull request Mar 8, 2018

[v9.x backport] This is backporting multiple commits that were requested #19244

Closed

4 tasks

MylesBorins added backported-to-v9.x and removed backport-requested-v9.x labels Mar 21, 2018

targos mentioned this pull request Mar 21, 2018

v9.9.0 proposal #19428

Merged

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

BridgeAR deleted the remove-does-not-throw branch April 1, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: remove assert.doesNotThrow() #18669

test: remove assert.doesNotThrow() #18669

BridgeAR commented Feb 9, 2018 •

edited

Loading

BridgeAR commented Feb 9, 2018

bnoordhuis Feb 9, 2018

BridgeAR Feb 10, 2018 •

edited

Loading

BridgeAR commented Feb 10, 2018

bnoordhuis left a comment

BridgeAR commented Feb 10, 2018

lpinca left a comment

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 13, 2018

mcollina left a comment

BridgeAR commented Feb 16, 2018

BridgeAR commented Mar 10, 2018

MylesBorins commented Jul 31, 2018

test: remove assert.doesNotThrow() #18669

test: remove assert.doesNotThrow() #18669

Conversation

BridgeAR commented Feb 9, 2018 • edited Loading

Checklist

Affected core subsystem(s)

BridgeAR commented Feb 9, 2018

bnoordhuis Feb 9, 2018

Choose a reason for hiding this comment

BridgeAR Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Feb 10, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 10, 2018

lpinca left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR commented Feb 13, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 16, 2018

BridgeAR commented Mar 10, 2018

MylesBorins commented Jul 31, 2018

BridgeAR commented Feb 9, 2018 •

edited

Loading

BridgeAR Feb 10, 2018 •

edited

Loading