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 /parallel/test-cluster-uncaught-exception.js to ES6 #9239

Closed
wants to merge 2 commits into from
Closed

test: refactor /parallel/test-cluster-uncaught-exception.js to ES6 #9239

wants to merge 2 commits into from

Conversation

deverickapollo
Copy link
Contributor

@deverickapollo deverickapollo commented Oct 22, 2016

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

Test

Description of change

This commit replaces function expressions with ES6 arrow
functions as well as improves the comparison operator to
check if operands are of same types.

This commit replaces function expressions with ES6 arrow
functions as well as improve the comparison operator to
check if operands are of same types.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 22, 2016
process.exit(MAGIC_EXIT_CODE);
});
});
master.on('exit', common.mustCall(code => assert.strictEqual(code, MAGIC_EXIT_CODE)));
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you ran make -j8 test (or just make test, either is fine) on these changes? I would expect the linter to complain about this line as being too long.


var isTestRunner = process.argv[2] != 'child';
const isTestRunner = process.argv[2] !== 'child';

if (isTestRunner) {
var master = fork(__filename, ['child']);
Copy link
Member

Choose a reason for hiding this comment

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

I think this var can be a const too, no?

@mscdex mscdex added the cluster Issues and PRs related to the cluster subsystem. label Oct 22, 2016

} else if (cluster.isMaster) {
process.on('uncaughtException', () => process.nextTick(() => process.exit(MAGIC_EXIT_CODE)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is much less readable than the current code.

Correction made to previous commit, correcting a line length
restriction.
@deverickapollo
Copy link
Contributor Author

I updated the test file to fix max line errors and edited the arrow function to make it more readable.

@Trott
Copy link
Member

Trott commented Oct 23, 2016

@Trott
Copy link
Member

Trott commented Oct 23, 2016

Nit: The first line of the commit log should not capitalize anything that isn't an acronym or otherwise requires capitalization. In other words "Refactor" should be "refactor". Wouldn't worry much about changing it right now. Whoever lands the code can change it (as they'll be squashing the multiple commits anyway). Just FYI for subsequent contributions.

@Trott
Copy link
Member

Trott commented Oct 23, 2016

LGTM if CI is OK.

@silverwind
Copy link
Contributor

The commit title should include the test name to not be confusing.

@deverickapollo deverickapollo changed the title test: Refactor to ES6 test: refactor /parallel/test-cluster-uncaught-exception.js to ES6 Oct 23, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM, can fix the commit log messages when landing

jasnell pushed a commit that referenced this pull request Oct 26, 2016
Replaces function expressions with ES6 arrow functions as well as
improve the comparison operator to check if operands are of same types.

PR-URL: #9239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

Landed with nits addressed and edited commit long in 383e40b

@jasnell jasnell closed this Oct 26, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
Replaces function expressions with ES6 arrow functions as well as
improve the comparison operator to check if operands are of same types.

PR-URL: #9239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
Replaces function expressions with ES6 arrow functions as well as
improve the comparison operator to check if operands are of same types.

PR-URL: #9239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants