assert: add partialDeepStrictEqual #54630

puskin94 · 2024-08-29T12:27:24Z

Fixes: #50399

Took heavy inspiration from #53415 , trying to push it to the finish line 🚀

On top of it, I took the liberty of:

refactor the code a bit
wrote more documentation
added more tests and restructured old ones
implement previously reported comments / suggestions

Co-Authored-By: Cristian Barlutiu

codecov · 2024-08-29T14:01:51Z

Codecov Report

Attention: Patch coverage is 95.74468% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (4f88179) to head (561c191).
Report is 376 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/assert.js	95.72%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54630      +/-   ##
==========================================
+ Coverage   88.23%   88.41%   +0.18%     
==========================================
  Files         652      654       +2     
  Lines      183920   187944    +4024     
  Branches    35863    36178     +315     
==========================================
+ Hits       162286   166180    +3894     
- Misses      14913    15000      +87     
- Partials     6721     6764      +43

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	`96.98% <100.00%> (+0.04%)`	⬆️
lib/assert.js	`99.09% <95.72%> (-0.78%)`	⬇️

... and 286 files with indirect coverage changes

doc/api/assert.md

test/parallel/test-assert-objects.js

github-actions · 2024-08-29T14:36:51Z

The notable-change PRs with changes that should be highlighted in changelogs. 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.

RedYetiDev · 2024-08-29T14:40:30Z

This PR adds new functionality, hence semver-minor

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

doc/api/assert.md

jasnell · 2024-08-29T17:40:06Z

doc/api/assert.md

