assert: adds partialDeepEqual, partialDeepStrictEqual, includes

[puskin]

Fixes: https://github.com/nodejs/node/issues/50399

Took heavy inspiration from https://github.com/nodejs/node/pull/53415 , trying to push it to the finish line 🚀

On top of it, I took the liberty of:

1. refactor the code a bit
2. wrote more documentation
3. added more tests and restructured old ones
4. added includes and includesStrict methods
5. implement previously reported comments / suggestions

Co-Authored-By: Cristian Barlutiu

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 95.73460% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.50%. Comparing base (4f88179) to head (8722fbd). Report is 463 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/assert.js	95.71%	8 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54630      +/-   ##
==========================================
+ Coverage   88.23%   88.50%   +0.26%     
==========================================
  Files         652      653       +1     
  Lines      183920   187937    +4017     
  Branches    35863    36233     +370     
==========================================
+ Hits       162286   166333    +4047     
+ Misses      14913    14827      -86     
- Partials     6721     6777      +56     

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	96.97% <100.00%> (+0.03%)	:arrow_up:
lib/assert.js	99.01% <95.71%> (-0.86%)	:arrow_down:

... and 289 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[github-actions[bot]]

The https://github.com/nodejs/node/labels/notable-change label has been added by @RedYetiDev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[avivkeller]

This PR adds new functionality, hence semver-minor

This PR's new functionality (IMO) is notable-change

[puskin]

is there any way we can push this forward? ar far as I know there is a little bit of people waiting for this and no clear consensus on the naming convention :)

[jasnell]

I still have concerns about whether we need everything in this PR but would very much like others to weigh in. @nodejs/test_runner @nodejs/assert ... anyone have thoughts?

[ljharb]

re naming, I definitely think the term "match" should be reserved for regexes.

if the only difference between these new methods and deepEqual etc is that they allow extra properties, then perhaps it would make more sense as an option to the existing methods rather than entirely new ones?

[puskin]

@ljharb the discussion where the "option" was discarded in favor of the new API started from this comment: https://github.com/nodejs/node/issues/50399#issuecomment-1860653681

[puskin]

other method names we could consider:

assert.subsetEqual() assert.containsSubset() assert.partialEqual()

[ljharb]

To clarify, the current proposal takes an "actual" and "expected", and it checks that every property on expected is deepEqual to the same property on actual? Are nested objects on expected checked using full or partial equality?

What about if I want to assert that a property is not present? is there a way to represent "never"?

[puskin]

good questions!

yes, this test will pass:

      {
        description: 'compares two deeply nested objects with partial equality',
        actual: { a: {nested: {property: true, some: 'other'}} },
        expected: { a: {nested: {property: true}} },
      },

and no, there is no way to check "not inclusion" , just if an object is a subset of another

[ljharb]

assert.subsetDeepEqual? (also shouldn't the default be strict, and the extra one be "Loose"?)

[puskin]

that would deviate from the already present implementations, like deepStrictEqual and deepEqual

[avivkeller]

+1 for partialDeepEqual

Agreed. Also, should this have a notPartialDeepEqual inverse?

[puskin]

Updated:

1. names: from deepMatch / deepMatchStrict to partialDeepEqual / partialDeepStrictEqual
2. tests: added a couple of cases and better syntax
3. validations: includes and includesStrict now validate actual and expected types beforehand

[mcollina]

It's probably worth mentioning in the docs about Map and Set support

[puskin]

Updated the docs with additional examples showing more complicated usages, including Maps and Sets

[avivkeller]

Should this have the inverses also? notPartialDeepEqual and such?

[puskin]

Should this have the inverses also? notPartialDeepEqual and such?

how would that look like? something like this?

assert.notPartialDeepEqual({a: 1}, {a: 1, b: 2});
// THROWS

assert.notPartialDeepEqual({a: 3}, {a: 1, b: 2});
// DOES NOT THROW

ayway, I would build that in a different PR, this one is already stuffed enough imho 😄

[avivkeller]

Got it, everything else seems good to me

[nodejs-github-bot]

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

[puskin]

it feels like the CI is stuck for hours now

[avivkeller]

it feels like the CI is stuck for hours now

It looks like the CI was aborted.

[nodejs-github-bot]

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

[puskin]

@Renegade334 oh, I wasn't aware of it! updated 😄

[simoneb]

Note that the method names will also need to be added to lib/internal/test_runner/test.js:

https://github.com/nodejs/node/blob/e49cf7acfbfb5ce36f49911d22f533c7056137c2/lib/internal/test_runner/test.js#L103-L120

An equivalent change will be required in test/parallel/test-runner-assert.js:

https://github.com/nodejs/node/blob/e49cf7acfbfb5ce36f49911d22f533c7056137c2/test/parallel/test-runner-assert.js#L7-L25

Maybe we should add an automatic way to check that this happens?

[avivkeller]

Certain names are intentionally excluded from that list (E.G strict), so adding an lint-rule validating it is easier said than done, IMO. Although - I may have an idea. #55036

[avivkeller]

Can you squash into a single commit with an updated message reflecting the current state?

[avivkeller]

@puskin94 can you rebase to include #55036? You can undo your changes to test-runner-assert, as that PR should cover it.

[boukeversteegh]

Nice, looking forward to this getting merged!

[nodejs-github-bot]

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