test: rename regression tests with descriptive file names by ryzokuken · Pull Request #19212 · nodejs/node · GitHub

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

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

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

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

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

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.

@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

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

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.

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

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

Refs: nodejs#19212

Still LG

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>

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

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

               const path = require('path');
+              if (!common.isWindows)
+                common.skip('Windows specific test.');

+              // 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.

		const path = require('path');

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

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

test: rename regression tests with descriptive file names by ryzokuken · Pull Request #19212 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: rename regression tests with descriptive file names - #19212

test: rename regression tests with descriptive file names#19212ryzokuken wants to merge 10 commits intonodejs:masternodejs/node:masterfrom ryzokuken:test/rename-regression-1ryzokuken/node:test/rename-regression-1Copy head branch name to clipboard

Conversation

ryzokuken commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 8, 2018

Uh oh!

Trott commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 10, 2018

Uh oh!

BridgeAR commented Mar 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test: rename regression tests with descriptive file names#19212
ryzokuken wants to merge 10 commits into
nodejs:masterfrom
ryzokuken:test/rename-regression-1

ryzokuken commented Mar 7, 2018 •
edited
Loading

ryzokuken commented Mar 8, 2018 •
edited
Loading