+// AssertionError
+```
+
+## `assert.includes(actual, expected[, message])`


I'm less convinced that this one is useful. Why not just simply use assert(actual.includes(expected))

@jasnell I just followed the most "approved" comment in the original PR and implemented it :)

#50399 (comment)

I agree with @jasnell and don't think this should be implemented. Let us concentrate on the partial inclusion. If anyone would ever ask for something else, we can still implement more.

lib/assert.js

puskin94 · 2024-09-10T09:23:42Z

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 · 2024-09-10T15:39:30Z

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 · 2024-09-10T15:56:51Z

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?

puskin94 · 2024-09-10T17:18:21Z

@ljharb the discussion where the "option" was discarded in favor of the new API started from this comment: #50399 (comment)

puskin94 · 2024-09-10T17:33:53Z

other method names we could consider:

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

ljharb · 2024-09-10T17:43:25Z

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"?

puskin94 · 2024-09-10T18:01:42Z

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 · 2024-09-10T18:04:32Z

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

puskin94 · 2024-09-10T18:31:31Z

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

MoLow

implementation LGTM, I agree with the concerns on naming, lets change the name so this can land?
+1 for partialDeepEqual

lib/assert.js

test/parallel/test-assert-objects.js

RedYetiDev · 2024-09-11T02:20:28Z

+1 for partialDeepEqual

Agreed. Also, should this have a notPartialDeepEqual inverse?

BridgeAR · 2024-11-06T12:19:19Z

lib/assert.js

+      return false;
+    }
+
+    return ArrayPrototypeEvery(expected, (item) => ArrayPrototypeIncludes(actual, item));


This is actually incorrect. It will match identical parts multiple times. It is also going to have a bad performance.

I am not going to block this though, to get the general idea in and to improve afterwards. Can we just mark it experimental for now, due to these issues?

@BridgeAR this was an actual issue, addressed it!

BridgeAR · 2024-11-06T12:23:12Z

lib/assert.js

+    const key = keysExpected[i];
+    assert(
+      ReflectHas(actual, key),
+      new AssertionError({ message: `Expected key ${String(key)} not found in actual object` }),


This message won't be ideal. If it's a deeper array, it is completely unclear what this belongs to. It was actually my main issue with my first attempt. To highlight what parts are missing is tricky.

BridgeAR · 2024-11-06T12:30:34Z

lib/assert.js

+  if (comparedObjects.has(actual)) {
+    return true;
+  }
+  comparedObjects.add(actual);


I did not verify that but I believe the circular structure won't always work this way. AFAIC we have to handle both sides similar to the current implementation.

tniessen

I think the documentation should precisely characterize the behavior of this function, including edge cases. Right now, it only mentions the "main difference", but that doesn't explain differences such as the following:

assert.deepStrictEqual(new Set([{ a: 1 }]), new Set([{ a: 1 }]))
// No error.

assert.partialDeepStrictEqual(new Set([{ a: 1 }]), new Set([{ a: 1 }]))
// Uncaught AssertionError.

On a side note, the commit message should use an imperative verb (e.g., add) instead of adds to comply with our guidelines.

tniessen · 2024-11-06T11:59:51Z

doc/api/assert.md

@@ -2548,6 +2548,81 @@ assert.throws(throwingFirst, /Second$/);
 Due to the confusing error-prone notation, avoid a string as the second
 argument.

+## `assert.partialDeepStrictEqual(actual, expected[, message])`


This should probably be marked as experimental.

tniessen · 2024-11-06T14:06:46Z

doc/api/assert.md

+* `expected` {any}
+* `message` {string|Error}
+
+[`assert.partialDeepStrictEqual()`][] Assesses the equivalence between the `actual` and `expected` parameters through a


Suggested change

[`assert.partialDeepStrictEqual()`][] Assesses the equivalence between the `actual` and `expected` parameters through a

[`assert.partialDeepStrictEqual()`][] asserts the equivalence between the `actual` and `expected` parameters through a

BridgeAR · 2024-11-07T08:51:52Z

@tniessen I consider the mentioned difference a bug in the current implementation. I see a couple of cases not yet working as expected. We can either make sure it works fine up front or land this and continue on it later.

puskin94 · 2024-11-07T12:23:15Z

Fixed the issue with the comparison between objects in Sets, updated the commit message and the documentation

tniessen · 2024-11-07T12:36:07Z

Fixed the issue with the comparison between objects in Sets

I checked out f0722f9. Is this expected behavior?

assert.partialDeepStrictEqual(new Set([{ a: 1 }]), new Set([{ a: 1 }, { b: 1 }]))
// No error.

nodejs-github-bot · 2024-11-07T12:45:22Z

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

puskin94 · 2024-11-07T13:45:50Z

assert.partialDeepStrictEqual(new Set([{ a: 1 }]), new Set([{ a: 1 }, { b: 1 }]))

you are right, I might have introduced that issue in the last commit. Fixed and added relevant tests :)

marco-ippolito

LGTM

nodejs-github-bot · 2024-11-07T13:55:36Z

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

tniessen

I still think this function deserves a precise definition of its behavior. The two Set-related bugs that I discovered today could have been expected behavior for all I know.

Looking at the implementation, objects in Sets are apparently not tested for reference-equality, but objects in Arrays are. Again, I have no idea if that is expected behavior or not.

assert.deepStrictEqual([{ a: 5 }], [{ a: 5 }])
// No error.

assert.partialDeepStrictEqual([{ a: 5 }], [{ a: 5 }])
// AssertionError.

Also, the order of elements of Arrays is ignored for the purpose of this assertion, and holes in sparse arrays are ignored as well. I again have no idea if that is expected or not, but as a user, I certainly wouldn't expect it based on the documentation.

puskin94 · 2024-11-07T15:08:11Z

I still think this function deserves a precise definition of its behavior. The two Set-related bugs that I discovered today could have been expected behavior for all I know.

Looking at the implementation, objects in Sets are apparently not tested for reference-equality, but objects in Arrays are. Again, I have no idea if that is expected behavior or not.
assert.deepStrictEqual([{ a: 5 }], [{ a: 5 }])
// No error.

assert.partialDeepStrictEqual([{ a: 5 }], [{ a: 5 }])
// AssertionError.
Also, the order of elements of Arrays is ignored for the purpose of this assertion, and holes in sparse arrays are ignored as well. I again have no idea if that is expected or not, but as a user, I certainly wouldn't expect it based on the documentation.

I would consider what you found an issue, I will fix the code soon, but I would like other people to chip in on how this case should work according to them

BridgeAR · 2024-11-08T11:54:42Z

I think this could land as experimental and we explicitly mention that this should always pass deepStrictEqual(). So it's like a superset of that. I also think we should verify that later on and improving the code accordingly.

@puskin94 would you mind adding that?

Fixes: nodejs#50399 Co-Authored-By: Cristian Barlutiu <[email protected]>

tniessen · 2024-11-08T14:48:34Z

I think this could land as experimental and we explicitly mention that this should always pass deepStrictEqual(). So it's like a superset of that.

I agree with the general strategy, but I think it'd be a good idea for the documentation to accurately define what superset of input pairs are permitted that are not allowed by deepStrictEqual. Otherwise, users have no way of knowing whether they are relying on expected behavior or on bugs.

BridgeAR · 2024-11-08T16:13:08Z

I think it will still be refined during the development. And while doing that, we should define it.
It is marked as experimental because it's not yet where it should be.

danielbayley · 2024-11-08T18:21:34Z

I still think this function deserves a precise definition of its behavior. The two Set-related bugs that I discovered today could have been expected behavior for all I know.
Looking at the implementation, objects in Sets are apparently not tested for reference-equality, but objects in Arrays are. Again, I have no idea if that is expected behavior or not.
assert.deepStrictEqual([{ a: 5 }], [{ a: 5 }])
// No error.

assert.partialDeepStrictEqual([{ a: 5 }], [{ a: 5 }])
// AssertionError.
Also, the order of elements of Arrays is ignored for the purpose of this assertion, and holes in sparse arrays are ignored as well. I again have no idea if that is expected or not, but as a user, I certainly wouldn't expect it based on the documentation.
I would consider what you found an issue, I will fix the code soon, but I would like other people to chip in on how this case should work according to them

Would these different behaviours not be best expressed as boolean props of an optional object passed as the last argument to these functions? Something like:

assert.deepStrictEqual([1, 2, 3], [3, 1, 2]) // pass

assert.deepStrictEqual([1, 2, 3], [3, 1, 2], { order: true }) // fail

tniessen · 2024-11-08T23:10:08Z

@danielbayley The problem with such options is that it's unclear whether or not they should apply to nested decision procedures. Neither option will cover all use cases.

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Aug 29, 2024

puskin94 force-pushed the assert-deep-match-and-includes branch from 466c2f0 to c0a58e2 Compare August 29, 2024 12:28

puskin94 force-pushed the assert-deep-match-and-includes branch from c0a58e2 to 04e92f0 Compare August 29, 2024 14:20

jasnell reviewed Aug 29, 2024

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

RedYetiDev reviewed Aug 29, 2024

View reviewed changes

test/parallel/test-assert-objects.js Show resolved Hide resolved

RedYetiDev added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Aug 29, 2024

puskin94 force-pushed the assert-deep-match-and-includes branch from 04e92f0 to 0ee83d4 Compare August 29, 2024 16:04

jasnell reviewed Aug 29, 2024

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

jasnell reviewed Aug 29, 2024

View reviewed changes

pmarchini reviewed Aug 31, 2024

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

pmarchini reviewed Aug 31, 2024

View reviewed changes

lib/assert.js Show resolved Hide resolved

lib/assert.js Show resolved Hide resolved

MoLow reviewed Sep 10, 2024

View reviewed changes

Renegade334 reviewed Sep 10, 2024

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

test/parallel/test-assert-objects.js Show resolved Hide resolved

puskin94 force-pushed the assert-deep-match-and-includes branch 2 times, most recently from ab7e68e to 3d52d4b Compare September 11, 2024 06:34

BridgeAR reviewed Nov 6, 2024

View reviewed changes

puskin94 force-pushed the assert-deep-match-and-includes branch from d58ffe7 to c47e009 Compare November 6, 2024 13:31

tniessen reviewed Nov 6, 2024

View reviewed changes

puskin94 force-pushed the assert-deep-match-and-includes branch from c47e009 to f0722f9 Compare November 7, 2024 12:19

aduh95 approved these changes Nov 7, 2024

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

tniessen changed the title ~~assert: adds partialDeepStrictEqual~~ assert: add partialDeepStrictEqual Nov 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

puskin94 force-pushed the assert-deep-match-and-includes branch from f0722f9 to af0b9a0 Compare November 7, 2024 13:45

marco-ippolito approved these changes Nov 7, 2024

View reviewed changes

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

tniessen reviewed Nov 7, 2024

View reviewed changes

puskin94 force-pushed the assert-deep-match-and-includes branch from af0b9a0 to 5af955e Compare November 7, 2024 15:09

assert: add partialDeepStrictEqual

561c191

Fixes: nodejs#50399 Co-Authored-By: Cristian Barlutiu <[email protected]>

puskin94 force-pushed the assert-deep-match-and-includes branch from 5af955e to 561c191 Compare November 8, 2024 13:23

	[`assert.partialDeepStrictEqual()`][] Assesses the equivalence between the `actual` and `expected` parameters through a
	[`assert.partialDeepStrictEqual()`][] asserts the equivalence between the `actual` and `expected` parameters through a

assert: add partialDeepStrictEqual #54630

Are you sure you want to change the base?

assert: add partialDeepStrictEqual #54630

Conversation

puskin94 commented Aug 29, 2024 • edited Loading

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 29, 2024

RedYetiDev commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puskin94 commented Sep 10, 2024

jasnell commented Sep 10, 2024

ljharb commented Sep 10, 2024

puskin94 commented Sep 10, 2024

puskin94 commented Sep 10, 2024

ljharb commented Sep 10, 2024

puskin94 commented Sep 10, 2024

ljharb commented Sep 10, 2024

puskin94 commented Sep 10, 2024

MoLow left a comment • edited Loading

Choose a reason for hiding this comment

RedYetiDev commented Sep 11, 2024

BridgeAR Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Nov 7, 2024

puskin94 commented Nov 7, 2024

tniessen commented Nov 7, 2024

nodejs-github-bot commented Nov 7, 2024

puskin94 commented Nov 7, 2024

marco-ippolito left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 7, 2024

tniessen left a comment

Choose a reason for hiding this comment

puskin94 commented Nov 7, 2024

BridgeAR commented Nov 8, 2024

tniessen commented Nov 8, 2024

BridgeAR commented Nov 8, 2024

danielbayley commented Nov 8, 2024

tniessen commented Nov 8, 2024

puskin94 commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

MoLow left a comment •

edited

Loading

BridgeAR Nov 6, 2024 •

edited

Loading