test: rename regression tests with descriptive file names #19161

ryzokuken · 2018-03-06T05:35:53Z

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

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Checklist

test/sequential/test-regress-GH-1697.js
test/sequential/test-regress-GH-1726.js
test/sequential/test-regress-GH-4015.js
test/sequential/test-regress-GH-4027.js
test/sequential/test-regress-GH-784.js
test/sequential/test-regress-GH-877.js

ryzokuken · 2018-03-06T06:15:03Z

@joyeecheung @BridgeAR please take a look

richardlau

Good start. Big 👍 for also updating to conform to the standard test structure.

richardlau · 2018-03-06T10:08:24Z

test/sequential/test-regress-fs-stat_sync.js

+// as a property on an object results in a NULL pointer dereference in release
+// builds and an assert in debug builds.
+// https://github.com/nodejs/node-v0.x-archive/issues/4015
+


Maybe rename the fixture used a few lines down for consistency?

richardlau · 2018-03-06T10:11:06Z

test/sequential/test-regress-net-response_size.js

@@ -21,6 +21,11 @@

 'use strict';
 const common = require('../common');
+
+// Make sure the net module's server doesn't throw an error when handling
+// responses that are either too long or too small (especially on windows)


Small nit: Windows

joyeecheung · 2018-03-06T11:52:00Z

Thanks for doing this! At this point I think we can drop the regress in the file names, names like just test-net-response-size.js should be fine.

EDIT: oops forgot to change the file name

joyeecheung · 2018-03-06T11:55:13Z

test/sequential/test-regress-fs-stat_sync.js

@@ -22,6 +22,14 @@
 'use strict';
 require('../common');
 const { fixturesDir } = require('../common/fixtures');
+
+// Check that the calls to Integer::New() and Date::New() succeed and bail out


This file can be named test-fs-statSync-overflow.js since it is testing that specific behavior of fs.statSync.

Also, can you rename the fixtures used below as well? Thanks

joyeecheung · 2018-03-06T11:56:59Z

test/sequential/test-regress-fs-watch_file.js

@@ -21,6 +21,17 @@

 'use strict';
 const common = require('../common');
+
+// Make sure the deletion event gets reported in the following scenario:


test-regress-fs-watch_file.js seems a bit general. Maybe name this test-fs-watchFile-enoent-after-deletion?

joyeecheung · 2018-03-06T11:59:01Z

test/sequential/test-regress-net-response_size.js

@@ -21,6 +21,11 @@

 'use strict';
 const common = require('../common');
+
+// Make sure the net module's server doesn't throw an error when handling


Can you use hyphens instead of underscores in the file name for consistency? Also for camel cases I think they can either stay in the name as camelCase or be made snake cases like camel-case

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

Rename affected tests to remove the "regress" keyword as well as improve naming scheme. Refs: nodejs#19105

Refs: nodejs#19105

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

BridgeAR · 2018-03-06T17:49:33Z

I removed the "fixes" part in the description. This does fix all sequential entries but not the parallel ones.

BridgeAR · 2018-03-06T17:50:51Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/227/

ryzokuken · 2018-03-06T17:56:51Z

Thanks, @BridgeAR! You're absolutely right. I should've paid more attention and done it myself, sorry.

I talked to @joyeecheung and we decided to break the original issue (#19105) plus a few other tests that were differently named (but somewhat on the same lines) and were missed into multiple PRs, so as to make sure that the PRs weren't too large.

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Refs: #19105 PR-URL: #19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename affected tests to remove the "regress" keyword as well as improve naming scheme. Refs: #19105 PR-URL: #19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Refs: #19105 PR-URL: #19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: nodejs#19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename affected tests to remove the "regress" keyword as well as improve naming scheme. Refs: nodejs#19105 PR-URL: nodejs#19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Refs: nodejs#19105 PR-URL: nodejs#19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: nodejs#19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename affected tests to remove the "regress" keyword as well as improve naming scheme. Refs: #19105 PR-URL: #19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Refs: #19105 PR-URL: #19161 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

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

richardlau reviewed Mar 6, 2018

View reviewed changes

joyeecheung reviewed Mar 6, 2018

View reviewed changes

ryzokuken added 5 commits March 6, 2018 18:21

test: rename tests to remove "regress" keyword

24016c4

Rename affected tests to remove the "regress" keyword as well as improve naming scheme. Refs: nodejs#19105

ryzokuken force-pushed the test/rename-regression branch from 6b700b1 to 24016c4 Compare March 6, 2018 13:41

ryzokuken added 3 commits March 6, 2018 19:29

test: address nits and rename the corresponding fixture

5b96920

Refs: nodejs#19105

ryzokuken changed the title ~~[WIP] test: rename regression tests with descriptive file names~~ test: rename regression tests with descriptive file names Mar 6, 2018

jasnell approved these changes Mar 6, 2018

View reviewed changes

BridgeAR approved these changes Mar 6, 2018

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Mar 6, 2018

joyeecheung approved these changes Mar 6, 2018

View reviewed changes

richardlau approved these changes Mar 7, 2018

View reviewed changes

gireeshpunathil approved these changes Mar 7, 2018

View reviewed changes

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

MylesBorins added the land-on-v8.x label Aug 17, 2018

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: rename regression tests with descriptive file names #19161

test: rename regression tests with descriptive file names #19161

ryzokuken commented Mar 6, 2018 •

edited by BridgeAR

Loading

ryzokuken commented Mar 6, 2018

richardlau left a comment

richardlau Mar 6, 2018

richardlau Mar 6, 2018

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung Mar 6, 2018

joyeecheung Mar 6, 2018

joyeecheung Mar 6, 2018 •

edited

Loading

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

ryzokuken commented Mar 6, 2018

test: rename regression tests with descriptive file names #19161

test: rename regression tests with descriptive file names #19161

Conversation

ryzokuken commented Mar 6, 2018 • edited by BridgeAR Loading

Checklist

Affected core subsystem(s)

Checklist

ryzokuken commented Mar 6, 2018

richardlau left a comment

Choose a reason for hiding this comment

richardlau Mar 6, 2018

Choose a reason for hiding this comment

richardlau Mar 6, 2018

Choose a reason for hiding this comment

joyeecheung commented Mar 6, 2018 • edited Loading

joyeecheung Mar 6, 2018

Choose a reason for hiding this comment

joyeecheung Mar 6, 2018

Choose a reason for hiding this comment

joyeecheung Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

ryzokuken commented Mar 6, 2018

ryzokuken commented Mar 6, 2018 •

edited by BridgeAR

Loading

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung Mar 6, 2018 •

edited

Loading