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 to use latest standards #8582

Closed
wants to merge 1 commit into from

Conversation

burakgazi
Copy link

@burakgazi burakgazi commented Sep 17, 2016

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

test

Description of change

refactor test to use latest standards
- change var to const
- function to =>
- equal to strictEqual

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels Sep 17, 2016
@eljefedelrodeodeljefe
Copy link
Contributor

LGTM. Can you please include the description of changes into the commit message?

Copy link
Contributor

@eljefedelrodeodeljefe eljefedelrodeodeljefe left a comment

Choose a reason for hiding this comment

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

LGTM with a nit. Can you please include the description of changes into the commit message?

@burakgazi
Copy link
Author

@mcollina

@mcollina
Copy link
Member

@burakgazi there is a typo in the commit message (standarts -> standards), and also the commas do not have a space before them.

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

    Refactored the code to latest standards, where all var is changed to const, functions are changed to arrow functions and assert.equal chaned to assert.strictEqual
}, function() {
socket2.bind({port: common.PORT + 1, exclusive: true}, function() {
}, () => {
socket2.bind({ port: common.PORT + 1, exclusive: true }, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the preferred style in core was to not have spaces inside of the curly braces. I could be wrong though.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't really see a reason for moving to arrow functions in this case.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

@eljefedelrodeodeljefe Could you update your review? I believe the changes you've requested have been made. Thank you.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

@imyller imyller self-assigned this Sep 20, 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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller
Copy link
Member

imyller commented Sep 20, 2016

I'll start landing this:

  • Four LGTMs
  • No objections
  • Requested nit changes have been addressed (commit message will be amended when landing)
  • CI tests passed (only the usual CI failures)

@imyller
Copy link
Member

imyller commented Sep 20, 2016

landed in 0bfd103

Thank you for your contribution, @burakgazi

imyller pushed a commit that referenced this pull request Sep 20, 2016
Refactored the code to latest standards, where all var is
changed to const, functions are changed to arrow functions
and assert.equal chaned to assert.strictEqual

PR-URL: #8582
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller imyller closed this Sep 20, 2016
@imyller imyller removed their assignment Sep 20, 2016
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Refactored the code to latest standards, where all var is
changed to const, functions are changed to arrow functions
and assert.equal chaned to assert.strictEqual

PR-URL: #8582
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
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. dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants