test: rename regression tests with descriptive file names #19212

ryzokuken · 2018-03-07T20:56:20Z

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-1531.js
test/parallel/test-regress-GH-2245.js
test/parallel/test-regress-GH-3238.js
test/parallel/test-regress-GH-3542.js
test/parallel/test-regress-GH-3739.js
test/parallel/test-regress-GH-4256.js

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

Change test-fs-existssync-false.js to make sure it conforms to the standard test structure. Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

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

Trott · 2018-03-08T19:44:53Z

test/parallel/test-fs-readfilesync-enoent.js


 const assert = require('assert');
 const fs = require('fs');
 const path = require('path');

+if (!common.isWindows)
+  common.skip('Windows specific test.');


Nit: I don't think this should be moved. I think it should be left where it was.

Trott · 2018-03-08T19:45:30Z

test/parallel/test-http-request-agent.js

@@ -1,5 +1,6 @@
 'use strict';
 const common = require('../common');
+const fixtures = require('../common/fixtures');


Nit: I don't think this should be moved either. I think it should be left where it was.

Trott · 2018-03-08T19:48:16Z

No objections, seems like a good thing to me.

I do think the shuffling around of unrelated things shouldn't happen in this PR, or even happen at all. I think this is probably the most efficient and obvious order of things, which is how these tests already operate:

load common
check common.isWindows or whatever if we need to skip the test on some platforms
then load everything else because:
- if there is a condition where the test will be skipped, we want that to be super-obvious to someone reading the test so put it right there at the top
- there's no point in loading a bunch of other modules if we're just going to skip the test

Trott · 2018-03-08T19:49:07Z

(Thanks for doing this! Those test names have always been kind of meh to me.)

Trott · 2018-03-08T19:49:30Z

test/parallel/test-cluster-kill-disconnect.js

+
+// Check that cluster works perfectly for both `kill` and `disconnect` cases.
+// Also take into account that the `disconnect` event may be received after the
+// `exit` event.


Nit: Maybe add a link to the GH issue that this test was named for originally?

I don't know how I could have missed this, will do.

ryzokuken · 2018-03-08T20:06:43Z

@Trott I totally agree with you on the Windows part, I too agree that if the test is unrelated, it should be skipped as early as possible, even from a performance point of view.

Regarding const fixtures = require('../common/fixtures'); though, I moved it to the top so that it comforms to https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure. Let me know if that's a non-issue and I'd move it right back.

P.S. I totally missed that it was perfectly conforming to the test structure, it was also about the "skipping" part. Fixing that in a sec.

Thanks.

Refs: nodejs#19212 Refs: Refs: nodejs#19105

BridgeAR

LGTM even though it would be nice if the nits would be addressed but that is not a blocker :-)

BridgeAR · 2018-03-08T20:32:57Z

test/parallel/test-fs-existssync-false.js

 const assert = require('assert');
 const fs = require('fs');
 const path = require('path');

-const tmpdir = require('../common/tmpdir');
-


Nit: I personally would not mode this.

Isn't that in conflict with the canonical test structure?

Ref: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-1-3

This is specific to the require('../common'); but it is fine to keep it as is.

BridgeAR · 2018-03-08T20:33:13Z

test/parallel/test-fs-existssync-false.js

+const tmpdir = require('../common/tmpdir');
+
+// This test ensures that fs.existsSync doesn't incorrectly return false
+// (especially on Windows)


Nit: It would be nice to punctuate the new comments.

Refs: nodejs#19212 Refs: Refs: nodejs#19105

Refs: nodejs#19212

BridgeAR

Still LG

joyeecheung · 2018-03-10T15:33:18Z

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

Rename the tests appropriately alongside mentioning the subsystem. Also, make a few basic changes to make sure the test conforms to the standard test structure. This renames: - test-regress-nodejsGH-1531 - test-regress-nodejsGH-2245 - test-regress-nodejsGH-3238 - test-regress-nodejsGH-3542 - test-regress-nodejsGH-3739 - test-regress-nodejsGH-4256 PR-URL: nodejs#19212 Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-03-11T03:18:35Z

Landed in 580ad01 🎉

Rename the tests appropriately alongside mentioning the subsystem. Also, make a few basic changes to make sure the test conforms to the standard test structure. This renames: - test-regress-GH-1531 - test-regress-GH-2245 - test-regress-GH-3238 - test-regress-GH-3542 - test-regress-GH-3739 - test-regress-GH-4256 PR-URL: #19212 Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: Rich Trott <rtrott@gmail.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 test conforms to the standard test structure. This renames: - test-regress-nodejsGH-1531 - test-regress-nodejsGH-2245 - test-regress-nodejsGH-3238 - test-regress-nodejsGH-3542 - test-regress-nodejsGH-3739 - test-regress-nodejsGH-4256 PR-URL: nodejs#19212 Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: Rich Trott <rtrott@gmail.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 test conforms to the standard test structure. This renames: - test-regress-GH-1531 - test-regress-GH-2245 - test-regress-GH-3238 - test-regress-GH-3542 - test-regress-GH-3739 - test-regress-GH-4256 PR-URL: #19212 Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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

ryzokuken changed the title ~~test: rename test-regress-GH-1531~~ [WIP] test: rename regression tests with descriptive file names Mar 7, 2018

ryzokuken added 6 commits March 8, 2018 02:42

test: change test-fs-existssync-false.js

3dd9688

Change test-fs-existssync-false.js to make sure it conforms to the standard test structure. Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

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

Trott reviewed Mar 8, 2018

View reviewed changes

Trott approved these changes Mar 8, 2018

View reviewed changes

test: handle (almost) all nits in nodejs#19212

8e50585

Refs: nodejs#19212 Refs: Refs: nodejs#19105

BridgeAR approved these changes Mar 8, 2018

View reviewed changes

ryzokuken added 2 commits March 9, 2018 02:05

test: handle all nits in nodejs#19212

fb4050f

Refs: nodejs#19212 Refs: Refs: nodejs#19105

test: punctuate tests in nodejs#19212

c8654f5

Refs: nodejs#19212

BridgeAR approved these changes Mar 8, 2018

View reviewed changes

jasnell approved these changes Mar 8, 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

BridgeAR closed this Mar 11, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

BethGriggs added the land-on-v8.x label Oct 2, 2018

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: rename regression tests with descriptive file names #19212

test: rename regression tests with descriptive file names #19212

ryzokuken commented Mar 7, 2018 •

edited

Loading

Trott Mar 8, 2018

Trott Mar 8, 2018

Trott commented Mar 8, 2018

Trott commented Mar 8, 2018

Trott Mar 8, 2018

ryzokuken Mar 8, 2018

ryzokuken commented Mar 8, 2018 •

edited

Loading

BridgeAR left a comment

BridgeAR Mar 8, 2018

ryzokuken Mar 8, 2018

BridgeAR Mar 8, 2018

BridgeAR Mar 8, 2018

BridgeAR left a comment

joyeecheung commented Mar 10, 2018

BridgeAR commented Mar 11, 2018

test: rename regression tests with descriptive file names #19212

test: rename regression tests with descriptive file names #19212

Conversation

ryzokuken commented Mar 7, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 8, 2018

Trott commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Mar 8, 2018 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 10, 2018

BridgeAR commented Mar 11, 2018

ryzokuken commented Mar 7, 2018 •

edited

Loading

ryzokuken commented Mar 8, 2018 •

edited

Loading