GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: split test-crypto-keygen.js by joyeecheung · Pull Request #49221 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Aug 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

To avoid timing out on slow machines in the CI.

Refs: #49202
Refs: #41206

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pluris reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@joyeecheung
test: split test-crypto-keygen.js
0066016 
To avoid timing out on ARM machines in the CI.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Aug 17, 2023
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
joyeecheung added 2 commits August 18, 2023 01:53
@joyeecheung
test: split JWK async elliptic curve keygen tests
e34f08e
@joyeecheung
test: reduce length in crypto keygen tests
d637fce
@joyeecheung

joyeecheung commented Aug 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Added some additional changes other than copy-pasting, locally before the changes:

❯ tools/test.py  'test/parallel/test-crypto-keyge*'
[00:09|% 100|+   2|-   0]: Done

After:

❯ tools/test.py  'test/parallel/test-crypto-keyge*'
[00:01|% 100|+  34|-   0]: Done

This is on a relatively fast machine (it's on ARM, I had to comment out the SKIP in test/parallel/parallel.status locally - they work fine for me though, maybe it's okay to unskip now? cc @tniessen Also they were marked flaky on Windows because they were timing out, maybe it's okay to remove the flaky mark if the split tests are fast enough), on slower machines in the CI there could be a bit more differences
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Aug 18, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I think the length changes in the last commit should not affect the effectiveness of the tests, but cc @nodejs/crypto to be sure
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 18, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 18, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53428/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Aug 19, 2023

Copy link
Copy Markdown
Contributor

Also they were marked flaky on Windows because they were timing out, maybe it's okay to remove the flaky mark if the split tests are fast enough

Can we run a stress test to find out?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Aug 21, 2023
View reviewed changes

@lpinca lpinca left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

RSLGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot mentioned this pull request Aug 22, 2023
CI Reliability 2023-08-22 nodejs/reliability#642
Open
21 tasks
@joyeecheung

joyeecheung commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

Let's see if the stress test job understands wildcard.. https://ci.nodejs.org/job/node-stress-single-test/419/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 23, 2023
CI Reliability 2023-08-23 nodejs/reliability#643
Open
27 tasks
@joyeecheung

joyeecheung commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

Welp, it looks like the stress test job does not understand wildcards, let's see if we can pass all the tests into it... https://ci.nodejs.org/job/node-stress-single-test/421/ if it still does not work, I guess we can only rely on the full CI to check.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 24, 2023
CI Reliability 2023-08-24 nodejs/reliability#644
Open
27 tasks
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53543/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53547/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva panva added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 27, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53579/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva panva removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 27, 2023
tniessen
tniessen reviewed Aug 27, 2023
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't have a strong opinion on this. Admittedly, it's a long and slow test, but moving it to pummel or increasing timeouts seems reasonable to me as well. I've said many times that, in my opinion, (short) timeouts cause more problems than they solve.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/common/crypto.js
Comment on lines +65 to +97
// Tests that a key pair can be used for encryption / decryption.
function testEncryptDecrypt(publicKey, privateKey) {
const message = 'Hello Node.js world!';
const plaintext = Buffer.from(message, 'utf8');
for (const key of [publicKey, privateKey]) {
const ciphertext = publicEncrypt(key, plaintext);
const received = privateDecrypt(privateKey, ciphertext);
assert.strictEqual(received.toString('utf8'), message);
}
}

// Tests that a key pair can be used for signing / verification.
function testSignVerify(publicKey, privateKey) {
const message = Buffer.from('Hello Node.js world!');

function oldSign(algo, data, key) {
return createSign(algo).update(data).sign(key);
}

function oldVerify(algo, data, key, signature) {
return createVerify(algo).update(data).verify(key, signature);
}

for (const signFn of [sign, oldSign]) {
const signature = signFn('SHA256', message, privateKey);
for (const verifyFn of [verify, oldVerify]) {
for (const key of [publicKey, privateKey]) {
const okay = verifyFn('SHA256', message, key, signature);
assert(okay);
}
}
}
}

@tniessen tniessen Aug 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

These functions have very generic names but very specific implementations. For example, testEncryptDecrypt works with certain asymmetric key pairs only, whereas most encryption and decryption operations in cryptography rely on symmetric keys.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Aug 27, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you have a suggestions for the names? i think generally it's fine to have very generic names in the test commons - that's what we've been doing for many tests utils too. The point is just sharing code among tests. The were named that way in one long test test anyway, so keeping the name doesn't make much of a difference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tniessen tniessen Aug 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The were named that way in one long test test anyway, so keeping the name doesn't make much of a difference.

Yes, in a test file that exclusively used asymmetric encryption. But moving them into common, these functions become available to all tests.

I don't want to make this more work for you than it has to be, so I am fine with leaving the names as they are.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Aug 27, 2023

Copy link
Copy Markdown
Member Author

moving it to pummel or increasing timeouts seems reasonable to me as well

Mind that moving to pummel means usually people do not run them locally (we don't run them in make test). Increasing timeouts in the test runner for all tests isn't a great idea IMO - two minutes are already a lot. If a test doesn't finish in two minutes, either it's actually failing for some reason, or something should be done to them or the test suites would just take too long to complete. Unless there's a way to introduce per-test timeouts in the test runner.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 28, 2023
CI Reliability 2023-08-28 nodejs/reliability#648
Open
25 tasks
This was referenced Aug 29, 2023
CI Reliability 2023-08-29 nodejs/reliability#649
Open
CI Reliability 2023-08-30 nodejs/reliability#650
Open
@joyeecheung joyeecheung added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. labels Aug 31, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 31, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 31, 2023

Copy link
Copy Markdown
Collaborator

Landed in a81d5e1...b781eaf
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot closed this Aug 31, 2023
nodejs-github-bot pushed a commit that referenced this pull request Aug 31, 2023
@joyeecheung @nodejs-github-bot
test: split test-crypto-keygen.js
7224940 
To avoid timing out on ARM machines in the CI.

PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 31, 2023
@joyeecheung @nodejs-github-bot
test: split JWK async elliptic curve keygen tests
b9a6ab5 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 31, 2023
@joyeecheung @nodejs-github-bot
test: reduce length in crypto keygen tests
b781eaf 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
test: split test-crypto-keygen.js
e04a260 
To avoid timing out on ARM machines in the CI.

PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
test: split JWK async elliptic curve keygen tests
4faa30c 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
test: reduce length in crypto keygen tests
6ed47bd 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@joyeecheung @targos
test: split test-crypto-keygen.js
344592d 
To avoid timing out on ARM machines in the CI.

PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
@joyeecheung @targos
test: split JWK async elliptic curve keygen tests
958e114 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
@joyeecheung @targos
test: reduce length in crypto keygen tests
67cb6f9 
PR-URL: #49221
Refs: #49202
Refs: #41206
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: split test-crypto-keygen.js
6ebbd6c 
To avoid timing out on ARM machines in the CI.

PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: split JWK async elliptic curve keygen tests
2aa755e 
PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: reduce length in crypto keygen tests
dc50f7f 
PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: split test-crypto-keygen.js
580db80 
To avoid timing out on ARM machines in the CI.

PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: split JWK async elliptic curve keygen tests
2447b77 
PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: reduce length in crypto keygen tests
2bfb541 
PR-URL: nodejs/node#49221
Refs: nodejs/node#49202
Refs: nodejs/node#41206
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@lpinca lpinca lpinca approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@joyeecheung @nodejs-github-bot @aduh95 @lpinca @tniessen @panva
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.