test: split test-crypto-keygen.js#49221
Uh oh!
There was an error while loading. Please reload this page.
Conversation
To avoid timing out on ARM machines in the CI.
nodejs-github-bot added needs-ci 
github-actionsbot removed the request-ci 
This comment was marked as outdated.
This comment was marked as outdated.
Added some additional changes other than copy-pasting, locally before the changes: After: This is on a relatively fast machine (it's on ARM, I had to comment out the SKIP in |
I think the length changes in the last commit should not affect the effectiveness of the tests, but cc @nodejs/crypto to be sure |
github-actionsbot removed the request-ci nodejs-github-bot commented Aug 18, 2023
aduh95 commented Aug 19, 2023
Can we run a stress test to find out? |
joyeecheung commented Aug 22, 2023
Let's see if the stress test job understands wildcard.. https://ci.nodejs.org/job/node-stress-single-test/419/ |
joyeecheung commented Aug 23, 2023
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. |
github-actionsbot removed the request-ci nodejs-github-bot commented Aug 24, 2023
nodejs-github-bot commented Aug 24, 2023
panva added the author ready nodejs-github-bot commented Aug 27, 2023
panva removed the author ready 
tniessen left a comment
tniessen left a comment There was a problem hiding this comment.
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.
| // Tests that a key pair can be used for encryption / decryption. | ||
| functiontestEncryptDecrypt(publicKey,privateKey){ | ||
| constmessage='Hello Node.js world!'; | ||
| constplaintext=Buffer.from(message,'utf8'); | ||
| for(constkeyof[publicKey,privateKey]){ | ||
| constciphertext=publicEncrypt(key,plaintext); | ||
| constreceived=privateDecrypt(privateKey,ciphertext); | ||
| assert.strictEqual(received.toString('utf8'),message); | ||
| } | ||
| } | ||
| // Tests that a key pair can be used for signing / verification. | ||
| functiontestSignVerify(publicKey,privateKey){ | ||
| constmessage=Buffer.from('Hello Node.js world!'); | ||
| functionoldSign(algo,data,key){ | ||
| returncreateSign(algo).update(data).sign(key); | ||
| } | ||
| functionoldVerify(algo,data,key,signature){ | ||
| returncreateVerify(algo).update(data).verify(key,signature); | ||
| } | ||
| for(constsignFnof[sign,oldSign]){ | ||
| constsignature=signFn('SHA256',message,privateKey); | ||
| for(constverifyFnof[verify,oldVerify]){ | ||
| for(constkeyof[publicKey,privateKey]){ | ||
| constokay=verifyFn('SHA256',message,key,signature); | ||
| assert(okay); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
joyeecheung commented Aug 27, 2023
Mind that moving to pummel means usually people do not run them locally (we don't run them in |
joyeecheung added commit-queue nodejs-github-bot removed the commit-queue nodejs-github-bot commented Aug 31, 2023
Landed in a81d5e1...b781eaf |
To avoid timing out on ARM machines in the CI. PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
To avoid timing out on ARM machines in the CI. PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
To avoid timing out on ARM machines in the CI. PR-URL: nodejs#49221 Refs: nodejs#49202 Refs: nodejs#41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#49221 Refs: nodejs#49202 Refs: nodejs#41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#49221 Refs: nodejs#49202 Refs: nodejs#41206 Reviewed-By: Luigi Pinca <[email protected]>
To avoid timing out on ARM machines in the CI. PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #49221 Refs: #49202 Refs: #41206 Reviewed-By: Luigi Pinca <[email protected]>
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]>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <[email protected]>
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]>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <[email protected]>
6 participants
To avoid timing out on slow machines in the CI.
Refs: #49202
Refs: #41206