nodejs: run JS test suite as part of the checks

[aduh95]

Description of changes

I noticed the JS test suite of Node.js wasn't run as part of the check phase, so I spent some time making it work. I've opened nodejs/node#53105 and nodejs/node#53104 to land the fixes upstream.

I've only the changes with nix-build -A nodejs-slim_20. I think there are some improvements that could be made (e.g. currently the list of skip tests is shared for all versions, it would probably make more sense to have per-version specific lists instead).

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[lheckemann]

Diff looks good, but this should go to staging rather than straight to master since it causes a lot of rebuilds. See the contributing docs section for how (just changing the target branch will notify a bunch of unrelated people due to github weirdness).

The commit metadata also seems wrong, check your Git user.name and user.email and use git commit --amend --reset-author to fix that.

[lheckemann]

@ofborg build nodejs_18 nodejs_20 nodejs_22

[lheckemann]

Looks like you rebased on staging (as opposed to the merge base of staging) before changing the target branch -- the rebase on staging shouldn't happen until after the target branch change -- but thankfully this didn't result in a huge amount of notifications so I think we can keep this PR rather than opening a new one (sorry to the unrelated requested reviewers!).

[lheckemann]

Looks good, will merge if the builds pass

[lheckemann]

The build seems to be failing on ofborg: https://logs.ofborg.org/?key=nixos/nixpkgs.313982&attempt_id=fdb75382-9baf-43dc-9dfa-990f53fc001f maybe this is an issue with the non-slim build?

[aduh95]

I think this is ready to land. AFAICT the macOS failures are unrelated.

[lheckemann]

Yes, the macOS failures are real but weren't introduced here. Not sure how to parse the first part of your comment though -- ready or not? :)

[aduh95]

Yes sorry, I meant that IMO it's ready to land.

[alyssais]

This has broken nodejs_18 and nodejs_22.

[aduh95]

@alyssais can you give more info regarding what's broken?

[alyssais]

On Mon, Jun 17, 2024 at 05:35:40AM -0700, Antoine du Hamel wrote: @alyssais can you give more info regarding what's broken?

They no longer build on x86_64-linux. (On current staging, nodejs_20 doesn't build either, but that wasn't introduced by this PR, rather by the OpenSSL upgrade, and nodejs has an upstream patch for it.)

[aduh95]

Do you have a link or could you copy the output please?

[alyssais]

nodejs_22.log

[alyssais]

nodejs_18 on b26563a is building for me now, so for that one it either fails intermittently or I made a mistake.

[aduh95]

Hum unfortunately some tests are flaky (it's also a problem for Node.js CI, but unfortunately quite tricky to fix). I'll try to make a patch to automatically re-run failing tests to avoid failing the build just because of a flake.

[alyssais]

If test failures are likely to be due to flakiness, is it worth it for us to run the tests in the first place?

[aduh95]

If the options are:

• run the tests, fail the build if any failure: we got a high confidence on the build, the build itself is flaky.
• run the tests, retry on failure: we got a less high confidence on the build, it's building more reliably.
• don't run the tests: low confidence on the build, it's building as reliably.

IMO the middle point is the best tradeoff, but that's certainly a subjective call.

[alyssais]

@lheckemann wdyt?

[lheckemann]

I'm guessing it's only individual tests that are flaky (and known to be flaky)? I think it would be worth skipping those specifically, or having the tests retry (if they can't be fixed in order not to be flaky). I imagine that would be a welcome improvement upstream as well?

[aduh95]

There's already such system upstream, and this PR is already taking advantage of it and skip the tests that have been marked as flaky upstream:

nixpkgs/pkgs/development/web/nodejs/nodejs.nix

Line 147 in 667f4bf

"FLAKY_TESTS=skip"

However, because the Node.js project has a relatively high bar for marking tests as flaky (the idea being a test marked as flaky has less chance of getting fixed, because flaky tests do not block PRs from merging); however, for NixOS, those flakes can be very annoying, and IMO it's not realistic to try to manage our own list of FLAKY tests, so I think the best course of action would be to allow to retry failing tests – if a test fails once, and succeeds e.g. the 9 following times, it's safe to assume it was just a flake, no?

[lheckemann]

Right, but we don't want to have to restart drv builds every time the tests fail (especially since that will involve building the code again too) and that means manual work in addition to wasted compute and time. And I'd expect tests that aren't marked as flaky... not to be flaky 🙃

[lheckemann]

Update, I don't think these are actually flaky, but failing maybe for OpenSSL-related reasons?

not ok 2937 parallel/test-tls-alpn-server-client
  ---
  duration_ms: 227.04000
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/parallel/test-tls-alpn-server-client.js:195:14)
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/common/index.js:468:15)
        at TLSSocket.emit (node:events:518:28)
        at emitErrorNT (node:internal/streams/destroy:169:8)
        at emitErrorCloseNT (node:internal/streams/destroy:128:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v20.12.2

not ok 1808 parallel/test-http2-server-unknown-protocol
  ---
  duration_ms: 142.85300
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/parallel/test-http2-server-unknown-protocol.js:45:12)
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/common/index.js:468:15)
        at TLSSocket.emit (node:events:518:28)
        at emitErrorNT (node:internal/streams/destroy:169:8)
        at emitErrorCloseNT (node:internal/streams/destroy:128:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v20.12.2

not ok 1713 parallel/test-http2-https-fallback
  ---
  duration_ms: 215.36500
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/parallel/test-http2-https-fallback.js:154:11)
        at TLSSocket.<anonymous> (/build/node-v20.12.2/test/common/index.js:468:15)
        at TLSSocket.emit (node:events:518:28)
        at emitErrorNT (node:internal/streams/destroy:169:8)
        at emitErrorCloseNT (node:internal/streams/destroy:128:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v20.12.2

[lheckemann]

Yep, nodejs/node@14863e8 seems to be the fix for this, so it must have been an unfortunate timing coincidence with the merging of this PR.

[lheckemann]

Asked about backporting the nodejs-side fix: nodejs/node#53373 (comment)

[aduh95]

Asked about backporting the nodejs-side fix: nodejs/node#53373 (comment)

I cherry-picked that commit to the 20.x update PR

Update, I don't think these are actually flaky, but failing maybe for OpenSSL-related reasons?

It's different from the failure reported in #313982 (comment):

     not ok 5 - appends positional arguments
      ---
      duration_ms: 16.503608
      location: '/build/node-v22.2.0/test/parallel/test-node-run.js:57:3'
      failureType: 'testCodeFailure'
      error: |-
        The input did not match the regular expression /Arguments: '--help "hello world test" A B C'/. Input:
        
        ''
        
      code: 'ERR_ASSERTION'
      name: 'AssertionError'

I tried running that test 999 times on my machine for a stress test on the latest main branch of nodejs/node, and got no failures, so either it's a very rare flake, either it was already fixed.

I'd expect tests that aren't marked as flaky... not to be flaky 🙃

It's certainly not an unreasonable expectation :) however it's inevitable there are some flaky tests that are going to slip through, and I guess the question is "are we fine with those flakes affecting us (Nix)?"

we don't want to have to restart drv builds every time the tests fail

That's precisely why I'm suggesting we retry the failing tests to not fail the build if it hits a flaky test (that is, a test flaky in practice even though not marked as flaky for whatever reason).

[alyssais]

Update, I don't think these are actually flaky, but failing maybe for OpenSSL-related reasons?

That's not the test failures we're talking about here. (I mentioned it already in #313982 (comment))

[lheckemann]

Argh, sorry. I'll throw it at a builder a couple of times to see if I can get an idea of how often it happens.

[vcunat]

Basically reverting this on darwin:
#325844