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: replace common.fixturesDir with common.fixtures #15862

Closed
wants to merge 1 commit into from
Closed

test: replace common.fixturesDir with common.fixtures #15862

wants to merge 1 commit into from

Conversation

kasimdoctor
Copy link

Action-> Replaced common.fixturesDir in test-http2-create-client-secure-session.js 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 http2 Issues or PRs related to the http2 subsystem. 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
@Trott
Copy link
Member

Trott commented Oct 8, 2017

Hi, @kasimdoctor! Welcome and thanks for this! Can you update it to use fixtures.path() rather than fixtures.fixturesDir? See docs at https://github.com/nodejs/node/tree/master/test/common#fixtures-module

jasnell
jasnell reviewed Oct 10, 2017
View reviewed changes
test/parallel/test-http2-create-client-secure-session.js Outdated
@@ -13,7 +14,7 @@ const h2 = require('http2');

function loadKey(keyname) {
return fs.readFileSync(
path.join(common.fixturesDir, 'keys', keyname), 'binary');
path.join(fixtures.fixturesDir, 'keys', keyname), 'binary');
Copy link
Member

Choose a reason for hiding this comment

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

This can be:

return fixtures.readKey(keyname, 'binary');

Copy link
Author

@kasimdoctor kasimdoctor Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good point.

lpinca
lpinca reviewed Oct 10, 2017
View reviewed changes
test/parallel/test-http2-create-client-secure-session.js Outdated
@@ -1,6 +1,7 @@
'use strict';

const common = require('../common');
const fixtures = require('../common/fixtures');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you mind moving this after the common.hasCrypto check?

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

@kasimdoctor
Copy link
Author

kasimdoctor commented Oct 11, 2017 •
edited
Loading

@jasnell @lpinca Comments addressed. Thanks!

As a side note, I also rebased from latest upstream.

lpinca
lpinca reviewed Oct 11, 2017
View reviewed changes
test/parallel/test-http2-create-client-secure-session.js Outdated
@@ -5,15 +5,16 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const fixtures = require('../common/fixtures');

const assert = require('assert');
const path = require('path');
const fs = require('fs');
Copy link
Member

Choose a reason for hiding this comment

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

We no longer need fs as it is no longer used, can you please remove it? The linter is not happy if we keep it :)

Copy link
Author

Choose a reason for hiding this comment

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

Oh yea, missed that. Thanks!

lpinca
lpinca approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with the last nit fixed.

@kasimdoctor
Copy link
Author

@lpinca Done.

@lpinca
Copy link
Member

lpinca commented Oct 11, 2017

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

@kasimdoctor
Copy link
Author

Looking at the linter output, I realized that path also should have been removed along with fs.

@lpinca
Copy link
Member

lpinca commented Oct 11, 2017

@kasimdoctor correct, if you can please do it :)

@kasimdoctor
Copy link
Author

@lpinca Done, please trigger CI again. Thanks!

@lpinca
Copy link
Member

lpinca commented Oct 11, 2017

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

@kasimdoctor
Copy link
Author

@lpinca Strange, but I no longer can check the status of the CI builds. I get the Read permission denied. Is this intentional?

@lpinca
Copy link
Member

lpinca commented Oct 12, 2017

No it isn't intentional afaik. Anyway there is only a failure which is unrelated to this change.
Will land in a bit.

@lpinca
Copy link
Member

lpinca commented Oct 12, 2017 •
edited
Loading

Landed in d828c01. Thank you!

@lpinca lpinca closed this Oct 12, 2017
lpinca pushed a commit that referenced this pull request Oct 12, 2017
@lpinca
test: use fixtures.readKey()
d828c01 
Use `fixtures.readKey()` in test-http2-create-client-secure-session.js.

PR-URL: #15862
Reviewed-By: Luigi Pinca <[email protected]>
@kasimdoctor
Copy link
Author

kasimdoctor commented Oct 12, 2017 •
edited
Loading

Thanks @lpinca! Safe to delete the branch I guess?

@Trott
Copy link
Member

Trott commented Oct 12, 2017

Thanks @lpinca! Safe to delete the branch I guess?

@kasimdoctor Yes.

@kasimdoctor kasimdoctor deleted the test-http2-client-session-fix branch October 12, 2017 16:36
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@addaleax
test: use fixtures.readKey()
9cb8a01 
Use `fixtures.readKey()` in test-http2-create-client-secure-session.js.

PR-URL: nodejs/node#15862
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@targos
test: use fixtures.readKey()
0b7993e 
Use `fixtures.readKey()` in test-http2-create-client-secure-session.js.

PR-URL: #15862
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@targos
test: use fixtures.readKey()
181ee5d 
Use `fixtures.readKey()` in test-http2-create-client-secure-session.js.

PR-URL: #15862
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 left review comments

@lpinca lpinca lpinca 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.
7 participants
@kasimdoctor @Trott @lpinca @jasnell @mscdex @MylesBorins @nodejs-github-bot