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.

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

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

Merged
merged 0 commits into from
Oct 15, 2018

Conversation

sam-github
Copy link
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 nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 11, 2018
@sam-github
Copy link
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 refack added the crypto Issues and PRs related to the crypto subsystem. label Oct 11, 2018
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

@nodejs/crypto might want to weigh in here.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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

@richardlau
Copy link
Member

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


testEncryptDecrypt(publicKey, privateKey);
testSignVerify(publicKey, privateKey);
})).catch(common.mustNotCall());
}))
Copy link
Member

Choose a reason for hiding this comment

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

Lint: missing semicolon.

@Trott
Copy link
Member

Trott commented Oct 13, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2018
@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

sam-github commented Oct 13, 2018

  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
Copy link
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
Copy link
Member

Trott commented Oct 14, 2018

@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 sam-github force-pushed the fix-flaky-test-crypto-keygen branch from 72800aa to 4bba44d Compare October 15, 2018 18:15
@sam-github
Copy link
Contributor Author

Landed in 561e30d

@sam-github sam-github merged commit 561e30d into nodejs:master Oct 15, 2018
@sam-github sam-github force-pushed the fix-flaky-test-crypto-keygen branch from 4bba44d to 561e30d Compare October 15, 2018 18:20
@sam-github 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
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 added a commit to AdamMajer/node that referenced this pull request Oct 18, 2018
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
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
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
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
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
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 codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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
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
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 codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-crypto-keygen
10 participants