-
Notifications
You must be signed in to change notification settings - Fork 30k
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 commom.fixturesDir with common.fixtures #15967
Conversation
@@ -5,8 +5,8 @@ const assert = require('assert'); | |||
const fs = require('fs'); | |||
const path = require('path'); | |||
|
|||
const fn = path.join(common.fixturesDir, 'elipses.txt'); | |||
const rangeFile = path.join(common.fixturesDir, 'x.txt'); | |||
const fn = path.join(common.fixtures, 'elipses.txt'); |
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 one would fail tests as common
module does not export fixtures. Have a look at test/common/fixtures.js
module and use fixtures.path
.
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.
Correct, the change here should be:
const fixtures = require('../common/fixtures');
then...
const fn = fixtures.path('elipses.txt');
const rangeFile = fixtures.path('x.txt');
Thank you so much for participating in the code and learn and for the contribution. There are a couple of additional edits that are necessary as explained in the comments. Those can be made by adding a new commit to your development branch and pushing those to your fork. |
@jasnell - PTAL |
CI is showing green, but no tests were actually run. Something is not working on CI. Will have to run again after it's fixed. ping @nodjes/build (I'll leave this comment on all the other PRs that this affects as of now, but I won't ping build on each one.) |
CI failures are unrelated. Landed in 9aa41cb, thanks for the contribution! |
PR-URL: #15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #15967 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test