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: switch to use common.fixtures.readKey #15814

Closed
wants to merge 2 commits into from
Closed

test: switch to use common.fixtures.readKey #15814

wants to merge 2 commits into from

Conversation

UnrememberMe
Copy link
Contributor

@UnrememberMe UnrememberMe commented Oct 6, 2017 •
edited
Loading

switch from common.fixturesDir to common.fixtures.readKey in test-https-strict.js

PR-URL: #15814
Refs: code-and-learn 2017 node.js interactive
Reviewed-By: @benjamingr, @UnrememberMe, @cjihrig, @tniessen, @richardlau

@UnrememberMe
test: switch to use common.fixtures.fixturesDir
8b088b1 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
rmg
rmg reviewed 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.

You can find the docs for fixtures.readKey() at https://github.com/nodejs/node/tree/master/test/common#fixturesreadkeyarg-enc

test/parallel/test-https-strict.js Outdated
@@ -33,7 +34,7 @@ const fs = require('fs');
const path = require('path');

function file(fname) {
return path.resolve(common.fixturesDir, 'keys', fname);
return path.resolve(fixtures.fixturesDir, 'keys', fname);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the path.resolve() itself could be replaced by fixtures.path('keys', fname).

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it looks like the file() wrapper is only used by read(), which is only being used for keys. The read('agent1-key.pem') calls below can probably be replaced directly with fixtures.readKey('agent1-key.pem').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@UnrememberMe
Remove intermediate calls and use fixtures.readKey directly.
0437fd4 
As per review feedback, there is already a function in common.fixture
that handles the logic of reading key files. Switch to use that.
@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 https Issues or PRs related to the https subsystem. label Oct 6, 2017
lpinca
lpinca reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-https-strict.js
@@ -21,6 +21,7 @@

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

@lpinca lpinca Oct 9, 2017 •
edited
Loading

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?

@UnrememberMe
Copy link
Contributor Author

I will need someone else to commit this PR for me since I don't have the right to push master.

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

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

@UnrememberMe this PR should get landed in the next couple of days (at most). We have over 200 new PRs from the Code and Learn, and we're working through them. If it's not landed in the next few days feel free to ping us.

gireeshpunathil
gireeshpunathil approved these changes Oct 10, 2017
View reviewed changes
Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

You seem to be making use of fixtures.readKey, not fixtures.fixturesDir so the PR title may be changed to that effect?

@UnrememberMe UnrememberMe changed the title test: switch to use common.fixtures.fixturesDir test: switch to use common.fixtures.readKey Oct 12, 2017
jasnell pushed a commit that referenced this pull request Oct 13, 2017
@UnrememberMe @jasnell
test: switch to use common.fixtures.fixturesDir
4e2e15f 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: #15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Landed in 4e2e15f

@jasnell jasnell closed this Oct 13, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@UnrememberMe @addaleax
test: switch to use common.fixtures.fixturesDir
97dc389 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: nodejs/node#15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@UnrememberMe @targos
test: switch to use common.fixtures.fixturesDir
61b0af0 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: #15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@UnrememberMe @MylesBorins
test: switch to use common.fixtures.fixturesDir
af4ecae 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: #15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@UnrememberMe @MylesBorins
test: switch to use common.fixtures.fixturesDir
c7070bb 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: #15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@UnrememberMe @MylesBorins
test: switch to use common.fixtures.fixturesDir
9eac5aa 
code and learn 2017 first task is to switch from common.fixturesDir to
common.fixtures.fixturesDir in test-https-strict.js

PR-URL: #15814
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmg rmg rmg left review comments

@lpinca lpinca lpinca left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

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