Skip to content

assert: allow printf-style messages as assertion error#58849

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
BridgeAR:BridgeAR/2025-06-26-add-format-string-to-assert-calls
Oct 17, 2025
Merged

assert: allow printf-style messages as assertion error#58849
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
BridgeAR:BridgeAR/2025-06-26-add-format-string-to-assert-calls

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Jun 26, 2025

Copy link
Copy Markdown
Member

Also add functions as allowed message input.
This allows to have heavy message computation to become cheaper.

This is not yet finished, I just thought I could get some early feedback.
Tests are often not as performance sensitive, so it's not that crucial.
I do like the ergonomics of it as well though and I know I worked around
a test perf issue of this kind in Node.js before.

image

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 26, 2025
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Jun 26, 2025
avivkeller
avivkeller reviewed Jun 27, 2025
View reviewed changes
Comment thread lib/assert.js Outdated
@BridgeAR BridgeAR force-pushed the BridgeAR/2025-06-26-add-format-string-to-assert-calls branch from f6aeff1 to ae6214e Compare September 11, 2025 22:59
@BridgeAR
assert: allow printf-style messages as assertion error
591a6e3 
Also add functions as allowed message input.
This allows to have leavy message computation to become cheaper.
@BridgeAR
fixup!
32e43c2
@BridgeAR BridgeAR force-pushed the BridgeAR/2025-06-26-add-format-string-to-assert-calls branch from ae6214e to 32e43c2 Compare October 6, 2025 12:12
@BridgeAR BridgeAR marked this pull request as ready for review October 6, 2025 12:24
@codecov

codecov Bot commented Oct 6, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.62162% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (7c85aa5) to head (67d2b7b).
⚠️ Report is 76 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/assert/utils.js 95.57% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58849      +/-   ##
==========================================
- Coverage   88.52%   88.52%   -0.01%     
==========================================
  Files         703      703              
  Lines      207825   207894      +69     
  Branches    40003    40016      +13     
==========================================
+ Hits       183976   184035      +59     
- Misses      15862    15867       +5     
- Partials     7987     7992       +5
Files with missing lines Coverage Δ
lib/assert.js 98.34% <100.00%> (-0.02%) ⬇️
lib/internal/test_runner/test.js 97.36% <100.00%> (ø)
lib/internal/assert/utils.js 97.26% <95.57%> (-2.74%) ⬇️

... and 37 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.

@BridgeAR BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 7, 2025
@BridgeAR BridgeAR requested review from avivkeller and marco-ippolito and removed request for avivkeller October 7, 2025 08:13
@BridgeAR

BridgeAR commented Oct 10, 2025

Copy link
Copy Markdown
Member Author

@nodejs/assert @marco-ippolito PTAL

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2025
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2025

Copy link
Copy Markdown
Collaborator

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

@BridgeAR

BridgeAR commented Oct 16, 2025

Copy link
Copy Markdown
Member Author

@nodejs/tsc PTAL, due to this being semver-major

@targos

targos commented Oct 16, 2025

Copy link
Copy Markdown
Member

So the breaking change is that existing calls that happen to have printf syntax in the message give a different output?

@BridgeAR

BridgeAR commented Oct 17, 2025

Copy link
Copy Markdown
Member Author

@targos for simple assert, using symbols would now throw. That is to make it consistent across the other methods and I think that should be fine to remove.

See https://github.com/nodejs/node/pull/58849/files#diff-968e57a9f4d4ce7494523f71b2594d844620c2794a68049cce33fcdc8c7c7b7cL907-L910

targos
targos approved these changes Oct 17, 2025
View reviewed changes

@targos targos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Breaking change LGTM

@BridgeAR BridgeAR 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 Oct 17, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2025
@nodejs-github-bot nodejs-github-bot merged commit d3f79aa into nodejs:main Oct 17, 2025
@nodejs-github-bot

nodejs-github-bot commented Oct 17, 2025

Copy link
Copy Markdown
Collaborator

Landed in d3f79aa

@Flarna Flarna mentioned this pull request Oct 17, 2025
Merged
@richardlau richardlau mentioned this pull request Oct 18, 2025
Closed
@joyeecheung

joyeecheung commented Oct 18, 2025

Copy link
Copy Markdown
Member

This is breaking the linter on the main branch https://ci.nodejs.org/job/node-test-linter/62229/

@richardlau

richardlau commented Oct 18, 2025

Copy link
Copy Markdown
Member

This is breaking the linter on the main branch https://ci.nodejs.org/job/node-test-linter/62229/

#60304 should fix the linter.

@RafaelGSS RafaelGSS mentioned this pull request Apr 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avivkeller avivkeller avivkeller left review comments

@targos targos targos approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@BridgeAR @nodejs-github-bot @targos @joyeecheung @richardlau @RafaelGSS @marco-ippolito @avivkeller