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

test: use smaller keys for a faster keygen test by sam-github · Pull Request #23430 · nodejs/node · GitHub

/ node Public

test: use smaller keys for a faster keygen test - #23430

Merged
sam-github merged 0 commit into
nodejs:masterfrom
sam-github:fix-flaky-test-crypto-keygen
Oct 15, 2018
ConversationCommits (0)ChecksFiles changed
Merged

test: use smaller keys for a faster keygen test#23430
sam-github merged 0 commit into
nodejs:masterfrom
sam-github:fix-flaky-test-crypto-keygen

Conversation

sam-github commented Oct 11, 2018

Copy link
Copy Markdown
Contributor

On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

Fix #23406

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot commented Oct 11, 2018

Copy link
Copy Markdown
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1206/pipeline

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

sam-github commented Oct 11, 2018

Copy link
Copy Markdown
Contributor Author

I'm not sure if I need to worry about the keysizes being too small to be allowed when FIPs is enabled, which the example @bnoordhuis pointed to in #23406 (comment) did.

refack added the crypto Issues and PRs related to the crypto subsystem. label Oct 11, 2018
thefourtheye approved these changes Oct 12, 2018
View reviewed changes

thefourtheye left a comment

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

@nodejs/crypto might want to weigh in here.

tniessen approved these changes Oct 12, 2018
View reviewed changes

tniessen left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

We can take care of FIPS later once we support it.

richardlau commented Oct 12, 2018

Copy link
Copy Markdown
Member

Defensively marking dont-land-on-v8.x as there are concerns re. FIPS.

bnoordhuis approved these changes Oct 12, 2018
View reviewed changes
Comment thread test/parallel/test-crypto-keygen.js Outdated

bnoordhuis Oct 12, 2018

Copy link
Copy Markdown
Member

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

Lint: missing semicolon.

Trott commented Oct 13, 2018

Copy link
Copy Markdown
Member

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

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 13, 2018

sam-github commented Oct 13, 2018

Copy link
Copy Markdown
Contributor Author

saw an unrelated failure, resuming: https://ci.nodejs.org/job/node-test-pull-request/17823/

sam-github commented Oct 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author
  1. ci: https://ci.nodejs.org/job/node-test-pull-request/17830/
  2. ci: https://ci.nodejs.org/job/node-test-pull-request/17834/

Because this is supposed to be less flaky I'd like to cause a series of ARM builds.

sam-github commented Oct 14, 2018

Copy link
Copy Markdown
Contributor Author

@Trott is this better? windows seems to be unstable, but I don't know enough about what is expected to know if this PR is making things more or less stable.

It intention is to fix #23406, so what do you think?

Trott commented Oct 14, 2018

Copy link
Copy Markdown
Member

@sam-github Failures on Windows are in other tests that are pre-existing known-unreliable tests. (Or am I wrong about that and this is making the test that you modified fail on Windows and I'm just missing that fact somewhere?) So I think you can land this. We typically do a stress test on things like this, but I consider the fact that it brings the run time down by approximately a factor of 10 to be sufficient grounds to land this even if it didn't fix the unreliable test.

sam-github force-pushed the fix-flaky-test-crypto-keygen branch from 72800aa to 4bba44d Compare October 15, 2018 18:15

sam-github commented Oct 15, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 561e30d

sam-github merged commit 561e30d into nodejs:master Oct 15, 2018
sam-github force-pushed the fix-flaky-test-crypto-keygen branch from 4bba44d to 561e30d Compare October 15, 2018 18:20
sam-github deleted the fix-flaky-test-crypto-keygen branch October 16, 2018 17:05
jasnell pushed a commit that referenced this pull request Oct 17, 2018
test: use smaller keys for a faster keygen test
eb48f28 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: #23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
AdamMajer mentioned this pull request Oct 18, 2018
Closed
3 tasks
AdamMajer added a commit to AdamMajer/node that referenced this pull request Oct 18, 2018
test: fix invalid modulesLength for DSA keygen
478e360 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

Refs: nodejs#23430
addaleax pushed a commit that referenced this pull request Oct 20, 2018
test: use smaller keys for a faster keygen test
bf2e6c7 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: #23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danbev pushed a commit that referenced this pull request Oct 24, 2018
test: fix invalid modulesLength for DSA keygen
a88ca52 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Oct 24, 2018
test: fix invalid modulesLength for DSA keygen
1521d89 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
test: use smaller keys for a faster keygen test
f82611a 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: #23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
test: fix invalid modulesLength for DSA keygen
79be83d 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
test: use smaller keys for a faster keygen test
90cf5fc 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: #23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
test: fix invalid modulesLength for DSA keygen
cb229e0 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
test: use smaller keys for a faster keygen test
4daf8e0 
On my machine, this brings test execution time down from about 2
seconds to 0.2 seconds.

PR-URL: #23430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
test: fix invalid modulesLength for DSA keygen
1747e47 
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere mentioned this pull request Nov 29, 2018
Merged
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

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
+2 more reviewers
refack refack approved these changes
thefourtheye thefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Investigate flaky parallel/test-crypto-keygen

10 participants

Back | FazBrowse Home | New Git URL