Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: refactor test-beforeexit-event-exit.js #10577

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 2, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@italoacasas italoacasas added the process Issues and PRs related to the process subsystem. label Jan 2, 2017
@Trott
Copy link
Member

Trott commented Jan 2, 2017

Perhaps too tangentially related to this change, but since "refactor" is general, perhaps not?:

We have (at least) three test files for the beforeExit event and they are all (IMO) sub-optimally named:

  • test-processs-before-exit.js
  • test-beforeexit-event-exit.js
  • test-beforeexit-event.js

Perhaps we can take this opportunity to rename them all to test-process-beforeExit-*?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 2, 2017

A search for 'beforeExit' in the test directory returned 9 files. If you let me know exactly which ones you want renamed (just the three you mentioned above?), I can rename them in another commit.

@Trott
Copy link
Member

Trott commented Jan 3, 2017

@cjihrig Looking more closely at it, two of the tests are awfully similar and could/should probably be merged. I have a feeling once we start pulling on this thread, lots of other things may shake out, so, uh, yeah, never mind. :-D I'll probably try to sort it out after this lands.

@Trott
Copy link
Member

Trott commented Jan 3, 2017

(Actually, if you could just rename this test to test/parallel/test-process-beforeExit-exit.js, that would be great. I'll rename the others.)

@sam-github
Copy link
Contributor

I commented elsewhere, but it looks to me like use of camelCase in the test folder is unconventional, most of the project sticks to non-case-sensitive filenaming conventions. Wouldn't we better getting rid of the camelCase, converting to minus-seperators, the dominant convention?

@@ -1,9 +1,8 @@
'use strict';
require('../common');
var assert = require('assert');
const common = require('../common');

process.on('beforeExit', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.mustCall(..., 0) also will work in this case I guess.

@Trott
Copy link
Member

Trott commented Jan 4, 2017

I commented elsewhere, but it looks to me like use of camelCase in the test folder is unconventional, most of the project sticks to non-case-sensitive filenaming conventions. Wouldn't we better getting rid of the camelCase, converting to minus-seperators, the dominant convention?

@sam-github You make some good points here and elsewhere. The counterargument is that knowing to do that conversion is one more piece of friction. "I know I'm looking for a beforeExit test. Let's look for tests with beforeExit in the name! None? OK. Maybe it's beforeexit. Or before-exit. Or something, I guess?" I'd rather there be just one and only one name for the event/method/whatever, not different names depending on if I'm looking for a filename or not.

I don't feel that strongly enough about it to push for it, and in the absence of a champion, status quo wins out. Sadly, it's not just the 43 mixed case files to consider changing. It's also lowercase everything vs. kebab-case everything: test-fs-readfilesync-pipe-large.js or test-fs-read-file-sync-hostname.js. :-(

TL;DR: We can leave this filename as is. :-D

@sam-github
Copy link
Contributor

Let's look for tests with beforeExit in the name! None? OK. Maybe it's beforeexit. Or before-exit. Or something, I guess?

agreed, the mapping should be clear and consistent. Could be I've been burned to badly with projects that literally could not be checked out on OS X or Windows because of case changes, and yes, that was with Subversion, but my interactions with tooling on mixed case projects have been poor at times.

In this case, if there is a majority practice, I'd document it, and then we can evolve towards it.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 5, 2017

PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit 7f69972 into nodejs:master Jan 5, 2017
@cjihrig cjihrig deleted the beforeexit branch January 5, 2017 17:07
@mscdex mscdex added test Issues and PRs related to the tests. and removed dont-land-on-v7.x labels Jan 13, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
PR-URL: #10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2017
PR-URL: #10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
PR-URL: #10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs/node#10577
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants