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.

Sign up for GitHub

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

Jump to bottom

Add 'fixtures' property to common.js exports #15959

Closed
wants to merge 1 commit into from
Closed

Add 'fixtures' property to common.js exports #15959

wants to merge 1 commit into from

Conversation

kanikashah90
Copy link

Two tasks are involved as part of this change:

  1. Add 'fixtures' property to common.js.
  2. Use the new 'fixtures' property of common.js in test-fs-error-messages.js instead of 'fixturesDir'
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label 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
@benjamingr
Copy link
Member

I don't understand this change

BridgeAR
BridgeAR reviewed Oct 7, 2017
View reviewed changes
test/common/index.js Outdated
@@ -36,6 +36,7 @@ const testRoot = process.env.NODE_TEST_DIR ?

const noop = () => {};

exports.fixtures = fixturesDir;
Copy link
Member

Choose a reason for hiding this comment

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

The idea is not about adding the fixturesDir to this file. It is about loading the fixtures in the specific test file and to use the explicit fixture function provided there. In this case path.

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Will push out the change with the recommendation provided.

BridgeAR
BridgeAR requested changes Oct 7, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Just marking the necessary change explicit to prevent this from being landed without that.

tniessen
tniessen requested changes Oct 7, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I am going to actually block this PR as I don't think it should be landed as it is. I will dismiss my review once the above comments have been addressed.

@gibfahn
Copy link
Member

gibfahn commented Oct 7, 2017 •
edited
Loading

@kanikashah90 instead of what you have, would you mind doing something like this:

add this line to the test:

const fixtures = require('../common/fixtures');

Change the path.join lines like this:

-const fn = path.join(common.fixturesDir, 'non-existent'); 
+const fn = fixtures.path('non-existent');

see the API docs here.

If you could push a new commit to your branch with these changes that would be great (and then this can be landed).

@kanikashah90 kanikashah90 force-pushed the fixturesDirToFixtures branch 2 times, most recently from b227e8a to 5a74175 Compare October 11, 2017 07:40
@kanikashah90 kanikashah90 force-pushed the fixturesDirToFixtures branch from 5a74175 to 68a0f08 Compare October 11, 2017 07:42
@kanikashah90
Copy link
Author

@gibfahn: I updated the changes with the above recommendation. I hope this change finds a place in the main code.

@gireeshpunathil
Copy link
Member

@BridgeAR , @tniessen PTAL.

tniessen
tniessen reviewed Oct 13, 2017
View reviewed changes
test/parallel/test-fs-error-messages.js
@@ -21,14 +21,15 @@

'use strict';
const common = require('../common');
const fixtures = require('../common/fixtures');
const assert = require('assert');
const path = require('path');
Copy link
Member

Choose a reason for hiding this comment

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

If I am not mistaken, path is not actually used anymore, so this line should be removed.

@BridgeAR
Copy link
Member

The commit message does not follow our guidelines but I fix that while landing.

@BridgeAR BridgeAR self-assigned this Oct 18, 2017
BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
@kanika46shah @BridgeAR
test: use fixtures module
932499e 
PR-URL: #15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member

Landed in 932499e

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !

@BridgeAR BridgeAR closed this Oct 19, 2017
@kanikashah90
Copy link
Author

@BridgeAR : Happy to see the change merged. Thanks :)

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@kanika46shah @MylesBorins
test: use fixtures module
489168d 
PR-URL: #15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@kanika46shah @addaleax
test: use fixtures module
3ba19b7 
PR-URL: nodejs/node#15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@kanika46shah @MylesBorins
test: use fixtures module
2afb8e7 
PR-URL: #15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@kanika46shah @MylesBorins
test: use fixtures module
567d1e2 
PR-URL: #15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@kanika46shah @MylesBorins
test: use fixtures module
cbbe125 
PR-URL: #15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@kanika46shah @addaleax
test: use fixtures module
4674b42 
PR-URL: nodejs/node#15959
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@watilde watilde watilde approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

Assignees

@BridgeAR BridgeAR

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.