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: in test/parallel/test-fs-realpath.js: #8769

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@mpmckenna8
Contributor

mpmckenna8 commented Sep 25, 2016

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

test: in test/parallel/test-fs-realpath.js:

Replaced .indexOf() with .includes() for more clarity as to
what it is doing.

Many of the instances of var changed to const.

Instances of assert.equal() refactored to assert.strictEqual() or
assert.ifError()

Removed the unlinkSync() call in the exit event handler because it
probably only handles files in the testing tmp directory so there is
no need to unlink them.

@mscdex mscdex added the fs label Sep 25, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott
Member

Trott commented Sep 25, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 25, 2016

Member

Whoops, let's try that again:

It looks like this has one lint issue (line longer than 80 chars). Run make jslint (or .\vcbuild jslint on Windows).

FWIW, that should have been caught by make test (because that runs the linter too).

Member

Trott commented Sep 25, 2016

Whoops, let's try that again:

It looks like this has one lint issue (line longer than 80 chars). Run make jslint (or .\vcbuild jslint on Windows).

FWIW, that should have been caught by make test (because that runs the linter too).

@mpmckenna8

This comment has been minimized.

Show comment
Hide comment
@mpmckenna8

mpmckenna8 Sep 25, 2016

Contributor

Ok, so I made sure all the line lengths are less than 80 characters. And no longer get linter errors.

Should I squash my commit messages together or leave the linted commit on there?

Contributor

mpmckenna8 commented Sep 25, 2016

Ok, so I made sure all the line lengths are less than 80 characters. And no longer get linter errors.

Should I squash my commit messages together or leave the linted commit on there?

test: in test/parallel/test-fs-realpath.js:
Replaced .indexOf() with .includes() for more clarity as to
what it is doing.

Many of the instances of var changed to const.

Instances of assert.equal() refactored to assert.strictEqual() or
assert.ifError()

Removed the unlinkSync() call in the exit event handler because it
probably only handles files in the testing tmp directory so there is
no need to unlink them.
@mpmckenna8

This comment has been minimized.

Show comment
Hide comment
@mpmckenna8

mpmckenna8 Sep 25, 2016

Contributor

So i squashed it together so there should just be one commit message from me which I think follows the guidelines. Let me know if there's anything else I can do.

Contributor

mpmckenna8 commented Sep 25, 2016

So i squashed it together so there should just be one commit message from me which I think follows the guidelines. Let me know if there's anything else I can do.

@Trott

This comment has been minimized.

Show comment
Hide comment
@imyller

LGTM

@imyller imyller self-assigned this Sep 27, 2016

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 27, 2016

Member

I'll start landing this:

  • Two LGTMs
  • No objections
  • Requested changes have been made
  • CI tests passed (only known/unrelated CI failures)
Member

imyller commented Sep 27, 2016

I'll start landing this:

  • Two LGTMs
  • No objections
  • Requested changes have been made
  • CI tests passed (only known/unrelated CI failures)

imyller added a commit to imyller/node that referenced this pull request Sep 27, 2016

test: cleanup parallel/test-fs-realpath.js
Replaced .indexOf() with .includes() for more clarity as to
what it is doing.

Many of the instances of var changed to const.

Instances of assert.equal() refactored to assert.strictEqual() or
assert.ifError()

Removed the unlinkSync() call in the exit event handler because it
probably only handles files in the testing tmp directory so there is
no need to unlink them.

PR-URL: nodejs#8769
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 27, 2016

Member

landed in 73ae2d1

Thank you for your contribution, @mpmckenna8

Member

imyller commented Sep 27, 2016

landed in 73ae2d1

Thank you for your contribution, @mpmckenna8

@imyller imyller closed this Sep 27, 2016

@imyller imyller removed their assignment Sep 27, 2016

jasnell added a commit that referenced this pull request Sep 29, 2016

test: cleanup parallel/test-fs-realpath.js
Replaced .indexOf() with .includes() for more clarity as to
what it is doing.

Many of the instances of var changed to const.

Instances of assert.equal() refactored to assert.strictEqual() or
assert.ifError()

Removed the unlinkSync() call in the exit event handler because it
probably only handles files in the testing tmp directory so there is
no need to unlink them.

PR-URL: #8769
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

Fishrock123 added a commit that referenced this pull request Oct 11, 2016

test: cleanup parallel/test-fs-realpath.js
Replaced .indexOf() with .includes() for more clarity as to
what it is doing.

Many of the instances of var changed to const.

Instances of assert.equal() refactored to assert.strictEqual() or
assert.ifError()

Removed the unlinkSync() call in the exit event handler because it
probably only handles files in the testing tmp directory so there is
no need to unlink them.

PR-URL: #8769
Reviewed-By: Rich Trott <rtrott@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