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

camantigue · 2017-10-06T18:00:59Z

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

pawelgolda · 2017-10-06T18:37:22Z

test/parallel/test-tls-delayed-attach.js

 const net = require('net');

 const sent = 'hello world';
 let received = '';

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


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

BridgeAR

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

BridgeAR · 2017-10-07T17:34:21Z

test/parallel/test-tls-delayed-attach.js

@@ -24,17 +24,18 @@ const common = require('../common');
 if (!common.hasCrypto)
  common.skip('missing crypto');

+


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

apapirovski · 2017-10-19T02:46:41Z

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 :)

camantigue · 2017-10-20T14:21:12Z

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?

apapirovski · 2017-10-20T14:28:25Z

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

apapirovski · 2017-10-20T14:44:31Z

test/parallel/test-tls-delayed-attach.js

 const net = require('net');

 const sent = 'hello world';
 let received = '';

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


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

apapirovski · 2017-10-20T14:45:29Z

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 .

camantigue · 2017-10-20T14:50:29Z

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.

tniessen · 2017-10-20T15:37:44Z

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

apapirovski · 2017-10-20T15:40:30Z

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

joyeecheung · 2017-10-21T14:01:20Z

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>

joyeecheung · 2017-10-21T14:32:26Z

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)

camantigue · 2017-10-21T14:52:01Z

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.

joyeecheung · 2017-10-21T15:00:47Z

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

camantigue · 2017-10-21T15:43:05Z

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.

gibfahn · 2017-10-22T12:28:18Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

camantigue commented Oct 6, 2017 •

edited by BridgeAR

Loading

pawelgolda Oct 6, 2017

BridgeAR left a comment

BridgeAR Oct 7, 2017

apapirovski commented Oct 19, 2017

camantigue commented Oct 20, 2017

apapirovski commented Oct 20, 2017

apapirovski Oct 20, 2017

apapirovski commented Oct 20, 2017 •

edited

Loading

camantigue commented Oct 20, 2017

tniessen commented Oct 20, 2017

apapirovski commented Oct 20, 2017

joyeecheung commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 •

edited

Loading

camantigue commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 •

edited

Loading

camantigue commented Oct 21, 2017 •

edited

Loading

gibfahn commented Oct 22, 2017

		@@ -24,17 +24,18 @@ const common = require('../common');
		if (!common.hasCrypto)
		common.skip('missing crypto');

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

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

Conversation

camantigue commented Oct 6, 2017 • edited by BridgeAR Loading

pawelgolda Oct 6, 2017

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Oct 7, 2017

Choose a reason for hiding this comment

apapirovski commented Oct 19, 2017

camantigue commented Oct 20, 2017

apapirovski commented Oct 20, 2017

apapirovski Oct 20, 2017

Choose a reason for hiding this comment

apapirovski commented Oct 20, 2017 • edited Loading

camantigue commented Oct 20, 2017

tniessen commented Oct 20, 2017

apapirovski commented Oct 20, 2017

joyeecheung commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 • edited Loading

camantigue commented Oct 21, 2017

joyeecheung commented Oct 21, 2017 • edited Loading

camantigue commented Oct 21, 2017 • edited Loading

gibfahn commented Oct 22, 2017

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