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: replaces fixturesDir with fixtures #15835

Closed
wants to merge 3 commits into from
Closed

test: replaces fixturesDir with fixtures #15835

wants to merge 3 commits into from

Conversation

mikemfleming
Copy link

@mikemfleming mikemfleming commented Oct 6, 2017 •
edited
Loading

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)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mikemfleming mikemfleming changed the title replaces fixturesDir with fixtures test: replaces fixturesDir with fixtures Oct 6, 2017
test: replaces fixturesDir with fixtures
dc66f9e 
behold, my first commit to node.js coming from node interactive in Vancouver.
this commit updates the common.fixturesDir method in the https-timeout test.
pawelgolda
pawelgolda reviewed Oct 6, 2017
View reviewed changes
test/parallel/test-https-timeout.js Outdated
@@ -30,8 +30,8 @@ const https = require('https');
const fs = require('fs');

const options = {
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fs.readFileSync(`${common.fixtures}/keys/agent1-key.pem`),
Copy link
Contributor

Choose a reason for hiding this comment

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

common module does not export fixtures. This should be using method fixtures.readKey to get keys.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is not quite correct. The change would be first to:

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

Then...

const options = {
  key: fixtures.readKey('agent1-key.pem'),
  cert: fixtures.readKey('agent1-cert.pem')
}

@mscdex mscdex added the https Issues or PRs related to the https 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
jasnell
jasnell requested changes Oct 9, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Thank you very much for participating in the code-and-learn! We appreciate the contribution!

There are a couple of things that need to be corrected about this PR. You can do so by making the changes suggested below and adding a new commit to your development branch.

test/parallel/test-https-timeout.js Outdated
@@ -30,8 +30,8 @@ const https = require('https');
const fs = require('fs');

const options = {
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fs.readFileSync(`${common.fixtures}/keys/agent1-key.pem`),
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is not quite correct. The change would be first to:

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

Then...

const options = {
  key: fixtures.readKey('agent1-key.pem'),
  cert: fixtures.readKey('agent1-cert.pem')
}

@gireeshpunathil
Copy link
Member

/cc ping @mikemfleming to see if they can make the suggested changes.

@mikemfleming
Copy link
Author

Just saw this, working on it now!

gireeshpunathil
gireeshpunathil reviewed Oct 13, 2017
View reviewed changes
test/parallel/test-https-timeout.js Outdated
@@ -21,6 +21,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.

@ mikemfleming - on a second look, I guess this require statement can move down the line, after the hasCrypto check.

gireeshpunathil
gireeshpunathil reviewed Oct 13, 2017
View reviewed changes
test/parallel/test-https-timeout.js Outdated
@@ -30,8 +31,8 @@ const https = require('https');
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.

@ mikemfleming - on a second look, the requiring of fs module is not used anywhere in the test, and I guess is likely to invite linter error. Will you remove this please?

Copy link
Author

Choose a reason for hiding this comment

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

Just removed it

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

@gireeshpunathil
Copy link
Member

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

@joyeecheung
Copy link
Member

Landed in 3043937, thank you for the contribution!

joyeecheung pushed a commit that referenced this pull request Oct 14, 2017
@joyeecheung
test: replaces fixturesDir with fixtures
3043937 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: #15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@addaleax
test: replaces fixturesDir with fixtures
240a71f 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: nodejs/node#15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@targos
test: replaces fixturesDir with fixtures
ab87520 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: #15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@MylesBorins
test: replaces fixturesDir with fixtures
6c07e50 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: #15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@MylesBorins
test: replaces fixturesDir with fixtures
98def7d 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: #15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@MylesBorins
test: replaces fixturesDir with fixtures
541866e 
This commit updates the common.fixturesDir method
in the https-timeout test.

PR-URL: #15835
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[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

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@pawelgolda pawelgolda pawelgolda left review comments

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell 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. https Issues or PRs related to the https 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
@mikemfleming @gireeshpunathil @joyeecheung @jasnell @lpinca @pawelgolda @mscdex @MylesBorins @Trott @nodejs-github-bot