FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: more regression tests for minDHSize option by bnoordhuis · Pull Request #3629 · nodejs/node · GitHub

/ node Public

test: more regression tests for minDHSize option - #3629

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:more-mindhsize-tests
Nov 3, 2015
ConversationCommits (2)ChecksFiles changed
Merged

test: more regression tests for minDHSize option#3629
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:more-mindhsize-tests

Conversation

bnoordhuis commented Nov 2, 2015

Copy link
Copy Markdown
Member

bnoordhuis added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Nov 2, 2015
thefourtheye reviewed Nov 2, 2015
View reviewed changes
Comment thread test/parallel/test-tls-client-mindhsize.js Outdated

thefourtheye Nov 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since we are doing this, can we include Infinity and NaN?

bnoordhuis Nov 2, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I added tests for negative infinity and nan. I left out positive infinity because I'm not even sure what it would mean to want keys of infinite size.

Fishrock123 Nov 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I left out positive infinity because I'm not even sure what it would mean to want keys of infinite size.

Infinite quantum entropy?

bnoordhuis force-pushed the more-mindhsize-tests branch from 8f66027 to 752e19b Compare November 2, 2015 19:15

bnoordhuis commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

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

thefourtheye commented Nov 3, 2015

Copy link
Copy Markdown
Contributor

LGTM

test: add regression test for 512 bits DH key
cddf358 
Check that trying to use a < 1024 bits DH key throws an exception.

parallel/test-tls-dhe tests this as well but it feels incongruous not to
do it here when both tests have similar logic for 1024/2048 bits keys.

PR-URL: nodejs#3629
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
test: more regression tests for minDHSize option
82022a7 
Check that tls.connect() fails in the expected way when passing in
invalid minDHSize options.

PR-URL: nodejs#3629
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
bnoordhuis force-pushed the more-mindhsize-tests branch from 752e19b to 82022a7 Compare November 3, 2015 10:48
bnoordhuis closed this Nov 3, 2015
bnoordhuis deleted the more-mindhsize-tests branch November 3, 2015 10:48
bnoordhuis merged commit 82022a7 into nodejs:master Nov 3, 2015
Fishrock123 mentioned this pull request Nov 6, 2015
Closed
bnoordhuis added a commit that referenced this pull request Nov 7, 2015
test: add regression test for 512 bits DH key
935b977 
Check that trying to use a < 1024 bits DH key throws an exception.

parallel/test-tls-dhe tests this as well but it feels incongruous not to
do it here when both tests have similar logic for 1024/2048 bits keys.

PR-URL: #3629
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
bnoordhuis added a commit that referenced this pull request Nov 7, 2015
test: more regression tests for minDHSize option
8152248 
Check that tls.connect() fails in the expected way when passing in
invalid minDHSize options.

PR-URL: #3629
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Fishrock123 mentioned this pull request Nov 11, 2015
Merged

MylesBorins commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

This should not land if f72e178 from #1831 does not land

rvagg mentioned this pull request Dec 17, 2015
Closed

jasnell commented Feb 17, 2016

Copy link
Copy Markdown
Member

@nodejs/lts ... please weigh in on this one... this LGTM for v4.4

MylesBorins commented Feb 17, 2016

Copy link
Copy Markdown
Contributor

it is worth noting that this relies on a semver major change from #1831

rvagg commented Feb 18, 2016

Copy link
Copy Markdown
Member

yeah, this is not for v4.x cause minDHSize is a v5 thing, updating labels

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL