test: rename regression tests with descriptive file names (pt. 3) #19275

ryzokuken · 2018-03-10T19:29:44Z

Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conforms to the standard test structure

Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

Checklist

test/parallel/test-regress-GH-4948.js
test/parallel/test-regress-GH-5051.js
test/parallel/test-regress-GH-5727.js
test/parallel/test-regress-GH-5927.js
test/parallel/test-regress-GH-6235.js
test/parallel/test-regress-GH-9819.js

BridgeAR · 2018-03-10T22:46:00Z

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

lpinca · 2018-03-11T08:22:25Z

test/parallel/test-net-listen-invalid-port.js

@@ -1,5 +1,10 @@
 'use strict';
 const common = require('../common');
+
+// This test ensures that port numbers are validated in *all* kinds of `listen`
+// calls. If an invalid port is supplied, ensures if a `RangeError` is thrown.


Nit: spurious if before "a rangeError"?

lpinca · 2018-03-11T08:51:06Z

test/parallel/test-http-socket-parser-null.js

 require('../common');
+
+// This test makes sure node doesn't crash when a request is sent and the
+// connection is closed early due to `this.socket.parser` getting set to null


I'm struggling to understand this description and test. I think this is actually not testing anything as IncomingMessage.prototype._dump() no longer touches this.socket.parser.

You're right. It no longer touches the value if it's null already. I'll try to improve this comment further.

ryzokuken · 2018-03-11T09:26:56Z

@lpinca does the new description seem satisfactory? I tried improving it, let me know if it's better.

lpinca · 2018-03-11T09:31:08Z

Yes, thanks. I think that test can be removed but it's probably better to discuss that in a separate and dedicated PR.

ryzokuken · 2018-03-11T09:34:48Z

@lpinca Sure! Do you think I should make an issue for that?

lpinca · 2018-03-11T09:40:24Z

@ryzokuken yes it makes sense. Opening a PR to start a discussion is also ok.

Refs: nodejs#19275 (comment)

PR-URL: #19279 Refs: #19275 (comment) Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

lpinca · 2018-03-13T14:38:54Z

@ryzokuken can you please remove d7a6757. I would also squash commits keeping only one and adding in the commit message something like this:

- Rename test-regress-GH-9819.js to test-crypto-tostring-segfault
- Rename test-regress-GH-5051.js to test-http-addrequest-localaddress
...

ryzokuken · 2018-03-13T17:20:27Z

@lpinca I hope this is what you were asking for? I'm sorry it was my first-time cherrypicking commits. 😅

lpinca · 2018-03-13T17:26:48Z

Yes, looks good, now if you can squash all of them into a single commit that would be great, otherwise we can do that before landing.

lpinca · 2018-03-13T17:30:32Z

If you want to do that and need guidance let me know.

ryzokuken · 2018-03-13T17:36:10Z

@lpinca thanks! I'll try it myself and bother you if I cannot manage to find a way to do it.

Rename the tests appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the tests conform to the standard test structure - Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault - Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress - Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port - Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe - Rename test-regress-nodejsGH-6235 to test-v8-global-setter Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

ryzokuken · 2018-03-13T17:52:06Z

@lpinca I hope it's good enough?

Gosh, people have been doing this for me all this time?

lpinca · 2018-03-13T17:54:00Z

Looks great. Thank you.
Will land this after a final CI run.

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

lpinca · 2018-03-13T19:07:58Z

Landed in 90b0538.

