test: refactor beforeExit tests by Trott · Pull Request #10581 · nodejs/node

Trott · 2017-01-03T01:26:48Z

Combine and rename tests for the beforeExit event on process.

The naming now more closely follows the de facto conventions of the
project.

The two tests were very similar and do not seem to benefit from being
separate.

Checklist

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

Affected core subsystem(s)

test process

sam-github · 2017-01-03T14:39:03Z

There are only 43 tests with mixed/camelCase, 1204 with lower case.

I don't think mixed case is conventional, and I don't think its a good idea. Some file systems aren't case-sensitive, and in the test names that do have camelCase, I can't figure out any pattern as to why. As far as I can see, the choise of camelCase or dash-seperators is completely random/arbitrary, and not the most common naming convention.

Fishrock123 · 2017-01-03T15:00:10Z

Yeah let's leave it lowercase.

Fishrock123 · 2017-01-03T15:00:54Z

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

thefourtheye · 2017-01-03T16:14:57Z

I wonder why nextTick is not tested here.

thefourtheye · 2017-01-03T16:16:56Z

Can these be closed over or something? If we add more tests later, having top level variables would make them inconvenient I guess.

sam-github · 2017-01-03T19:12:09Z

There are two dimensions of the tests: one is to test the various mechanisms to ensure they are dealt with (tick/timeout/net listen/etc.), the other is to test recursive use of a single mechanism.

There are lots of ways to refactor this, and there are dimensions not covered by the tests (not everything is done recursively), but in the absence of regressions, I don't know if anyone has the energy to write them all.

Fishrock123 · 2017-01-04T21:43:41Z

Actually after a second look I wonder if these shouldn't just be kept separate?

Trott · 2017-01-05T04:29:42Z

@sam-github Renamed the file so it is without mixed case. PTAL

Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate.

Trott · 2017-01-05T04:45:39Z

@thefourtheye Block-scoped the N and n variables. PTAL

Trott · 2017-01-05T04:50:46Z

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

Trott · 2017-01-05T06:16:20Z

Only failure in CI is Jenkins/Hudson related. But this test ran and succeeded before the failure, so CI is effectively ✅

Trott · 2017-01-06T00:57:21Z

Landed in d2c96af

Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate. PR-URL: #10581 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

sam-github · 2017-01-09T19:20:36Z

retroactive LGTM, @Trott , sorry, still digging out of my email

Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate. PR-URL: nodejs#10581 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate. PR-URL: #10581 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate. PR-URL: nodejs#10581 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins · 2017-03-08T10:01:39Z

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Trott added process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. labels Jan 3, 2017

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

thefourtheye reviewed Jan 3, 2017
View reviewed changes

Comment threadtest/parallel/test-process-beforeExit.js Outdated
thefourtheyeJan 3, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why nextTick is not tested here.

thefourtheye reviewed Jan 3, 2017
View reviewed changes

jasnell approved these changes Jan 3, 2017
View reviewed changes

thefourtheye mentioned this pull request Jan 3, 2017
doc: add test naming information to guide #10584
Closed
2 tasks

Trott force-pushed the beforeExit branch from ae5f38f to 23a2468Compare January 5, 2017 04:29

nodejs-github-bot added the dont-land-on-v7.x label Jan 5, 2017

test: refactor beforeExit tests
5f40989
Combine and rename tests for the `beforeExit` event on `process`. The naming now more closely follows the de facto conventions of the project. The two tests were very similar and do not seem to benefit from being separate.

Trott force-pushed the beforeExit branch from 23a2468 to 5f40989Compare January 5, 2017 04:45

nodejs-github-bot added the dont-land-on-v7.x label Jan 5, 2017

thefourtheye approved these changes Jan 5, 2017
View reviewed changes

Trott closed this Jan 6, 2017

mscdex removed the dont-land-on-v7.x label Jan 13, 2017

italoacasas mentioned this pull request Jan 25, 2017
Proposal to update v7.x-staging #10989
Closed

italoacasas mentioned this pull request Jan 29, 2017
v7.5.0 proposal #11062
Merged

MylesBorins added dont-land-on-v4.x labels Mar 8, 2017

Trott deleted the beforeExit branch January 13, 2022 22:44

Uh oh!

Uh oh!

Conversation

Trott commented Jan 3, 2017

Checklist

Affected core subsystem(s)

Uh oh!

sam-github commented Jan 3, 2017

Uh oh!

Fishrock123 commented Jan 3, 2017

Uh oh!

Fishrock123 commented Jan 3, 2017

Uh oh!

thefourtheyeJan 3, 2017

Choose a reason for hiding this comment

Uh oh!

thefourtheyeJan 3, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jan 3, 2017

Uh oh!

Fishrock123 commented Jan 4, 2017

Uh oh!

Trott commented Jan 5, 2017

Uh oh!

Trott commented Jan 5, 2017

Uh oh!

Trott commented Jan 5, 2017

Uh oh!

Trott commented Jan 5, 2017

Uh oh!

Trott commented Jan 6, 2017

Uh oh!

sam-github commented Jan 9, 2017

Uh oh!

MylesBorins commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants