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.

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

Replace common.fixtiresDir with readFixtureKey #16817

Closed
wants to merge 1 commit into from

Conversation

wsierakowski
Copy link
Contributor

Replaced common.fixturesDir with the readFixtureKey from the fixtures module.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 6, 2017

Typo in commit message and unnecessary merge commit.

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 6, 2017
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readFixtureKey('agent1-key.pem'),
cert: fixtures.readFixtureKey('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.

these can be:

key: fixtures.readSync('agent1-key.pem'),
cert: fixtures.readSync('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.

@gireeshpunathil (and for @wsierakowski's benefit too): I think you mean readKey() rather than readSync(). (Aside: API should have probably been readKeySync() rather than readKey() but whatever. :-D )

Copy link
Member

Choose a reason for hiding this comment

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

@Trott - yes, thanks for the correction!

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi, @wsierakowski! Welcome and thank you for the PR! readFixtureKey() is not a function in fixtures. This test with this change will not run. Can you change that function name to readKey() instead? You can run the test from the command line with tools/test.py test/parallel/test-tls-js-stream.js to make sure it works. After commiting the change to your branch, pushing it to your GitHub repo will update this PR with the new code.

(You don't need to worry about removing the merge commit mentioned by mscdex because we will squash that out when landing your code. But if you get rid of it, that's fine too.)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I went ahead and made the requested changes myself. Hope that's OK.

@Trott
Copy link
Member

Trott commented Nov 8, 2017

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: nodejs#16817
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 8, 2017

Landed in fa8aee8.
Thanks for the contribution! 🎉

@Trott Trott closed this Nov 8, 2017
@wsierakowski
Copy link
Contributor Author

wsierakowski commented Nov 8, 2017

Thanks for fixing this @Trott, sorry for not doing this earlier by myself, I got too involved with conference-related activities in the meantime :)

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: #16817
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: #16817
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: #16817
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: #16817
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Use fixtures module in test/parallel/test-tls-js-stream.js.

PR-URL: #16817
Reviewed-By: Rich Trott <[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
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants