Changing assert to become a class

[miguelmarcondesf]

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.
• Review performance implications.

---

cc @BridgeAR

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[mcollina]

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]

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]

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]

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.

[BridgeAR]

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]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 94.30894% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.13%. Comparing base (214e4db) to head (41ae5be).

Files with missing lines	Patch %	Lines
lib/assert.js	93.57%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58253      +/-   ##
==========================================
- Coverage   90.13%   90.13%   -0.01%     
==========================================
  Files         639      639              
  Lines      188192   188277      +85     
  Branches    36911    36935      +24     
==========================================
+ Hits       169634   169706      +72     
- Misses      11287    11334      +47     
+ Partials     7271     7237      -34     

Files with missing lines	Coverage Δ
lib/internal/assert/assertion_error.js	95.94% <100.00%> (+0.01%)	⬆️
lib/assert.js	98.77% <93.57%> (-0.74%)	⬇️

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

[BridgeAR]

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.

[BridgeAR]

AFAIK these should be kept. The diff itself should already contain everything. This is just printing additional information and that should be fine to cut off.

[miguelmarcondesf]

Sorry, I didn't get it if I need to change something here.
Without removing the ellipsis, actual and expected messages will have ... in the end, even if it is returning the full diff string.

[BridgeAR]

That is correct, while it would show up in the diff in the message that is a combination of both sides.
So keeping this cut off should be fine.

[BridgeAR]

I believe my phrasing was not ideal 😅
What I meant is that the overall full diff contains all changes. This part is just the same information again, just as part of the object and it would repeat the information from the diff. As such, not showing the full output here should absolutely be fine and I think we should keep the ellipsis.

[BridgeAR]

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.

[BridgeAR]

That is correct, while it would show up in the diff in the message that is a combination of both sides.
So keeping this cut off should be fine.

[miguelmarcondesf]

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!

[BridgeAR]

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.

[anonrig]

Can we modernize this and use ES6 classes?

[anonrig]

Can we just use class syntax here?

[miguelmarcondesf]

Hey, we initially started the implementation with ES6 classes, but it was raised that this could lead to more churn than needed. So maybe we can start wth 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.