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

CommonFixturesFix - Fixed references to common.fixturesDir in test-tl… #16041

Closed
wants to merge 5 commits into from
Closed

CommonFixturesFix - Fixed references to common.fixturesDir in test-tl… #16041

wants to merge 5 commits into from

Conversation

timothywisdom
Copy link

…s-client-getephemeralkeyinfo.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
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
BridgeAR
BridgeAR approved these changes Oct 6, 2017
View reviewed changes
test/parallel/test-tls-client-getephemeralkeyinfo.js Outdated
@@ -1,20 +1,21 @@
'use strict';
const common = require('../common');
const commonFixturesDir = require('../common.fixtures').fixturesDir;
Copy link
Member

Choose a reason for hiding this comment

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

Optional - it would be nice to use destructuring here as in

const { fixturesDir } = require('../common.fixtures').fixturesDir;

@BridgeAR
Copy link
Member

BridgeAR commented Oct 6, 2017

The commit message does not contain the subsystem and it is not listed in the description either but that can be fixed while landing. The commit message may also only be 50 characters long.

@mscdex mscdex added the tls Issues and PRs related to the tls 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 self-assigned this Oct 9, 2017
BridgeAR
BridgeAR reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-tls-client-getephemeralkeyinfo.js Outdated
const key = fs.readFileSync(`${common.fixturesDir}/keys/agent2-key.pem`);
const cert = fs.readFileSync(`${common.fixturesDir}/keys/agent2-cert.pem`);
const key = fs.readFileSync(`${commonFixturesDir}/keys/agent2-key.pem`);
const cert = fs.readFileSync(`${commonFixturesDir}/keys/agent2-cert.pem`);
Copy link
Member

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

This should actually use the fixtures.readKey function. I fix this while landing.

Copy link
Member

Choose a reason for hiding this comment

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

@timothywisdom it would actually be nice if you could fix this test.

@BridgeAR BridgeAR removed their assignment Oct 9, 2017
jasnell
jasnell reviewed Oct 12, 2017
View reviewed changes
test/parallel/test-tls-client-getephemeralkeyinfo.js Outdated
if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const tls = require('tls');
const fs = require('fs');

const key = fs.readFileSync(`${common.fixturesDir}/keys/agent2-key.pem`);
const cert = fs.readFileSync(`${common.fixturesDir}/keys/agent2-cert.pem`);
const fixturesKey = fixtures.readKey('/agent2-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.

the path does not need the leading /

@claudiorodriguez
Copy link
Contributor

claudiorodriguez commented Oct 13, 2017 •
edited
Loading

Can you rebase the 5 commits into a single one?
Also, change tests: <...> for test: <...>

@timothywisdom
Copy link
Author

I've been told by many a git guru to avoid git rebase as it rewrites history, which is generally a bad idea. Also I believe this pull request, if merged, will merge all the commits, not just the first. So what's the argument for rebasing?

@joyeecheung
Copy link
Member

joyeecheung commented Oct 14, 2017 •
edited
Loading

@timothywisdom In this repo we don't use the Github merge button, nor do we use git merge, and we rewrite history of PRs all the time as long as it has not been merged into master. See the collaborator guide if you want to know how we land commits into master branch.

Also I believe this pull request, if merged, will merge all the commits, not just the first

As the collaborator guide describes, this PR will be rebased against master and squashed into one commit by the person landing it anyway.

The downside of rewriting history usually applies when you are rewriting history of a branch that other people collaborate on, but PR happens in branches of forks, not the master branch.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 14, 2017 •
edited
Loading

Also, this PR needs a rebase since #15980 has landed

jasnell
jasnell reviewed Oct 15, 2017
View reviewed changes
test/parallel/test-tls-client-getephemeralkeyinfo.js
const key = fs.readFileSync(`${common.fixturesDir}/keys/agent2-key.pem`);
const cert = fs.readFileSync(`${common.fixturesDir}/keys/agent2-cert.pem`);
const fixturesKey = fixtures.readKey('agent2-key.pem');
const fixturesCert = fixtures.readKey('agent2-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.

there shouldn't be reason to change the variable names here.

jasnell
jasnell reviewed Oct 15, 2017
View reviewed changes
test/parallel/test-tls-client-getephemeralkeyinfo.js
key: key,
cert: cert,
key: fixturesKey,
cert: fixturesCert,
Copy link
Member

@jasnell jasnell Oct 15, 2017 •
edited by BridgeAR
Loading

Choose a reason for hiding this comment

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

if the key and cert variable names are not changed above, this can become just ...

const options = { key, cert, ciphers: cipher };

@BridgeAR BridgeAR assigned BridgeAR and unassigned BridgeAR Oct 18, 2017
@BridgeAR
Copy link
Member

@timothywisdom this contains conflicts right now. Would you be so kind and rebase?

@timothywisdom
Copy link
Author

I had to change

const key
const cert

to

const fixturesKey
const fixturesCert

due to some error I was getting when running the tests (I don't remember what it was now). I've been swept away into another project and I don't have time to work on this right now. Setting up this repo and running the tests on this project (on my windows machine) takes too many hours. If someone else would like to take over this PR and make whatever changes you'd like, please be my guest.

@Trott
Copy link
Member

Trott commented Oct 28, 2017

It looks like the fixture stuff was fixed in #15980. The 521 is in fact correct. It's not 512. Strange, I know.

Seems like this can be closed.

Sorry to hear about your Windows compilation problems. FWIW, we're trying to figure out a better solution for Windows over in #16278.

If you'd like a different good first issue, feel free to contact help@nodetodo.org.

Thanks for the PR! Sorry it didn't end up landing.

@Trott Trott closed this Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell left review comments

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@timothywisdom @BridgeAR @claudiorodriguez @joyeecheung @Trott @jasnell @mscdex @nodejs-github-bot