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

BridgeAR · 2018-03-08T23:27:43Z

This is a backport of multiple commits that were requested to be backported. It just seemed easier for me to do this in one batch. I can also split this into multiple PRs if requested.

#18610
#18669
#18745
#18758
#18831
#18857
#17919

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

BridgeAR · 2018-03-08T23:35:12Z

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

MylesBorins · 2018-03-20T11:58:50Z

@BridgeAR this is going to need to be rebased. It also looks like failures may have been related

This adds puctiations to the comments, uses a capital letters for the first character, removes a few obsolete comments and switches to assert.ok when suitable. It also moves all `assert.deepEqual()` and `assert.deepStrictEqual()` tests to the appropriate file. PR-URL: nodejs#18610 Reviewed-By: Joyee Cheung <[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]>

This enables the `no-unsafe-finally` eslint rule to make sure we have a proper control flow in try / catch. PR-URL: nodejs#18745 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Move the print statements below a console.log call. PR-URL: nodejs#18758 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

This also fixes the three entries that did not pass. PR-URL: nodejs#18831 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs#18831 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

This implements a function based system. Instead of passing in the error code as first argument, the error code itself is a error class. It already contains the correct error type, so while adding a new error no one has to think about the error type anymore. In case a single error code has more than one error type, the error class has properties for the non default error types. Those can be used as fallback. This prevents typos, makes the implementation easier and it is less verbose when writing the code for a new error. The implementation itself does not interfere with the old implementation. So the old and the new system can co-exist and it is possible to slowly migrate the old ones to the new system. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This updates all internal errors to the new error type. While doing so it removes unused errors. A few errors currently seem to have the wrong type. To identify them later, comments were added next to the error type. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This ports the errors to the new error system. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Some error types are not properly set. This adds comments which ones are probably falty and to what they should be set instead. PR-URL: nodejs#18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#17919 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Lance Ball <[email protected]>

BridgeAR · 2018-03-21T00:32:08Z

Rebased and I fixed the linting issue as well.

BridgeAR · 2018-03-21T00:49:22Z

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

This adds puctiations to the comments, uses a capital letters for the first character, removes a few obsolete comments and switches to assert.ok when suitable. It also moves all `assert.deepEqual()` and `assert.deepStrictEqual()` tests to the appropriate file. Backport-PR-URL: #19244 PR-URL: #18610 Reviewed-By: Joyee Cheung <[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]>

This enables the `no-unsafe-finally` eslint rule to make sure we have a proper control flow in try / catch. Backport-PR-URL: #19244 PR-URL: #18745 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Move the print statements below a console.log call. Backport-PR-URL: #19244 PR-URL: #18758 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

This also fixes the three entries that did not pass. Backport-PR-URL: #19244 PR-URL: #18831 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Backport-PR-URL: #19244 PR-URL: #18831 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

This implements a function based system. Instead of passing in the error code as first argument, the error code itself is a error class. It already contains the correct error type, so while adding a new error no one has to think about the error type anymore. In case a single error code has more than one error type, the error class has properties for the non default error types. Those can be used as fallback. This prevents typos, makes the implementation easier and it is less verbose when writing the code for a new error. The implementation itself does not interfere with the old implementation. So the old and the new system can co-exist and it is possible to slowly migrate the old ones to the new system. Backport-PR-URL: #19244 PR-URL: #18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This updates all internal errors to the new error type. While doing so it removes unused errors. A few errors currently seem to have the wrong type. To identify them later, comments were added next to the error type. Backport-PR-URL: #19244 PR-URL: #18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This ports the errors to the new error system. Backport-PR-URL: #19244 PR-URL: #18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Some error types are not properly set. This adds comments which ones are probably falty and to what they should be set instead. Backport-PR-URL: #19244 PR-URL: #18857 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #19244 PR-URL: #17919 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Lance Ball <[email protected]>

MylesBorins · 2018-03-21T10:52:50Z

landed in 4e9279d...55f7bbb

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

Assuming all of these are going to need to be manually backported to 8.x as well. Do they make sense to backport?

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Mar 8, 2018

MylesBorins force-pushed the v9.x-staging branch 3 times, most recently from d457b9d to 03c321a Compare March 20, 2018 11:56

BridgeAR force-pushed the v9-backports branch from 99d51e1 to cc189e7 Compare March 21, 2018 00:27

BridgeAR added 15 commits March 21, 2018 01:31

doc: update buffer examples

01e5624

Move the print statements below a console.log call. PR-URL: nodejs#18758 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

BridgeAR force-pushed the v9-backports branch from cc189e7 to 6086e71 Compare March 21, 2018 00:31

MylesBorins closed this Mar 21, 2018

MylesBorins mentioned this pull request Jul 31, 2018

fix common.expectsError type checking #13686

Closed

4 tasks

BridgeAR deleted the v9-backports branch April 1, 2019 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BridgeAR commented Mar 8, 2018

BridgeAR commented Mar 8, 2018

MylesBorins commented Mar 20, 2018

BridgeAR commented Mar 21, 2018

BridgeAR commented Mar 21, 2018

MylesBorins commented Mar 21, 2018

MylesBorins commented Jul 31, 2018

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

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

Conversation

BridgeAR commented Mar 8, 2018

Checklist

BridgeAR commented Mar 8, 2018

MylesBorins commented Mar 20, 2018

BridgeAR commented Mar 21, 2018

BridgeAR commented Mar 21, 2018

MylesBorins commented Mar 21, 2018

MylesBorins commented Jul 31, 2018