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: changed common.fixturesDir to use common.fixtures module #15904

Closed
wants to merge 1 commit into from

Conversation

RaphaelRheault
Copy link
Contributor

@RaphaelRheault RaphaelRheault commented Oct 6, 2017

Done at Code & Learn for Node.js Interactive Vancouver

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@RaphaelRheault RaphaelRheault changed the title fix: changed the test-fs-realpath test to use the common/fixtures module test: changed common.fixturesDir to use common.fixtures module Oct 6, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@@ -185,7 +186,7 @@ function test_deep_relative_dir_symlink(callback) {
common.printSkipMessage('symlink test (no privs)');
return runNextTest();
}
const expected = path.join(common.fixturesDir, 'cycles', 'folder');
const expected = path.join(fixtures.fixturesDir, 'cycles', 'folder');
Copy link
Member

Choose a reason for hiding this comment

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

There is the fixtures.path function for this and it would be nice to use that instead.

@@ -156,7 +157,7 @@ function test_deep_relative_file_symlink(callback) {
return runNextTest();
}

const expected = path.join(common.fixturesDir, 'cycles', 'root.js');
const expected = path.join(fixtures.fixturesDir, 'cycles', 'root.js');
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to const expected = fixtures.path('cycles', 'root.js')?

@@ -185,7 +186,7 @@ function test_deep_relative_dir_symlink(callback) {
common.printSkipMessage('symlink test (no privs)');
return runNextTest();
}
const expected = path.join(common.fixturesDir, 'cycles', 'folder');
const expected = path.join(fixtures.fixturesDir, 'cycles', 'folder');
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to const expected = fixtures.path('cycles', 'folder')?

@BridgeAR
Copy link
Member

Ping @RaphaelRheault

@apapirovski
Copy link
Member

Hi @RaphaelRheault, would you like to follow up on this so that it can land? A few small changes were requested above — let me know if you need any pointers. Thanks for helping us improve Node!

@Trott
Copy link
Member

Trott commented Oct 28, 2017

I fixed the nits and force-pushed. This should be good to go. PTAL.

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

@tniessen
Copy link
Member

Landed in 6ff2909, thank you! 🎉

@tniessen tniessen closed this Oct 28, 2017
tniessen pushed a commit that referenced this pull request Oct 28, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15904
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants