nodejs: skip some TLS tests on 20.x and 18.x

[aduh95]

Addresses #343421 (comment).

It looks like some tests are only passing on 22.x branch atm, I wasn't able to pin point any obvious commit that address this issue, I guess it's fine to skip those tests for now to unblock the build.

[aduh95]

(Having just say that, I realize that it might be nodejs/node@01f751b that I somehow missed. I can't really test that hypothesis as there are simply too much stuff to rebuild, once the caches have been populated I'll try to see if the tests pass with that commit applied)

[fabianhjr]

once the caches have been populated I'll try to see if the tests pass with that commit applied

afaik that change will make every dependent rebuild anyways. If these tests are disabled I would push for them to only be re-enabled until the next staging-next cycle to avoid wasting resources/compute doing 5k rebuilds per platform. (Including big packages like web engines)

Edit: Misunderstood the cache the other way around, leaving as draft while the dependencies of nodejs are built to avoid this being merged before you have a chance to test the potential patch.

[fabianhjr]

In a test run on my local system the following were fixed by that patch:

       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-client-renegotiation-13.js
       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-client-verify.js
       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-multiple-cas-as-string.js
       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-peer-certificate-encoding.js
       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-sni-server-client.js

The following would still be pending:

       > out/Release/node /build/node-v20.17.0/test/parallel/test-tls-multi-key.js

[fabianhjr]

Only tested building nodejs_20, nodejs_18 might still need other patches or disabling of tests

[aduh95]

I've forced pushed to have the patches listed in the "correct" order, which will simplify updating Node.js versions when future releases will pick up those patches.

[aduh95]

nodejs_18 might still need other patches or disabling of tests

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

Lines 261 to 267 in 6030d78

	] ++ lib.optionals (!stdenv.buildPlatform.isDarwin || lib.versionAtLeast version "20") [
	# There are some test failures on macOS before v20 that are not worth the
	# time to debug for a version that would be eventually removed in less
	# than a year (Node.js 18 will be EOL at 2025-04-30). Note that these
	# failures are specific to Nix sandbox on macOS and should not affect
	# actual functionality.
	"test-ci-js"

v18.x is fine actually, no test runs -> no test failures :D EDIT: I read it wrong, it's only disabled on macOS.

[emilazy]

Do we want to just vendor this patch since we’re applying so many anyway? It’ll never be merged into 18.x or 20.x, presumably.

[aduh95]

It’ll never be merged into 18.x or 20.x, presumably.

No reason it wouldn't. For v20.x, all (non-semver-minor) patches are typically always backported after a maturation period until it reaches maintenance (next month). For v18.x, I've opened nodejs/node#55101 and plan to include it as well – having passing tests with OpenSSL 3.2 is useful not just for the Nix project.
It will definitely land on v22.x anyway, which is also affected.

Do we want to just vendor this patch since we’re applying so many anyway?

I'm not against it, but also I don't think it's necessary, it seems fine to keep the test on that list while we wait for the fix to land upstream. The minimum wait time is 48 hours for that kind of PRs in the Node.js project, so depending on whether this PR has landed, I'll either update it or I'll open a new one targeting staging once that happens.

[emilazy]

Ah, okay; I didn’t realize the LTS branches got so much attention :)

It should be okay to just roll it into this PR once merged. We just threw away all the builds to revert the GCC bump, so it’s going to be quite a few days before eating a Node.js rebuild becomes unpalatable; if you’re okay waiting for the PR to be merged then I’m fine merging this after that.