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: cleanup test require symlink #8305

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@paulgrock
Contributor

paulgrock commented Aug 27, 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
  • Changed indexOf to includes for clarity.
  • Switched to assert.strictEqual from assert.equal
  • Changed some var to const
  • Test cleanup with common.refreshTmpDir
test: cleanup test require symlink
* Changed `==` to `includes` for clarity.
* Switched to `assert.strictEqual` from `assert.equal`
* Changed some `var` to `const`
* Test cleanup with `common.refreshTmpDir`
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

Changes LGTM if CI is happy.

There's an additional potential improvement around avoiding writing to the fixture directory. But that can be a separate good first contribution for someone else. It would involve copying the fixture to the temp directory or otherwise recreating the directory structure in the temp directory. The price for the do-not-write-to-the-fixture-dir purity will be increased text complexity. I'm OK with leaving that out of this PR.

Member

Trott commented Aug 27, 2016

Changes LGTM if CI is happy.

There's an additional potential improvement around avoiding writing to the fixture directory. But that can be a separate good first contribution for someone else. It would involve copying the fixture to the temp directory or otherwise recreating the directory structure in the temp directory. The price for the do-not-write-to-the-fixture-dir purity will be increased text complexity. I'm OK with leaving that out of this PR.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

CI is green.

Member

Trott commented Aug 27, 2016

CI is green.

@mscdex mscdex added the module label Aug 28, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 28, 2016

Contributor

LGTM with a comment.

Contributor

cjihrig commented Aug 28, 2016

LGTM with a comment.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 28, 2016

Member

LGTM

Member

benjamingr commented Aug 28, 2016

LGTM

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 28, 2016

Member

LGTM

Member

addaleax commented Aug 28, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2016

Member

LGTM

Member

jasnell commented Aug 29, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 30, 2016

test: cleanup test require symlink
* Changed `==` to `includes` for clarity.
* Switched to `assert.strictEqual` from `assert.equal`
* Changed some `var` to `const`
* Test cleanup with `common.refreshTmpDir`

PR-URL: #8305
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

Landed in eca74a9! Thank you!

Member

jasnell commented Aug 30, 2016

Landed in eca74a9! Thank you!

@jasnell jasnell closed this Aug 30, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: cleanup test require symlink
* Changed `==` to `includes` for clarity.
* Switched to `assert.strictEqual` from `assert.equal`
* Changed some `var` to `const`
* Test cleanup with `common.refreshTmpDir`

PR-URL: nodejs#8305
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: cleanup test require symlink
* Changed `==` to `includes` for clarity.
* Switched to `assert.strictEqual` from `assert.equal`
* Changed some `var` to `const`
* Test cleanup with `common.refreshTmpDir`

PR-URL: #8305
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment