test: fix test test-tls-dhe for OpenSSL32 #54903

mhdawson · 2024-09-12T13:12:06Z

OpenSSL32 has a minimum dh key size by 2048 by default.
Adjust test to use larger 3072 key instead of 1024 when OpenSSL32 is present.

Refs: nodejs#53382 - OpenSSL32 has a minimum dh key size by 2048 by default. - Adjust test to use larger 3072 key instead of 1024 when OpenSSL32 is present. Signed-off-by: Michael Dawson <midawson@redhat.com>

mhdawson · 2024-09-12T13:14:21Z

CI run on OpenSSL32 - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/42/

richardlau · 2024-09-12T14:08:46Z

test/parallel/test-tls-dhe.js

+  if (!common.hasOpenSSL(3, 2)) {
+    await testCustomParam(1024, dheCipher);
+  } else {
+    await testCustomParam(3072, dheCipher);


Do we need to test 3072-bit keys? We're already testing 2048-bit keys on the line below. And if we do need to test 3072-bit keys, that could be with all version of OpenSSL?

We tested with two different keys before so I kept it that way, we could add 3072 for all versions but that might add to runtime. I think what is in the PR is my first choice but I don't feel to strongly one way or the other.

I think testing the 3072-bit key will only slow down the test, but I also have no strong opinion.

codecov · 2024-09-12T14:48:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.05%. Comparing base (9db6327) to head (722f460).
Report is 23 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54903   +/-   ##
=======================================
  Coverage   88.05%   88.05%           
=======================================
  Files         651      651           
  Lines      183405   183405           
  Branches    35822    35823    +1     
=======================================
  Hits       161499   161499           
+ Misses      15159    15158    -1     
- Partials     6747     6748    +1

see 24 files with indirect coverage changes

Signed-off-by: Michael Dawson <midawson@redhat.com>

richardlau · 2024-09-12T15:24:38Z

CI run on OpenSSL32 - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/42/

Test has failed:
https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/42/nodes=ubuntu2204_sharedlibs_openssl32_x64/testReport/junit/(root)/parallel/test_tls_dhe/

duration_ms: 428.137
exitcode: 1
severity: fail
stack: |-
  Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
      at mustCall (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/common/index.js:451:10)
      at _expectWarning (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/common/index.js:690:10)
      at Proxy.expectWarning (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/common/index.js:726:31)
      at Object.<anonymous> (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/parallel/test-tls-dhe.js:47:8)
      at Module._compile (node:internal/modules/cjs/loader:1546:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
      at Module.load (node:internal/modules/cjs/loader:1317:32)
      at Module._load (node:internal/modules/cjs/loader:1127:12)
      at TracingChannel.traceSync (node:diagnostics_channel:315:14)
...

mhdawson · 2024-09-12T15:26:55Z

another run on openssl after pushing an update to address one thing I'd missed - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/44/

mhdawson · 2024-09-12T15:50:38Z

@richardlau, test passes on OpenSSL32 after the update.

nodejs-github-bot · 2024-09-12T21:57:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62377/ 💛

nodejs-github-bot · 2024-09-14T13:25:26Z

Landed in c4f2954

Refs: #53382 - OpenSSL32 has a minimum dh key size by 2048 by default. - Adjust test to use larger 3072 key instead of 1024 when OpenSSL32 is present. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: #54903 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test: fix test test-tls-dhe for OpenSSL32

6fcb989

Refs: nodejs#53382 - OpenSSL32 has a minimum dh key size by 2048 by default. - Adjust test to use larger 3072 key instead of 1024 when OpenSSL32 is present. Signed-off-by: Michael Dawson <midawson@redhat.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 12, 2024

lpinca approved these changes Sep 12, 2024

View reviewed changes

richardlau reviewed Sep 12, 2024

View reviewed changes

squash: fixup missed error

722f460

Signed-off-by: Michael Dawson <midawson@redhat.com>

lpinca approved these changes Sep 12, 2024

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024

richardlau approved these changes Sep 12, 2024

View reviewed changes

richardlau added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024

This comment was marked as outdated.

Sign in to view

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2024

This comment was marked as outdated.

Sign in to view

jasnell approved these changes Sep 13, 2024

View reviewed changes

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 14, 2024

nodejs-github-bot merged commit c4f2954 into nodejs:main Sep 14, 2024
62 checks passed

richardlau mentioned this pull request Sep 14, 2024

Test failures with OpenSSL 3.2.2 #53382

Open

26 tasks

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test test-tls-dhe for OpenSSL32 #54903

test: fix test test-tls-dhe for OpenSSL32 #54903

mhdawson commented Sep 12, 2024

mhdawson commented Sep 12, 2024 •

edited

Loading

richardlau Sep 12, 2024

mhdawson Sep 12, 2024

lpinca Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

richardlau commented Sep 12, 2024

mhdawson commented Sep 12, 2024

mhdawson commented Sep 12, 2024

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 12, 2024 •

edited by jasnell

Loading

nodejs-github-bot commented Sep 14, 2024

test: fix test test-tls-dhe for OpenSSL32 #54903

test: fix test test-tls-dhe for OpenSSL32 #54903

Conversation

mhdawson commented Sep 12, 2024

mhdawson commented Sep 12, 2024 • edited Loading

richardlau Sep 12, 2024

Choose a reason for hiding this comment

mhdawson Sep 12, 2024

Choose a reason for hiding this comment

lpinca Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

richardlau commented Sep 12, 2024

mhdawson commented Sep 12, 2024

mhdawson commented Sep 12, 2024

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 12, 2024 • edited by jasnell Loading

nodejs-github-bot commented Sep 14, 2024

mhdawson commented Sep 12, 2024 •

edited

Loading

lpinca Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

nodejs-github-bot commented Sep 12, 2024 •

edited by jasnell

Loading