test: split test-crypto-keygen.js - #49221
Conversation
Sorry, something went wrong.
To avoid timing out on ARM machines in the CI.
This comment was marked as outdated.
|
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 |
Sorry, something went wrong.
|
I think the length changes in the last commit should not affect the effectiveness of the tests, but cc @nodejs/crypto to be sure |
Sorry, something went wrong.
Sorry, something went wrong.
Can we run a stress test to find out? |
Sorry, something went wrong.
lpinca
left a comment
There was a problem hiding this comment.
RSLGTM
Sorry, something went wrong.
|
Let's see if the stress test job understands wildcard.. https://ci.nodejs.org/job/node-stress-single-test/419/ |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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.
Sorry, something went wrong.
| // 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); | ||
| } | ||
| } | ||
| } | ||
| } |
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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 <luigipinca@gmail.com>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <luigipinca@gmail.com>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#49221 Refs: nodejs/node#49202 Refs: nodejs/node#41206 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
To avoid timing out on slow machines in the CI.
Refs: #49202
Refs: #41206