test: update test to support OpenSSL32 #54968

mhdawson · 2024-09-16T16:33:30Z

This test fails on OpenSSL32 because it complains the key being
used is too short.

It seems to have been missed when the test suite was udpated to have
a Makefile to generate key material as the keys are hard coded in the
test as opposed to being read in from the fixtures/key directory.

Update the test to use keys/certs from the fixtures directory and
to remove newlines at the end of the key and cert to retain the
inteded test.

Signed-off-by: Michael Dawson [email protected]

lpinca · 2024-09-16T16:45:13Z

Both test/fixtures/keys/rsa_private.pem and test/fixtures/keys/rsa_cert.crt have trailing newlines.

codecov · 2024-09-16T17:45:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (c1afd2c) to head (a2373be).
Report is 32 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54968      +/-   ##
==========================================
- Coverage   88.06%   88.06%   -0.01%     
==========================================
  Files         652      652              
  Lines      183545   183545              
  Branches    35866    35865       -1     
==========================================
- Hits       161639   161637       -2     
- Misses      15150    15156       +6     
+ Partials     6756     6752       -4

see 27 files with indirect coverage changes

mhdawson · 2024-09-16T18:07:53Z

@lpinca how do you see that? I'm looking at the files on disk (linux) and I don't see any newlines. I also don't see anything that would indicate that there are newlines in the UI either -> https://github.com/nodejs/node/blob/main/test/fixtures/keys/rsa_private.pem, but maybe I have a setting that would hide that?

richardlau · 2024-09-16T18:21:11Z

$ od -h test/fixtures/keys/rsa_private.pem
0000000 2d2d 2d2d 422d 4745 4e49 5220 4153 5020
...
0003200 5649 5441 2045 454b 2d59 2d2d 2d2d 000a
0003217
$

mhdawson · 2024-09-16T18:54:23Z

ok. my bad I may have been looking for the carriage return instea of the new line. Anyway I'll update to keep the test with larger keys.

Refs: nodejs#53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]>

mhdawson · 2024-09-16T19:18:59Z

@lpinca , @richardlau fixed. Thanks for catching my mistake :)

mhdawson · 2024-09-16T19:22:35Z

Run on OpenSSL32 - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/52/ (PASSED)

lpinca · 2024-09-16T19:45:38Z

test/parallel/test-tls-cert-regression.js

+if (key[key.length - 1] === 0x0A) {
+  key = key.slice(0, key.length - 1);
+}


let i = 0; while (key[key.length - 1 - i] === 0x0a) i++; if (i !== 0) key = key.slice(0, key.length - i);

There might be multiple trailing newlines.

@lpinca added that for both key and cert.

Signed-off-by: Michael Dawson <[email protected]>

nodejs-github-bot · 2024-09-17T13:50:16Z

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

richardlau

Non-blocking suggestion:
Perhaps assert that there is no newline at the end of the array in case someone modifies the test later?

mhdawson · 2024-09-18T14:30:52Z

@richardlau since I have a green CI I'd like to go ahead and land, but will add that in a follow on PR.

nodejs-github-bot · 2024-09-18T16:36:09Z

Landed in f8b7a17

Refs: nodejs#54968 Refs: nodejs#53382 Add additional asserts as suggestd by Richard in: nodejs#54968 Signed-off-by: Michael Dawson <[email protected]>

