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: refactor common.PORT to random port #13848

Closed
wants to merge 1 commit into from
Closed

test: refactor common.PORT to random port #13848

wants to merge 1 commit into from

Conversation

guylil
Copy link

@guylil guylil commented Jun 21, 2017 •
edited
Loading

Addresses issue discussed in #12376

Test changed: test/parallel/test-tls-client-default-ciphers.js

This is my first contribution! #goodnessSquad

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 Jun 21, 2017
aqrln
aqrln previously approved these changes Jun 21, 2017
View reviewed changes
Copy link
Contributor

@aqrln aqrln left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks. Can you please amend your commit message so that it says common.PORT instead of common.Port, and the full commit message elaborates on what specific test was changed? Also, the reference to the issue should adhere to the Refs: https://github.com/nodejs/node/issues/12376 format. It can be fixed on landing as well, though.

@aqrln aqrln added the tls Issues and PRs related to the tls subsystem. label Jun 21, 2017
@aqrln
Copy link
Contributor

aqrln commented Jun 21, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8780/

@guylil guylil changed the title test: refactor common.Port to random port test: refactor common.PORT to random port Jun 21, 2017
@guylil
test: refactor common.PORT to random port
1b75126 
Addresses issue discussed in #12376
test amended:	 test/parallel/test-tls-client-default-ciphers.js
		test/parallel/test-tls-connect.js
PR-URL: #13847
Reviewed-By:
@guylil
Copy link
Author

guylil commented Jun 21, 2017

Hi,
Thanks

I did the changes you asked and amended another test

I reviewed all the testes needed changes on this issue

and I think all of them fixed already, except those two

Cheers

benjamingr
benjamingr approved these changes Jun 21, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Nice work. Please feel free to also give us any feedback about the onboarding process today.

#goodnessSquad

aqrln
aqrln reviewed Jun 21, 2017
View reviewed changes
test/parallel/test-tls-connect.js
@@ -37,7 +37,7 @@ const path = require('path');
const cert = fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem'));
const key = fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem'));

const options = {cert: cert, key: key, port: common.PORT};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this should be changed, it may change the reason of the connection failure.

test/parallel/test-tls-connect.js
@@ -51,7 +51,7 @@ const path = require('path');
const conn = tls.connect({
cert: cert,
key: key,
port: common.PORT,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@benjamingr benjamingr mentioned this pull request Jun 22, 2017
Closed
lpinca
lpinca reviewed Jun 22, 2017
View reviewed changes
test/parallel/test-tls-client-default-ciphers.js
@@ -40,7 +40,7 @@ function test1() {
};

try {
tls.connect(common.PORT);
tls.connect(0);
Copy link
Member

Choose a reason for hiding this comment

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

Does it actually make sense to connect to port 0? Binding to port 0 allocates a free port but connecting to port 0 is not allowed in many programs. I guess because it is an invalid port so I'm not sure if it's ok here.

@aqrln aqrln dismissed their stale review June 22, 2017 22:04

It might be better to just leave these tests as they are and move them to test/sequential.

Trott
Trott requested changes Jun 23, 2017
View reviewed changes
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.

Connecting to port 0 gives EADDRNOTAVAIL while connecting to a valid-but-not-listening port gives ECONNREFUSED. Moreover, using port 0 for connecting (as opposed to listening) is a bit nonsensical semantically. Thanks for the pull request, but this needs a different approach. (Easiest thing to do might be to simply move both tests as they currently exist to test/sequential instead of test/parallel. They can always be moved back to test/parallel if someone finds a different solution.)

@refack
Copy link
Contributor

refack commented Jun 23, 2017

Easiest thing to do might be to simply move both tests as they currently exist to test/sequential instead of test/parallel. They can always be moved back to test/parallel if someone finds a different solution.

👍

@Trott
Copy link
Member

Trott commented Aug 10, 2017 •
edited
Loading

Hi, @guylil! What do you say we try to get this thing across the finish line? Here's my recommendation:

  • Remove the changes to test-tls-connect.js.
  • Rebase against current master.
  • In test-tls-client-default-ciphers.js, instead of changing common.PORT to 0, change it to a string representing a path. /foo/bar is a perfectly good choice. (I sometimes go for Homestar Runner references, so maybe /sterrance or /cannonmouth. Or whatever you want, for the most part. It doesn't matter because it's going to get ignored in the test anyway. So just as long as it's clear that it's just a placeholder.)
  • Run make test to be certain tests pass and lint rules are being followed.
  • Push it up to your branch.
  • Leave a comment here along the lines of "Hey, I updated everything, please take another look."

Hope you're up for it! And if not, thanks for the pull request anyway!

@Trott
Copy link
Member

Trott commented Aug 16, 2017

Pull request is against unknown repository. Looks like this isn't going to happen. Closing. Thanks for the PR anyway, and if I'm wrong about any of this and this should be re-opened or something, feel free to comment!

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

@aqrln aqrln aqrln left review comments

@lpinca lpinca lpinca left review comments

@Trott Trott Trott requested changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
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.
7 participants
@guylil @aqrln @refack @Trott @benjamingr @lpinca @nodejs-github-bot