Skip to content

test: use fixtures module for path resolve#16842

Closed
sercanyersen wants to merge 1 commit intonodejs:masterfrom
sercanyersen:master
Closed

test: use fixtures module for path resolve#16842
sercanyersen wants to merge 1 commit intonodejs:masterfrom
sercanyersen:master

Conversation

@sercanyersen
Copy link
Contributor

@sercanyersen sercanyersen commented Nov 6, 2017

This is a part of the fixture use normalization process commit.

Changes in just one file where the path.resolve method replaced with fixtures.path.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 Nov 6, 2017
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Nov 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
@Trott
Copy link
Member

Trott commented Nov 6, 2017

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

Trott
Trott requested changes Nov 6, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi, @sercanyersen! Welcome and thanks for the PR! Because path is no longer used, the linter is complaining about its declaration. Can you remove it, commit to your branch, and push it up to GitHub so the changes show up in this PR? (Also, you can run make lint-js to see JS lint issues, if any.)

lpinca
lpinca reviewed Nov 7, 2017
View reviewed changes
test/parallel/test-http2-respond-with-fd-errors.js Outdated
Copy link
Member

@lpinca lpinca Nov 7, 2017

Choose a reason for hiding this comment

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

Nit: can you please move this after the common.hasCrypto check?

Trott
Trott approved these changes Nov 8, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I made the lint fix and nit fixes, rebased, and pushed to your branch. Hope that's OK!

@Trott
Copy link
Member

Trott commented Nov 8, 2017

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

@Trott
Copy link
Member

Trott commented Nov 12, 2017

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

@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

Retrying the one failed CI platform; https://ci.nodejs.org/job/node-test-commit-arm-fanned/12516/

@Trott
Copy link
Member

Trott commented Nov 13, 2017

Retrying Raspberry Pi again in the hopes it builds successfully this time: https://ci.nodejs.org/job/node-test-commit-arm-fanned/12540/

gireeshpunathil pushed a commit that referenced this pull request Nov 14, 2017
@sercanyersen @gireeshpunathil
test: use fixtures module for path resolve
803cacd 
PR-URL: #16842
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gireeshpunathil
Copy link
Member

gireeshpunathil commented Nov 14, 2017

Landed in 803cacd , thanks for the contribution!

evanlucas pushed a commit that referenced this pull request Nov 14, 2017
@sercanyersen @evanlucas
test: use fixtures module for path resolve
ad55db2 
PR-URL: #16842
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
@sercanyersen @MylesBorins
test: use fixtures module for path resolve
375bec0 
PR-URL: #16842
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees

No one assigned

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@sercanyersen @Trott @jasnell @gireeshpunathil @lpinca @targos @cjihrig @mscdex @MylesBorins @nodejs-github-bot