GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test,crypto: adjust tests for BoringSSL by codebytere · Pull Request #61459 · nodejs/node

Conversation

@codebytere

@codebytere codebytere commented Jan 21, 2026

Copy link
Copy Markdown
Member

Ongoing effort to make crypto tests work with BoringSSL.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Jan 21, 2026
@codebytere
codebytere marked this pull request as ready for review January 21, 2026 10:38
@codebytere
codebytere force-pushed the bssl-test-tweaks branch from f659ab0 to e21d796 Compare January 21, 2026 11:01
@codecov

codecov Bot commented Jan 21, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (da5843b) to head (d27e019).
⚠️ Report is 332 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61459      +/-   ##
==========================================
- Coverage   89.67%   89.65%   -0.02%     
==========================================
  Files         676      676              
  Lines      206469   206469              
  Branches    39537    39542       +5     
==========================================
- Hits       185157   185117      -40     
- Misses      13448    13491      +43     
+ Partials     7864     7861       -3     

see 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@codebytere
codebytere force-pushed the bssl-test-tweaks branch from e21d796 to efdaebc Compare January 21, 2026 12:07
addaleax
addaleax reviewed Jan 21, 2026
View reviewed changes
Comment thread test/parallel/test-tls-pfx-authorizationerror.js Outdated
assert.strictEqual(client.getCertificate().serialNumber,
'147D36C1C2F74206DE9FAB5F2226D78ADB00A426');
assert.match(client.getCertificate().serialNumber,
/147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i);

@addaleax addaleax Jan 21, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

^ Do you know what's causing these? This is a difference in the public API that I would not expect.

This seems like the kind of thing where we should consistently return an uppercase string from the API, because it would be reasonable for user code to rely on that particular behavior. This should do the trick:

diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc
index 11622d823bbc..f40f6dd4f2bc 100644
--- a/src/crypto/crypto_x509.cc
+++ b/src/crypto/crypto_x509.cc
@@ -253,8 +253,8 @@ MaybeLocal<Value> GetSignatureAlgorithmOID(Environment* env,
 
 MaybeLocal<Value> GetSerialNumber(Environment* env, const X509View& view) {
   if (auto serial = view.getSerialNumber()) {
-    return OneByteString(env->isolate(),
-                         static_cast<unsigned char*>(serial.get()));
+    return ToV8Value(
+        env, ToUpper(std::string_view(static_cast<char*>(serial.get()))));
   }
   return Undefined(env->isolate());
 }

If there's more to this and we really, really cannot do that for some reason, these regexps should be anchored, i.e.

Suggested change
/147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i);
/^147D36C1C2F74206DE9FAB5F2226D78ADB00A426$/i);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Jan 21, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(and actually, maybe this is something we should handle at all sites where we call BN_bn2hex()...)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@codebytere codebytere Jan 27, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Given that'd be a slightly larger undertaking and not just a test change should i break that into a new PR?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Jan 27, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If you want to go with the diff above, I think that would be a smaller change overall 🙂

I personally don't have a strong preference for a new PR or keeping this one, I'm happy to review either way

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Feb 9, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@codebytere Fyi, I've opened a separate PR with that suggestion: #61752

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codebytere reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread test/parallel/test-crypto-key-objects.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

panva
panva reviewed Jan 27, 2026
View reviewed changes
Comment thread test/parallel/test-crypto-key-objects-to-crypto-key.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

addaleax added a commit to addaleax/node that referenced this pull request Feb 9, 2026
@addaleax
crypto: always return certificate serial numbers as uppercase
c2c8497
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: nodejs#61459 (comment)
@addaleax addaleax mentioned this pull request Feb 9, 2026
crypto: always return certificate serial numbers as uppercase #61752
Closed
addaleax added a commit that referenced this pull request Feb 12, 2026
@addaleax
crypto: always return certificate serial numbers as uppercase
4a13a62
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this pull request Feb 19, 2026
@addaleax @aduh95
crypto: always return certificate serial numbers as uppercase
938841e
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@codebytere
codebytere requested review from addaleax and panva February 25, 2026 15:17
@codebytere

codebytere commented Mar 13, 2026

Copy link
Copy Markdown
Member Author

@addaleax @panva is there anything else I need to do here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 reviewed Mar 13, 2026
View reviewed changes
Comment thread test/parallel/test-crypto-authenticated.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@codebytere
codebytere force-pushed the bssl-test-tweaks branch 3 times, most recently from b7d8764 to b8ec581 Compare March 13, 2026 11:33
@codebytere
codebytere requested a review from aduh95 March 13, 2026 11:34
@codebytere
test,crypto: adjust tests for BoringSSL
d27e019
@codebytere
codebytere force-pushed the bssl-test-tweaks branch from b8ec581 to d27e019 Compare March 13, 2026 11:34
aduh95 pushed a commit that referenced this pull request Apr 4, 2026
@addaleax @aduh95
crypto: always return certificate serial numbers as uppercase
52d171b
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 5, 2026
@addaleax @aduh95
crypto: always return certificate serial numbers as uppercase
255ea8f
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 6, 2026
@addaleax @aduh95
crypto: always return certificate serial numbers as uppercase
87a40d8
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 8, 2026
@addaleax @aduh95
crypto: always return certificate serial numbers as uppercase
72aad8b
This hides a discrepancy between OpenSSL and BoringSSL, as
the latter returns lowercase hex values.

Refs: #61459 (comment)
PR-URL: #61752
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@panva

panva commented Apr 24, 2026

Copy link
Copy Markdown
Member

Superseded by #62883 using a recent BoringSSL snapshot.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@panva panva closed this Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva Awaiting requested review from panva
@addaleax addaleax Awaiting requested review from addaleax
@aduh95 aduh95 Awaiting requested review from aduh95

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@codebytere @panva @addaleax @aduh95 @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.