Rename the tests appropriately alongside mentioning the subsystem. Also, make a few basic changes to make sure the tests conform to the standard test structure. - Rename test-regress-GH-9819 to test-crypto-tostring-segfault - Rename test-regress-GH-5051 to test-http-addrequest-localaddress - Rename test-regress-GH-5727 to test-net-listen-invalid-port - Rename test-regress-GH-5927 to test-tty-stdin-pipe - Rename test-regress-GH-6235 to test-v8-global-setter PR-URL: #19275 Refs: #19105 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #19279 Refs: #19275 (comment) Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Rename the tests appropriately alongside mentioning the subsystem. Also, make a few basic changes to make sure the tests conform to the standard test structure. - Rename test-regress-GH-9819 to test-crypto-tostring-segfault - Rename test-regress-GH-5051 to test-http-addrequest-localaddress - Rename test-regress-GH-5727 to test-net-listen-invalid-port - Rename test-regress-GH-5927 to test-tty-stdin-pipe - Rename test-regress-GH-6235 to test-v8-global-setter PR-URL: #19275 Refs: #19105 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#19279 Refs: nodejs#19275 (comment) Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Rename the tests appropriately alongside mentioning the subsystem. Also, make a few basic changes to make sure the tests conform to the standard test structure. - Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault - Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress - Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port - Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe - Rename test-regress-nodejsGH-6235 to test-v8-global-setter PR-URL: nodejs#19275 Refs: nodejs#19105 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #19275 (comment)

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 10, 2018

BridgeAR approved these changes Mar 10, 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 Mar 10, 2018

lpinca reviewed Mar 11, 2018

View reviewed changes

lpinca approved these changes Mar 11, 2018

View reviewed changes

ryzokuken added a commit to ryzokuken/node that referenced this pull request Mar 11, 2018

test: delete test/parallel/test-regress-GH-4948

8f3a863

Refs: nodejs#19275 (comment)

ryzokuken mentioned this pull request Mar 11, 2018

test: delete test-regress-GH-4948 #19279

Closed

jasnell approved these changes Mar 11, 2018

View reviewed changes

ryzokuken force-pushed the test/rename-regression-2 branch from 9b6950d to dff7294 Compare March 13, 2018 17:19

ryzokuken force-pushed the test/rename-regression-2 branch from dff7294 to bc42761 Compare March 13, 2018 17:46

lpinca closed this Mar 13, 2018

targos mentioned this pull request Mar 20, 2018

v9.9.0 proposal #19428

Merged

Trott mentioned this pull request Apr 30, 2018

doc: remove squash guideline from onboarding doc #20413

Closed

2 tasks

BethGriggs pushed a commit that referenced this pull request Oct 16, 2018

test: delete test/parallel/test-regress-GH-4948

fe3836a

Refs: #19275 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: rename regression tests with descriptive file names (pt. 3) #19275

test: rename regression tests with descriptive file names (pt. 3) #19275

ryzokuken commented Mar 10, 2018 •

edited

Loading

BridgeAR commented Mar 10, 2018

lpinca Mar 11, 2018

lpinca Mar 11, 2018

ryzokuken Mar 11, 2018

ryzokuken commented Mar 11, 2018

lpinca commented Mar 11, 2018

ryzokuken commented Mar 11, 2018

lpinca commented Mar 11, 2018

lpinca commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

lpinca commented Mar 13, 2018

lpinca commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

lpinca commented Mar 13, 2018

lpinca commented Mar 13, 2018

test: rename regression tests with descriptive file names (pt. 3) #19275

test: rename regression tests with descriptive file names (pt. 3) #19275

Conversation

ryzokuken commented Mar 10, 2018 • edited Loading

Checklist

BridgeAR commented Mar 10, 2018

lpinca Mar 11, 2018

Choose a reason for hiding this comment

lpinca Mar 11, 2018

Choose a reason for hiding this comment

ryzokuken Mar 11, 2018

Choose a reason for hiding this comment

ryzokuken commented Mar 11, 2018

lpinca commented Mar 11, 2018

ryzokuken commented Mar 11, 2018

lpinca commented Mar 11, 2018

lpinca commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

lpinca commented Mar 13, 2018

lpinca commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

ryzokuken commented Mar 13, 2018

lpinca commented Mar 13, 2018

lpinca commented Mar 13, 2018

ryzokuken commented Mar 10, 2018 •

edited

Loading