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

sam-github · 2018-10-11T22:39:07Z

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

Checklist

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

nodejs-github-bot · 2018-10-11T22:39:09Z

@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

sam-github · 2018-10-11T22:41:30Z

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.

thefourtheye

@nodejs/crypto might want to weigh in here.

tniessen

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

richardlau · 2018-10-12T00:24:25Z

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

bnoordhuis · 2018-10-12T08:59:23Z

test/parallel/test-crypto-keygen.js

Lint: missing semicolon.

Trott · 2018-10-13T13:57:11Z

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

sam-github · 2018-10-13T18:51:42Z

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

sam-github · 2018-10-13T20:58:21Z

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

sam-github · 2018-10-14T00:00:24Z

@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 · 2018-10-14T00:19:19Z

@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 · 2018-10-15T18:19:50Z

Landed in 561e30d

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>

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

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>

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>

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>

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>

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>

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>

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>

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>

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

refack approved these changes Oct 11, 2018

View reviewed changes

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

tniessen approved these changes Oct 12, 2018

View reviewed changes

richardlau added the dont-land-on-v8.x label Oct 12, 2018

jasnell approved these changes Oct 12, 2018

View reviewed changes

bnoordhuis approved these changes Oct 12, 2018

View reviewed changes

test/parallel/test-crypto-keygen.js Outdated

Copy link

Member

bnoordhuis Oct 12, 2018

Choose a reason for hiding this comment

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

Lint: missing semicolon.

cjihrig approved these changes Oct 12, 2018

View reviewed changes

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

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

AdamMajer mentioned this pull request Oct 18, 2018

test: fix invalid modulesLength during DSA keygen #23732

Closed

3 tasks

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

sam-github commented Oct 11, 2018

Uh oh!

nodejs-github-bot commented Oct 11, 2018

Uh oh!

sam-github commented Oct 11, 2018

Uh oh!

thefourtheye left a comment

Uh oh!

tniessen left a comment •

edited

Loading

Uh oh!

richardlau commented Oct 12, 2018

Uh oh!

bnoordhuis Oct 12, 2018

Uh oh!

Trott commented Oct 13, 2018

Uh oh!

sam-github commented Oct 13, 2018

Uh oh!

sam-github commented Oct 13, 2018 •

edited

Loading

Uh oh!

sam-github commented Oct 14, 2018

Uh oh!

Trott commented Oct 14, 2018

Uh oh!

sam-github commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

sam-github commented Oct 11, 2018

Checklist

Uh oh!

nodejs-github-bot commented Oct 11, 2018

Uh oh!

sam-github commented Oct 11, 2018

Uh oh!

thefourtheye left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Oct 12, 2018

Uh oh!

bnoordhuis Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 13, 2018

Uh oh!

sam-github commented Oct 13, 2018

Uh oh!

sam-github commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Oct 14, 2018

Uh oh!

Trott commented Oct 14, 2018

Uh oh!

sam-github commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tniessen left a comment •

edited

Loading

sam-github commented Oct 13, 2018 •

edited

Loading