test: common.fixturesDir --> common.fixtures by camantigue · Pull Request #15976 · nodejs/node · GitHub

Replaced common.fixturesDir with usage of common.fixtures module in
test/parallel/test-tls-delayed-atach.js.

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

Replaced `common.fixturesDir` with usage of `common.fixtures` module in `test/parallel/test-tls-delayed-atach.js`. Checklist [*] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes

This could be using method fixtures.readKey instead to get the keys.

This needs a rebase and a force push before this can be landed (there are merge commits in here). LGTM otherwise.

Nit - it would be nice not to have any unrelated changes and if this could be changed back again.

Hi @camantigue — are you still interested in pursuing this? It looks like there's a bit of feedback from @BridgeAR & @pawelgolda that needs to be addressed. Let us know if you need any pointers :)

Hi @apapirovski - I'm still interested in updating the changes. Is it just re-doing it without the merge commits? Or is it also preferred to use fixtures.readKey for line 37?

@camantigue Ideally we would update to use fixtures.readKey like so fixtures.readKey('agent1-key.pem'). Also if you could remove the extra line break @BridgeAR mentioned, that would be appreciated.

Utilized the fixtures module's readKey function instead of readSync and removed an unnecessary line of space.

This actually doesn't need /keys/ because the readKey already knows where to look.

If you're having trouble rebasing this, let us know. Since some of these merges are quite far in the past you might need to go quite far in history for your git rebase -i .

Hi @apapirovski - How can I go about rebasing this? I don't think I've had experience doing that. Making the changes for the /keys/ right now.

Removed `/keys/` from the key file path string as the readKey function handles that for you.

First, you need to add the upstream remote, fetch our current master and checkout your branch. Skip this section if you already did this:

git remote add upstream https://github.com/nodejs/node.git
git fetch upstream master
git checkout change-test-parallel-delayed-attach-fixtures

To rebase, do this:

git rebase upstream/master

If you run into problems while rebasing due to the merge commits, you can also cherry-pick your changes:

git reset --hard upstream/master
git cherry-pick 778fbdd071829fc245df7388fa5edc47828a4489
git cherry-pick 0414bced86075a461a63c4c942add3a1d9ede76a
git cherry-pick 15579d4b30a78b97c791ae1d85d5643c9cdcb2f3
git cherry-pick c21f0b23da3788633cf7aede0a49e29d5c11ba6f

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

Landed in c4c6381, thank you for the contribution!

PR-URL: #15976 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Hi @camantigue, the email that you used when authoring the commit is not added to your github account, so you are not promoted to "Contributor" after the commit landed. If you want the promotion, please add your email to your github account (the commit email is Paul-Marion.F.Camantigue@kp.org)

Hi @joyeecheung this account (@camantigue) has my e-mail set as paulcamantigue@gmail.com and when I try to add the e-mail to the @paulcamantigue-kaiser account it states that my e-mail is already in use, which makes sense. How can I fix this? Hopefully won't have to put a dummy e-mail in this account and replace the Paul-Marion.F.Camantigue@kp.org with my paulcamantigue@gmail.com.

@camantigue I think you can put a dummy email in @paulcamantigue-kaiser and add Paul-Marion.F.Camantigue@kp.org to @camantigue? Assuming you don't really use @paulcamantigue-kaiser anymore.

Hey @joyeecheung I just changed the e-mail and it looks like the icons changed to my @camantigue account (I read your comment in #16360 haha). Let me know if anything else is needed for the promotion.

Let me know if anything else is needed for the promotion.

@camantigue looks like GitHub noticed, and updated accordingly, congratulations on becoming a contributor to Node.js!

PR-URL: #15976 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs/node#15976 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: #15976 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs/node#15976 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

paulcamantigue-kaiser and others added 4 commits January 23, 2017 19:35

Merge remote-tracking branch 'nodejs/master'

11515ec

Merge remote-tracking branch 'nodejs/master'

6fe647a

Merge remote-tracking branch 'nodejs/master'

7feb0db

test: common.fixturesDir --> common.fixtures

778fbdd

Replaced `common.fixturesDir` with usage of `common.fixtures` module in `test/parallel/test-tls-delayed-atach.js`. Checklist [*] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017

pawelgolda reviewed Oct 6, 2017

View reviewed changes

mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017

BridgeAR approved these changes Oct 7, 2017

View reviewed changes

BridgeAR reviewed Oct 7, 2017

View reviewed changes

camantigue added 2 commits October 20, 2017 07:31

Update test-tls-delayed-attach.js

0414bce

Used Fixtures's readKey instead of readSync

15579d4

Utilized the fixtures module's readKey function instead of readSync and removed an unnecessary line of space.

apapirovski reviewed Oct 20, 2017

View reviewed changes

Removed /keys/ for key file path

c21f0b2

Removed `/keys/` from the key file path string as the readKey function handles that for you.

jasnell approved these changes Oct 20, 2017

View reviewed changes

tniessen approved these changes Oct 20, 2017

View reviewed changes

tniessen self-assigned this Oct 20, 2017

gireeshpunathil approved these changes Oct 21, 2017

View reviewed changes

joyeecheung closed this Oct 21, 2017

joyeecheung mentioned this pull request Oct 21, 2017

meta: note re author check in COLLABORATOR_GUIDE #16360

Closed

2 tasks

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

               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'),

               const options = {
-                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'),

		if (!common.hasCrypto)
		common.skip('missing crypto');

test: common.fixturesDir --> common.fixtures by camantigue · Pull Request #15976 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: common.fixturesDir --> common.fixtures - #15976

test: common.fixturesDir --> common.fixtures#15976camantigue wants to merge 7 commits intonodejs:masternodejs/node:masterfrom camantigue:change-test-parallel-delayed-attach-fixturescamantigue/node:change-test-parallel-delayed-attach-fixturesCopy head branch name to clipboard

Conversation

camantigue commented Oct 6, 2017 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pawelgolda Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Oct 19, 2017

Uh oh!

camantigue commented Oct 20, 2017

Uh oh!

apapirovski commented Oct 20, 2017

Uh oh!

apapirovski Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camantigue commented Oct 20, 2017

Uh oh!

tniessen commented Oct 20, 2017

Uh oh!

apapirovski commented Oct 20, 2017

Uh oh!

joyeecheung commented Oct 21, 2017

Uh oh!

joyeecheung commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camantigue commented Oct 21, 2017

Uh oh!

joyeecheung commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camantigue commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Oct 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

test: common.fixturesDir --> common.fixtures#15976
camantigue wants to merge 7 commits into
nodejs:masterfrom
camantigue:change-test-parallel-delayed-attach-fixtures

camantigue commented Oct 6, 2017 •
edited by BridgeAR
Loading

apapirovski commented Oct 20, 2017 •
edited
Loading

joyeecheung commented Oct 21, 2017 •
edited
Loading

joyeecheung commented Oct 21, 2017 •
edited
Loading

camantigue commented Oct 21, 2017 •
edited
Loading