Skip to content

test: refactor test to use latest standards#8582

Closed
burakgazi wants to merge 1 commit into
nodejs:masterfrom
burakgazi:master
Closed

test: refactor test to use latest standards#8582
burakgazi wants to merge 1 commit into
nodejs:masterfrom
burakgazi:master

Conversation

@burakgazi

@burakgazi burakgazi commented Sep 17, 2016

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@eljefedelrodeodeljefe eljefedelrodeodeljefe left a comment

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.

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

@burakgazi

Copy link
Copy Markdown
Author

@mcollina

@mcollina

Copy link
Copy Markdown
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
Copy Markdown
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.

@cjihrig cjihrig left a comment

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.

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

@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

@imyller imyller self-assigned this Sep 20, 2016

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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