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:replace indexOf, assert.equal, add mustCall #8766

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@richHong
Contributor

richHong commented Sep 24, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

In test/parallel/test-fs-symlink.js:

Line 15: the use of o.indexOf() can be replaced with a usage of o.includes() which will make it more clear.
Line 43: the use of assert.equal() can be assert.strictEqual() instead
Line 28: the callback should be wrapped in common.mustCall() like in other instances of callbacks later in the file.

@mscdex mscdex added the fs label Sep 24, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 25, 2016

Member

LGTM (either with or without the assert.ifError() suggestion) if CI is green: https://ci.nodejs.org/job/node-test-pull-request/4272/

Member

Trott commented Sep 25, 2016

LGTM (either with or without the assert.ifError() suggestion) if CI is green: https://ci.nodejs.org/job/node-test-pull-request/4272/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 26, 2016

Member

LGTM with the ifError change.

Member

jasnell commented Sep 26, 2016

LGTM with the ifError change.

@lpinca

lpinca approved these changes Sep 27, 2016

LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment

Trott added a commit to Trott/io.js that referenced this pull request Sep 28, 2016

test:replace indexOf, assert.equal, add mustCall()
replace indexOf with includes
replace assert.equal with assert.strictEqual
add common.mustCall
replace throw error with assert.ifError

PR-URL: nodejs#8766
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 28, 2016

Member

Landed in 8cd2306

Member

Trott commented Sep 28, 2016

Landed in 8cd2306

@Trott Trott closed this Sep 28, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 28, 2016

Contributor

Just a minor nit, but I noticed a missing space between subsystem name and first part of the actual message.

Contributor

mscdex commented Sep 28, 2016

Just a minor nit, but I noticed a missing space between subsystem name and first part of the actual message.

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

test:replace indexOf, assert.equal, add mustCall()
replace indexOf with includes
replace assert.equal with assert.strictEqual
add common.mustCall
replace throw error with assert.ifError

PR-URL: #8766
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

test:replace indexOf, assert.equal, add mustCall()
replace indexOf with includes
replace assert.equal with assert.strictEqual
add common.mustCall
replace throw error with assert.ifError

PR-URL: #8766
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment