-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 common.fixturesDir with common.fixtures #15949
Conversation
@@ -1,11 +1,12 @@ | |||
'use strict'; | |||
const common = require('../common'); | |||
const { path } = require('../common/fixtures'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat confusing because it has the same name as the path
module and therefore I would normally expect it to be the path module. It would be nicer to use the fixture name here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, even though I agree with @BridgeAR and would like to see that comment addressed.
PR-URL: nodejs#15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Landed in 3a26555 Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 ! |
PR-URL: #15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs/node#15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: #15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: #15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: #15949 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
None