Sitelet https://github.com/nodejs/node/issues/49202
Skip to content

test: test-crypto-keygen.js should be split into multiple tests to avoid timeout in CI #49202

Description

@joyeecheung

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..)

Activity

  1. pluris commented on Aug 17, 2023

    @pluris
    Contributor

    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
  2. joyeecheung commented on Aug 17, 2023

    @joyeecheung
    MemberAuthor

    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.

  3. joyeecheung commented on Aug 17, 2023

    @joyeecheung
    MemberAuthor

    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

  4. joyeecheung commented on Aug 17, 2023

    @joyeecheung
    MemberAuthor

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

  5. pluris commented on Aug 17, 2023

    @pluris
    Contributor

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

  6. joyeecheung commented on Aug 18, 2023

    @joyeecheung
    MemberAuthor
    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)

  7. added
    testIssues and PRs related to Node.js core tests and test infrastructure.
    cryptoIssues and PRs related to the crypto subsystem.
    on Aug 22, 2023
  8. added a commit that references this issue on Sep 10, 2023
  9. 7 remaining items

  10. shnooshnoo commented on Nov 28, 2023

    @shnooshnoo

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

  11. joyeecheung commented on Nov 28, 2023

    @joyeecheung
    MemberAuthor

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    cryptoIssues and PRs related to the crypto subsystem.testIssues and PRs related to Node.js core tests and test infrastructure.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions