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

nodejs: run JS test suite as part of the checks #313982

Merged
merged 1 commit into from
May 31, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 23, 2024

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:
  • 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
Copy link
Member

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.

@aduh95 aduh95 changed the base branch from master to staging May 23, 2024 15:23
@lheckemann
Copy link
Member

@ofborg build nodejs_18 nodejs_20 nodejs_22

@lheckemann
Copy link
Member

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

Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, will merge if the builds pass

@lheckemann
Copy link
Member

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?

@lheckemann
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented May 31, 2024

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

@lheckemann lheckemann merged commit 7c63164 into NixOS:staging May 31, 2024
24 checks passed
@tie tie mentioned this pull request Jun 3, 2024
12 tasks
@alyssais
Copy link
Member

This has broken nodejs_18 and nodejs_22.

@aduh95 aduh95 deleted the nodejs-jstest branch June 17, 2024 12:35
@aduh95
Copy link
Contributor Author

aduh95 commented Jun 17, 2024

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

@alyssais
Copy link
Member

alyssais commented Jun 18, 2024 via email

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 18, 2024

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

@alyssais
Copy link
Member

nodejs_22.log

@alyssais
Copy link
Member

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

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 19, 2024

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
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Jun 19, 2024

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
Copy link
Member

@lheckemann wdyt?

@lheckemann
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Jun 20, 2024

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:

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
Copy link
Member

lheckemann commented Jun 20, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@aduh95 aduh95 mentioned this pull request Jun 21, 2024
13 tasks
@aduh95
Copy link
Contributor Author

aduh95 commented Jun 21, 2024

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
Copy link
Member

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
Copy link
Member

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.

emilazy added a commit to emilazy/nixpkgs that referenced this pull request Jul 9, 2024
This was recently enabled in NixOS#313982. It seems to be much too flaky
on Darwin currently, especially x86; see:

* <https://hydra.nixos.org/build/264860513/nixlog/8>
* <https://hydra.nixos.org/build/264956149/nixlog/3>
* <https://hydra.nixos.org/build/265094929/nixlog/3>
* <https://hydra.nixos.org/build/264901296/nixlog/3>

Disable these tests on macOS for now as the broken Node.js package
is holding up a lot of builds.

Fixes: b26563a
@vcunat
Copy link
Member

vcunat commented Jul 9, 2024

Basically reverting this on darwin:
#325844

aduh95 pushed a commit to aduh95/nixpkgs that referenced this pull request Jul 11, 2024
This was recently enabled in NixOS#313982. It seems to be much too flaky
on Darwin currently, especially x86; see:

* <https://hydra.nixos.org/build/264860513/nixlog/8>
* <https://hydra.nixos.org/build/264956149/nixlog/3>
* <https://hydra.nixos.org/build/265094929/nixlog/3>
* <https://hydra.nixos.org/build/264901296/nixlog/3>

Disable these tests on macOS for now as the broken Node.js package
is holding up a lot of builds.

Fixes: b26563a
(cherry picked from commit d25d9b6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants