Skip to content
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

assert: support inequalities #55796

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Nov 9, 2024

This PR introduces four new assertion methods

Motivation

These methods align with the capabilities provided by other popular testing libraries, such as:

While it’s possible to use standard assertions like assert(x > y), dedicated methods offer more informative error messages by including displaying the values of x and y in the failure output. This saves time and effort, particularly when working with large test suites, as there’s no need to manually format output with assert(x > y, `${x} > ${y}`).

@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 Nov 9, 2024
Copy link

codecov bot commented Nov 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (58a8eb4) to head (1eb4315).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55796   +/-   ##
=======================================
  Coverage   88.40%   88.41%           
=======================================
  Files         654      654           
  Lines      187815   187891   +76     
  Branches    36136    36145    +9     
=======================================
+ Hits       166045   166130   +85     
- Misses      15001    15003    +2     
+ Partials     6769     6758   -11     
Files with missing lines Coverage Δ
lib/assert.js 99.88% <100.00%> (+0.01%) ⬆️
lib/internal/test_runner/test.js 96.98% <100.00%> (+<0.01%) ⬆️

... and 29 files with indirect coverage changes

@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2024

This PR goes against Node.js core principles, we're all idealist people who strive for a better world, we do not and never will want to support inequalities! :trollface:

lib/assert.js Outdated Show resolved Hide resolved
lib/assert.js Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

besides naming, LGTM

@MoLow MoLow requested a review from BridgeAR November 10, 2024 22:02
@RedYetiDev
Copy link
Member Author

Would you rather lessThanOrEqual or lessThanOrEqualTo?

* @param {string | Error} [message]
*/
assert.greaterThan = function greaterThan(actual, expected, message = `Expected ${actual} to be greater than ${expected}`) {
if (actual <= expected) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt we validate that inputs are numbers?

@LiviaMedeiros
Copy link
Contributor

I feel +1 for having this whenever I see assert(actual < expectedMax) without message just to see the actual value; but the implementation raises questions, especially if we want it to be a part of public API.

node:assert has Legacy and Strict modes. The current implementation feels like a part of Legacy mode, because inequality operators work similar to == rather than ===.
Do we want to add new methods to Legacy mode?
Do we want to have both versions?
What value types do we want to support? Do we allow lessThan(false, 2)? lessThan(1, 2n)? lessThan('a', 'b')? lessThan([], [1])?
Do we want type validation to be configurable?
Will it match the expectations for everyone?

An important thing to consider is that message is the key here.

// `false == true` not informative, have to look at the code
assert(retryCount < RETRY_MAX);

// `Expected 5 to be less than 5` tells the compared values but still not informative
assert.lessThan(retryCount, RETRY_MAX);

// Informative, both lines yield same message
assert(retryCount < RETRY_MAX, `retryCount has value of ${retryCount} which is greater than RETRY_MAX`);
assert.lessThan(retryCount, RETRY_MAX, `retryCount has value of ${retryCount} which is greater than RETRY_MAX`);

// Even better when message tells the context of error
assert(retryCount < RETRY_MAX, 'function myFunc retried too many times when delay is set to 0 and data is recursive')

The current implementation uses message as is if it's present, so it becomes just a slower and more verbose version of assert.ok.

If we make strict version with type validation, how do we handle the case when types are not matched? E.g.:

// Old code without explicit `assert.equal(typeof retryCount, 'number')`
assert.ok(retryCount <= RETRY_MAX, `retry count greater than ${RETRY_MAX}`);

// New code?
assert.strictNotGreaterThan(retryCount, RETRY_MAX, `retry count greater than ${RETRY_MAX} OR retry count's type ${typeof retryCount} is not number`);

@RedYetiDev
Copy link
Member Author

Sounds good to me. Currently, as you said, I do use the message property, but then difference that the error contains the "actual" and "expected" failures as properties.

I could also do what the other assertion methods do, that is, have their own message that displays along with the user provided message.

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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants