Skip to content

Conversation

andyband7
Copy link
Contributor

@andyband7 andyband7 commented Oct 6, 2017

Removes the usage of common.fixturesDir and replaces with common.fixtures module

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

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the util Issues and PRs related to the built-in util module. 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
let err;
const badSyntaxPath =
path.join(common.fixturesDir, 'syntax', 'bad_syntax')
path.join(fixtures.fixturesDir, 'syntax', 'bad_syntax')
Copy link
Member

Choose a reason for hiding this comment

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

I think this should actually use the fixtures.path function.

Copy link
Member

Choose a reason for hiding this comment

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

For instance,

const badSyntaxPath = fixtures.path('syntax', 'bad_syntax');

@BridgeAR
Copy link
Member

Ping @andyband7

@andyband7
Copy link
Contributor Author

Thanks for the reminder @BridgeAR . Ive committed an updated test, is this how you meant it?

@gireeshpunathil
Copy link
Member

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

AIX issue was not related to this PR.

@mhdawson
Copy link
Member

Windows failures was a not related to this PR. CI run looks good.

@mhdawson
Copy link
Member

@BridgeAR if the change matches what you had in mind I think this is ready to land.

let err;
const badSyntaxPath =
path.join(common.fixturesDir, 'syntax', 'bad_syntax')
fixtures.path('syntax', 'bad_syntax')
Copy link
Member

Choose a reason for hiding this comment

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

nit: this line can be merged with the line above now. This can be handled during landing

@joyeecheung
Copy link
Member

Landed in 76abf0f, thanks!

joyeecheung pushed a commit that referenced this pull request Oct 19, 2017
PR-URL: #15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
PR-URL: #15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[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: #15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15947
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[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. test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants