test: remove assert.doesNotThrow() by BridgeAR · Pull Request #18669 · nodejs/node · GitHub

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

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

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.

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.

Thanks, still LGTM at a quick glance.

Rebased due to conflicts.

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

Rubber stamp LGTM

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?

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()`.

Rebased due to conflicts.

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

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

LGTM

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Prohibit the usage of `assert.doesNotThrow()`. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Prohibit the usage of `assert.doesNotThrow()`. Backport-PR-URL: #19244 PR-URL: #18669 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Prohibit the usage of `assert.doesNotThrow()`. PR-URL: nodejs#18669 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 outstanding review comments, 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

test: remove assert.doesNotThrow() by BridgeAR · Pull Request #18669 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: remove assert.doesNotThrow() - #18669

test: remove assert.doesNotThrow()#18669BridgeAR wants to merge 4 commits intonodejs:masternodejs/node:masterfrom BridgeAR:remove-does-not-throwBridgeAR/node:remove-does-not-throwCopy head branch name to clipboard

Conversation

BridgeAR commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR commented Feb 9, 2018

Uh oh!

bnoordhuis Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 10, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 10, 2018

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 12, 2018

Uh oh!

BridgeAR commented Feb 12, 2018

Uh oh!

BridgeAR commented Feb 12, 2018

Uh oh!

BridgeAR commented Feb 12, 2018

Uh oh!

BridgeAR commented Feb 13, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 16, 2018

Uh oh!

BridgeAR commented Mar 10, 2018

Uh oh!

MylesBorins commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: remove assert.doesNotThrow()#18669
BridgeAR wants to merge 4 commits into
nodejs:masterfrom
BridgeAR:remove-does-not-throw

BridgeAR commented Feb 9, 2018 •
edited
Loading

BridgeAR Feb 10, 2018 •
edited
Loading