Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: changed test-http2-respond-file-range to use fixturesDir direct… #15852

Closed
wants to merge 1 commit into from
Closed

test: changed test-http2-respond-file-range to use fixturesDir direct… #15852

wants to merge 1 commit into from

Conversation

michirue
Copy link
Contributor

@michirue michirue commented Oct 6, 2017 •
edited by gibfahn
Loading

…ly from fixtures module.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
rmg
rmg approved these changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@rmg rmg left a comment

Choose a reason for hiding this comment

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

The change looks good.

@michirue if you have a chance, would you be able to clean up the commit message so that the first line if less than 50 characters?

If not, maybe whoever lands this could clean it up when they do.

@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
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 6, 2017
@gibfahn
Copy link
Member

gibfahn commented Oct 6, 2017

CI: https://ci.nodejs.org/job/node-test-commit/12830/

lpinca
lpinca approved these changes Oct 12, 2017
View reviewed changes
test/parallel/test-http2-respond-file-range.js
@@ -14,7 +15,7 @@ const {
HTTP2_HEADER_LAST_MODIFIED
} = http2.constants;

const fname = path.resolve(common.fixturesDir, 'printA.js');
const fname = path.resolve(fixturesDir, 'printA.js');
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 could be fixtures.fixturesPath('printA.js').

Copy link
Member

@jasnell jasnell Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Well, it would be fixtures.path('printA.js')

@mcollina
Copy link
Member

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

@mcollina
Copy link
Member

Failures are unrelated landing.

@mcollina mcollina self-assigned this Oct 17, 2017
mcollina
mcollina approved these changes Oct 17, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

Landed as d60f9b7.

@mcollina mcollina closed this Oct 17, 2017
mcollina pushed a commit that referenced this pull request Oct 17, 2017
@michirue @mcollina
test: http2-respond-file-range to use fixtures
d60f9b7 
Changed test-http2-respond-file-range to use fixtures.path directly
instead of passing through the common module.

PR-URL: #15852
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ryan Graham <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
@michirue @addaleax
test: http2-respond-file-range to use fixtures
e0237ca 
Changed test-http2-respond-file-range to use fixtures.path directly
instead of passing through the common module.

PR-URL: nodejs/node#15852
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ryan Graham <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@michirue @MylesBorins
test: http2-respond-file-range to use fixtures
2a5e7ec 
Changed test-http2-respond-file-range to use fixtures.path directly
instead of passing through the common module.

PR-URL: #15852
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ryan Graham <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

@rmg rmg rmg approved these changes

Assignees

@mcollina mcollina

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
@michirue @gibfahn @mcollina @rmg @jasnell @lpinca @mscdex @MylesBorins @Trott @nodejs-github-bot