Refs: #54968 Refs: #53382 Add additional asserts as suggestd by Richard in: #54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: #54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: nodejs#53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: nodejs#54968 Refs: nodejs#53382 Add additional asserts as suggestd by Richard in: nodejs#54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: nodejs#53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: nodejs#54968 Refs: nodejs#53382 Add additional asserts as suggestd by Richard in: nodejs#54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: nodejs#53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: nodejs#54968 Refs: nodejs#53382 Add additional asserts as suggestd by Richard in: nodejs#54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: #53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: #54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: #54968 Refs: #53382 Add additional asserts as suggestd by Richard in: #54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: #54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: nodejs#53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: nodejs#54968 Refs: nodejs#53382 Add additional asserts as suggestd by Richard in: nodejs#54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Refs: #53382 This test fails on OpenSSL32 because it complains the key being used is too short. It seems to have been missed when the test suite was udpated to have a Makefile to generate key material as the keys are hard coded in the test as opposed to being read in from the fixtures/key directory. Update the test to use keys/certs from the fixtures directory and to remove newlines at the end of the key and cert to retain the inteded test. Signed-off-by: Michael Dawson <[email protected]> PR-URL: #54968 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Refs: #54968 Refs: #53382 Add additional asserts as suggestd by Richard in: #54968 Signed-off-by: Michael Dawson <[email protected]> PR-URL: #54997 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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

RedYetiDev added the openssl Issues and PRs related to the OpenSSL dependency. label Sep 16, 2024

mhdawson force-pushed the openssl32-fixes-6 branch from 47b3573 to 0b3b1fe Compare September 16, 2024 18:20

mhdawson force-pushed the openssl32-fixes-6 branch from 0b3b1fe to 68089ee Compare September 16, 2024 19:16

mhdawson force-pushed the openssl32-fixes-6 branch from 68089ee to 99bd1cf Compare September 16, 2024 19:17

mhdawson changed the title ~~test: remove test that is no longer needed~~ test: update test to support OpenSSL32 Sep 16, 2024

lpinca approved these changes Sep 16, 2024

View reviewed changes

lpinca reviewed Sep 16, 2024

View reviewed changes

sqauash: address review comments

a2373be

Signed-off-by: Michael Dawson <[email protected]>

lpinca approved these changes Sep 16, 2024

View reviewed changes

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

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

richardlau approved these changes Sep 17, 2024

View reviewed changes

richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 18, 2024

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

nodejs-github-bot merged commit f8b7a17 into nodejs:main Sep 18, 2024
59 checks passed

mhdawson mentioned this pull request Sep 18, 2024

test: add asserts to validate test assumptions #54997

Closed

richardlau mentioned this pull request Sep 19, 2024

Test failures with OpenSSL 3.2.2 #53382

Open

26 tasks

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

aduh95 mentioned this pull request Nov 7, 2024

2024-11-12, Version 18.20.5 'Hydrogen' (LTS) #55768

Merged

marco-ippolito mentioned this pull request Nov 16, 2024

v20.18.1 proposal #55879

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: update test to support OpenSSL32 #54968

test: update test to support OpenSSL32 #54968

mhdawson commented Sep 16, 2024 •

edited

Loading

lpinca commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

mhdawson commented Sep 16, 2024 •

edited

Loading

richardlau commented Sep 16, 2024

mhdawson commented Sep 16, 2024

mhdawson commented Sep 16, 2024

mhdawson commented Sep 16, 2024 •

edited

Loading

lpinca Sep 16, 2024

mhdawson Sep 16, 2024

nodejs-github-bot commented Sep 17, 2024

richardlau left a comment

mhdawson commented Sep 18, 2024

nodejs-github-bot commented Sep 18, 2024

test: update test to support OpenSSL32 #54968

test: update test to support OpenSSL32 #54968

Conversation

mhdawson commented Sep 16, 2024 • edited Loading

lpinca commented Sep 16, 2024 • edited Loading

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

mhdawson commented Sep 16, 2024 • edited Loading

richardlau commented Sep 16, 2024

mhdawson commented Sep 16, 2024

mhdawson commented Sep 16, 2024

mhdawson commented Sep 16, 2024 • edited Loading

lpinca Sep 16, 2024

Choose a reason for hiding this comment

mhdawson Sep 16, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 17, 2024

richardlau left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 18, 2024

nodejs-github-bot commented Sep 18, 2024

mhdawson commented Sep 16, 2024 •

edited

Loading

lpinca commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

mhdawson commented Sep 16, 2024 •

edited

Loading

mhdawson commented Sep 16, 2024 •

edited

Loading