Skip to content

test: use fixtures module for path resolve - #16842

Closed
sercanyersen wants to merge 1 commit into
nodejs:masterfrom
sercanyersen:master
Closed

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

Conversation

@sercanyersen

@sercanyersensercanyersen commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@mscdexmscdex added the http2 Issues or PRs related to the http2 subsystem. label Nov 6, 2017
@TrottTrott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
@Trott

Trott commented Nov 6, 2017

Copy link
Copy Markdown
Member

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

Trott
Trott requested changes Nov 6, 2017
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

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
Comment threadtest/parallel/test-http2-respond-with-fd-errors.js Outdated

@lpincalpincaNov 7, 2017

Copy link
Copy Markdown
Member

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

@TrottTrott left a comment

Copy link
Copy Markdown
Member

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

Trott commented Nov 8, 2017

Copy link
Copy Markdown
Member

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

@Trott

Trott commented Nov 12, 2017

Copy link
Copy Markdown
Member

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

@jasnell

jasnell commented Nov 12, 2017

Copy link
Copy Markdown
Member

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

@Trott

Trott commented Nov 13, 2017

Copy link
Copy Markdown
Member

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

gireeshpunathil commented Nov 14, 2017

Copy link
Copy Markdown
Member

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>
@gibfahngibfahn mentioned this pull request Nov 21, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@lpincalpincalpinca approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes

Assignees

No one assigned

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.http2Issues or PRs related to the http2 subsystem.testIssues 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