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: test-crypto-keygen.js should be split into multiple tests to avoid timeout in CI #49202

Closed
joyeecheung opened this issue Aug 16, 2023 · 8 comments
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Aug 16, 2023

It looks like #41206 is coming back to the CI, see https://ci.nodejs.org/job/node-test-binary-windows-js-suites/22596/RUN_SUBSET=2,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/parallel/test_crypto_keygen_/ It's a test with 1800+ lines that is crammed with computation-intensive crypto tests that seem to be logically separable. On my local machine which is quite powerful it still takes >5s to run. It would be good to split it into multiple tests to avoid timing out in the CI (I could do it if no one picks this up, but I feel like maybe people from @nodejs/crypto have better ideas about how to split them properly and give the split tests proper names - if I am doing it I would probably just name them test-crypto-keygen-1.js, test-crypto-keygen-2.js..)

@joyeecheung joyeecheung added the good first issue Issues that are suitable for first-time contributors. label Aug 16, 2023
@pluris
Copy link
Contributor

pluris commented Aug 17, 2023

If I want to reproduce it, can I run it with the command below?

python.exe tools/test.py -J --repeat=1000 test/parallel/test-crypto-keygen.js

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 17, 2023

You can just run it with out/Release/node test/parallel/test-crypto-keygen.js and see how long it takes and try to split it to make the split tests run faster, the time out only reproduces on very slow machines, running it many times would not make a difference if you run it on a fast machine.

@joyeecheung
Copy link
Member Author

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/22633/RUN_SUBSET=2,nodes=win2016-COMPILED_BY-vs2022-x86/console

And test-crypto-dh.js too (which crams 19 crypto.createDiffieHellman() calls in one test

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 17, 2023

I am going to do it to deflake the CI sooner than later.

@joyeecheung joyeecheung removed the good first issue Issues that are suitable for first-time contributors. label Aug 17, 2023
@pluris
Copy link
Contributor

pluris commented Aug 17, 2023

I was checking to give it a try, but you've already fixed it. That's a good job. 👍

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 18, 2023

ok 57 parallel/test-crypto-secure-heap
  ---
  duration_ms: 1981.18900
  ...
ok 58 parallel/test-crypto-dh-constructor
  ---
  duration_ms: 4528.98300
  ...
ok 59 parallel/test-crypto-classes
  ---
  duration_ms: 5376.81000
  ...
ok 60 parallel/test-crypto-dh
  ---
  duration_ms: 9867.30400
  ...
ok 61 parallel/test-crypto-keygen
  ---
  duration_ms: 9152.01800

Here is a list of crypto tests that took more than 1s for me locally (probably would be much slower in the CI on slow machines, in the failures referenced above, over 2 minutes)

@VoltrexKeyva VoltrexKeyva added test Issues and PRs related to the tests. crypto Issues and PRs related to the crypto subsystem. labels Aug 22, 2023
nodejs-github-bot pushed a commit that referenced this issue Aug 31, 2023
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 issue Aug 31, 2023
nodejs-github-bot pushed a commit that referenced this issue Aug 31, 2023
nodejs-github-bot pushed a commit that referenced this issue Sep 7, 2023
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: #49492
Refs: #49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
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 issue Sep 10, 2023
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
ruyadorno pushed a commit that referenced this issue Sep 28, 2023
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: #49492
Refs: #49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
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]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: nodejs#49492
Refs: nodejs#49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Nov 15, 2023
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: #49492
Refs: #49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Nov 27, 2023
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 issue Nov 27, 2023
targos pushed a commit that referenced this issue Nov 27, 2023
@shnooshnoo
Copy link

hi @joyeecheung seems like this was fixed and can be closed now, can it?

@joyeecheung
Copy link
Member Author

I think only the two largest tests from #49202 (comment) have been split, but then I think so far I haven't seen the other less large ones timing out in the CI, so maybe it's good for now. We can split some more if any of them time out again.

sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: nodejs/node#49492
Refs: nodejs/node#49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
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 issue Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: nodejs/node#49492
Refs: nodejs/node#49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
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 issue Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants