X Tutup
The Wayback Machine - https://web.archive.org/web/20250814212517/https://github.com/nodejs/node/pull/58253
Skip to content

Changing assert to become a class #58253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Aug 5, 2025

Conversation

miguelmarcondesf
Copy link
Contributor

@miguelmarcondesf miguelmarcondesf commented May 9, 2025

Changing assert to become a class

This PR refactors assert from a method to a dedicated class. This change is motivated by the need for greater flexibility and configurability in assertion behavior.

By turning assert into a class, we will be able to pass options that customize its behavior, such as doing specific checks, how the stack trace will look like, etc.

Checklist

  • Refactor assert into a class structure (old-school pattern).
  • Add diff option to show the full diff in assertion errors.
  • Ensure backward compatibility with existing assert usages.
  • Add new unit tests to cover the class-based behavior.

cc @BridgeAR

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@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 May 9, 2025
@mcollina mcollina requested review from cjihrig, MoLow and BridgeAR May 10, 2025 21:56
@mcollina
Copy link
Member

I'm mildly concerned on the long term maintainability of this, as it essentially rewrites the module - backporting might lead to more churn than needed. You might get less churn by using the "old school" pattern:

function Assert () {
}

Assert.prototype.notEqual = function () {}

With this pattern, indentation would not change and this PR might be easier to review.

This change is motivated by the need for greater flexibility and configurability in assertion behavior.

Can you clarify the need for this?

Apart from that, this would need a CITGM check to verify it doesn't break end users in any way.

@pmarchini
Copy link
Member

Hey @miguelmarcondesf, thanks for the contribution 🚀

My 2 cents on this: while I understand the reasons behind this refactor, if I’m not mistaken, I don’t see any options actually being used in the class.
We're only supporting an empty object for potential future use cases.
I don’t see a clear benefit in terms of cognitive complexity or readability in the refactor itself.

So I’m wondering what the intended usage is for the new options we plan to introduce.

The reason I’m asking is to provide a better context and justify any potential cons of rewriting the module (e.g., portability to other versions), while also providing an overview of what’s expected to be added.

@miguelmarcondesf
Copy link
Contributor Author

Can you clarify the need for this?

@mcollina Thank you for sharing your concerns!
This idea came from a conversation with @BridgeAR when I was looking for something to contribute

@miguelmarcondesf
Copy link
Contributor Author

Hey @pmarchini, thanks for your thoughts!

My 2 cents on this: while I understand the reasons behind this refactor, if I’m not mistaken, I don’t see any options being used in the class.

I decided to open it in draft so I could receive more guidance on this, the PR already had a lot of modifications, and I wanted more perspectives so we could start this discussion.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I just had a glimpse at it so far, while it looks really good!

I definitely believe this is something we want to have!

The reason is that it's almost impossible to configure any algorithm behavior so far. Adding options to the current API is not really a way to go due to the overloading we have.

Using a class would finally allow to e.g., adjust how a diff should be visualized (all changes or cutting it off as it's done right now?), to adjust the algorithm checks in the deep equal comparison (e.g., should the prototype be checked, yes or no?). We definitely have many use cases for it being a class that users may adjust to produce the outcome of their needs that can't properly be addressed with our defaults.

I don’t see a clear benefit in terms of cognitive complexity or readability in the refactor itself.

Good point, @pmarchini!

I myself also try to only implement things when we make use of it.

A major concern has been the diff generated. I think we could add that as first option, since it's quite straight forward and it would address an issue.

#51902

@miguelmarcondesf miguelmarcondesf marked this pull request as ready for review June 9, 2025 23:52
@miguelmarcondesf
Copy link
Contributor Author

I'm mildly concerned on the long term maintainability of this, as it essentially rewrites the module - backporting might lead to more churn than needed. You might get less churn by using the "old school" pattern:

Hey @mcollina thank you for the suggestion! I made the changes to the old-school pattern and if it makes sense, perhaps in future iterations we can migrate the functions to a version using a regular class.

Also, for now first option added as suggested by @BridgeAR was the diff generated.

Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 89.68254% with 13 lines in your changes missing coverage. Please review.

Project coverage is 90.03%. Comparing base (1c4fe6d) to head (04b90b4).
Report is 80 commits behind head on main.

Files with missing lines Patch % Lines
lib/assert.js 88.69% 12 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58253      +/-   ##
==========================================
- Coverage   90.08%   90.03%   -0.05%     
==========================================
  Files         641      648       +7     
  Lines      188718   191114    +2396     
  Branches    37022    37472     +450     
==========================================
+ Hits       170000   172076    +2076     
- Misses      11417    11661     +244     
- Partials     7301     7377      +76     
Files with missing lines Coverage Δ
lib/internal/assert/assertion_error.js 95.93% <100.00%> (+<0.01%) ⬆️
lib/internal/errors.js 97.50% <100.00%> (+<0.01%) ⬆️
lib/assert.js 98.11% <88.69%> (-1.40%) ⬇️

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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This is already looking very promising! We need to document the class and the new AssertionError option, since both are exposed publicly.

We should also not expose the internal options.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The code change is LGTM!

I am actually thinking it might be worth changing the default behavior for the non strict methods to be strict and add an option to change that back. That would prevent wrong usage which is quite likely right now when using the non-strict methods.

I do think changing that would be great before we land this.

@miguelmarcondesf
Copy link
Contributor Author

I am actually thinking it might be worth changing the default behavior for the non strict methods to be strict and add an option to change that back. That would prevent wrong usage which is quite likely right now when using the non-strict methods.

@BridgeAR sounds good! I just pushed some changes related to that, thank you!

@miguelmarcondesf miguelmarcondesf force-pushed the assert-become-class branch 2 times, most recently from 3827903 to 41ae5be Compare June 18, 2025 22:07
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Great work! This is LGTM with my last comment being addressed!

I do think someone else should also have a look, since this is a bigger feature overall.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can we modernize this and use ES6 classes?

@miguelmarcondesf
Copy link
Contributor Author

LGTM with a couple of suggestions.

@jasnell Thank you for pointing that!
I added some comments to the assert file and our documentation, and also added some scenarios for the destructing behavior.
Let me know if it makes sense.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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 Aug 5, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 5, 2025
@nodejs-github-bot nodejs-github-bot merged commit 4f5d11e into nodejs:main Aug 5, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4f5d11e

panva pushed a commit to panva/node that referenced this pull request Aug 7, 2025
PR-URL: nodejs#58253
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Aug 8, 2025
PR-URL: #58253
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
mete0rfish pushed a commit to mete0rfish/node-contribute that referenced this pull request Aug 9, 2025
PR-URL: nodejs#58253
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
panva pushed a commit to panva/node that referenced this pull request Aug 9, 2025
PR-URL: nodejs#58253
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 12, 2025
PR-URL: #58253
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants
X Tutup