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 https agent disable session #15901

Closed
wants to merge 1 commit into from
Closed

Test https agent disable session #15901

wants to merge 1 commit into from

Conversation

irynagrom
Copy link

@irynagrom irynagrom commented Oct 6, 2017 •
edited
Loading

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
watilde
watilde reviewed Oct 6, 2017
View reviewed changes
test/parallel/test-https-agent-disable-session-reuse.js Outdated

const TOTAL_REQS = 2;

const options = {
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readSync('/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.

We also can use fixtures.readFixtureKey.

+  key: fixtures.readFixtureKey('agent1-key.pem'),
-  key: fixtures.readSync('/keys/agent1-key.pem'),

Copy link
Author

Choose a reason for hiding this comment

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

in the fixtures.js the readFixturesKey is exported as readKey. I have updated the branch

@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
BridgeAR
BridgeAR reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-https-agent-disable-session-reuse.js Outdated
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readKey('/keys/agent1-key.pem'),
cert: fixtures.readSync('/keys/agent1-cert.pem')
Copy link
Member

Choose a reason for hiding this comment

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

The keys directory should not be used anymore. You only need the filename.

Copy link
Member

Choose a reason for hiding this comment

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

These are also using two different methods... it should be:

key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem')

Copy link
Member

Choose a reason for hiding this comment

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

oops! didn't see that @joyeecheung had already pointed that out below :-)

joyeecheung
joyeecheung reviewed Oct 11, 2017
View reviewed changes
test/parallel/test-https-agent-disable-session-reuse.js Outdated
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readKey('/keys/agent1-key.pem'),
cert: fixtures.readSync('/keys/agent1-cert.pem')
Copy link
Member

Choose a reason for hiding this comment

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

Can you change these to:

key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem'),

?

cjihrig
cjihrig approved these changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM once @joyeecheung's nit is addressed.

@BridgeAR
Copy link
Member

Ping @irynaricker

@apapirovski
Copy link
Member

Hi @irynaricker, it looks like a few tiny changes are required to land this pull request. Would you still be interested in getting this done? Let me know if any pointers are needed, and thanks for helping us improve Node!

@Trott
test: use fixtures module
6e45eb0 
Replace usage of common.fixturesDir with fixtures module.
@Trott
Copy link
Member

Trott commented Oct 28, 2017

I went ahead and fixed the last few nits. Hope that's OK. PTAL. This should be good to go.

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

@irynaricker If you have any feedback on what we could or should have done differently to make this go smoother for you, I would love to hear it. (My email is in my GitHub profile.) If this was just a case of "got busy" or "lost interest", then cool. But if it was a case of "got frustrated", it would be helpful for me to know that.

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 29, 2017
@Trott
test: use fixtures module
1906df1 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: nodejs#15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 29, 2017

Landed in 1906df1.

Thanks for the contribution! 🎉

@Trott Trott closed this Oct 29, 2017
@tniessen
Copy link
Member

@irynaricker The email address you used to commit the changes does not match your GitHub email address, so the commit cannot be associated with your account. Please add the email address you used to commit the changes to your account in the GitHub settings if you would like this change to be linked to your profile.

@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@gibfahn
test: use fixtures module
5454229 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@gibfahn
test: use fixtures module
3912793 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@gibfahn
test: use fixtures module
ce07cbe 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@Qard
test: use fixtures module
f7b3ffe 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: nodejs/node#15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@Qard
test: use fixtures module
8cb7518 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: nodejs/node#15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@MylesBorins
test: use fixtures module
d71d8c9 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[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: use fixtures module
fdc2a2d 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@MylesBorins
test: use fixtures module
d6dc579 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: #15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
test: use fixtures module
a5dc598 
Replace usage of common.fixturesDir with fixtures module in
test-https-agent-disable-session-reuse.js.

PR-URL: nodejs/node#15901
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@watilde watilde watilde left review comments

@jasnell jasnell jasnell left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig 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.