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

• [X] Refactor assert into a class structure (old-school pattern).
• [X] Add diff option to show the full diff in assertion errors.
• [X] Ensure backward compatibility with existing assert usages.
• [X] 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.

[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[bot]]

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 :warning:

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%)	:arrow_up:
lib/internal/errors.js	97.50% <100.00%> (+<0.01%)	:arrow_up:
lib/assert.js	98.11% <88.69%> (-1.40%)	:arrow_down:

... and 115 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[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!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

I'll wait until Friday this week and land it as is in case there is no further review until then.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jasnell]

The one concern that I have with this is that it is not uncommon for code to destructure assert methods, and if someone destructures them from an Assert instance it's going to lose the diff setting.

assert.strictEqual({a: 1}, {b: {c: 1}});  // default, diff: "simple"

const myAssert = new assert.Assert({diff:'full'});

myAssert.strictEqual({a: 1}, {b: {c: 1}});   // diff: "full"

const {strictEqual} = myAssert;

strictEqual({a: 1}, {b: {c: 1}});  // diff: "simple"

Now, this is just kind of To Be Expected when prototype methods are destructed from class instances and I appreciate the work that's been done to make this work, but it's likely a good idea to document this at least

I'm also a bit concerned about folks making future changes in here that fail to take destructuring into consideration by adding references to this that are not appropriately guarded with this?. Some comments directly in the file could help as a reminder for this and the new test could likely be expanded to include coverage of calling the methods separated from the assertInstance that is created.

[nodejs-github-bot]

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

[miguelmarcondesf]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 4f5d11e6fbb791bf4cdd21c5e900f740452da04c