assert: add matchObjectStrict and matchObject

[synapse]

fixes #50399

What is this PR doing:

• Added 2 new assert functions matchObjectStrict & matchObject
• Added tests to check different object types (Feel free to suggest more types to be tested)
• Added a new section in the Assert documentation to explain the usage

The compareBranch (used by matchObject) works as follows:

• matchObject recursively traverses the actual objects, Map, and Set and rely on deepEqual to compare its values (event across different realms) - N.B. array are directly compared using deepEqual and will not match if they are from different realms
• if the value is a plain object then it will retrigger the compareBranch from that branch onward
• else will compare the actual and expected values using the built-in isDeepEqual or isDeepStrictEqual based on the loose/strict param
• it compares keys - (using Reflect.ownKeys() instead of Object.keys() to include symbol properties)
• checks for recursive object values

[aduh95]

There are a lot of unrelated changes, making this hard to review. Consider removing those (maybe a problem with the configuration of your text editor?), and run the linters (NODE=node make test-doc -j and make lint-js).

[aduh95]

That's hardly what's happening, is it? At least I find the wording very confusing (I would expect the same behavior as strictEqual with the current wording), we should explain in more details what's happening here, what's being compared: is it only own properties? is the prototype important?

[aduh95]

Is it actually any? What happens if we pass a primitive?

[synapse]

It's any because it compares the values (and the keys), which could be primitives. I'll add a couple of tests for primitives too.

[targos]

@nodejs/assert

[panva]

Feel free to suggest more types to be tested.

Should't this fall back on assert.deepEqual and assert.deepStrictEqual respectively when dealing with anything but simple objects?

If I pass two different KeyObject or CryptoKey instances to matchObject, despite them being different cryptographic keys this doens't throw.

If it falled back on on assert.deepEqual and assert.deepStrictEqual then handling of those respective objects would be handled (#50897).

lib/internal/util/comparisons.js already has these already in

[panva]

Based on the docs I find it very confusing to discern between this and assert.deep(Strict)Equal. "Matching" suggests (to me at least) that the "expected" argument might be an object with more properties than "actual" but that's not what's documented nor implemented.

[synapse]

Feel free to suggest more types to be tested.

Should't this fall back on assert.deepEqual and assert.deepStrictEqual respectively when dealing with anything but simple objects?

If I pass two different KeyObject or CryptoKey instances to matchObject, despite them being different cryptographic keys this doens't throw.

If it falled back on on assert.deepEqual and assert.deepStrictEqual then handling of those respective objects would be handled (#50897).

lib/internal/util/comparisons.js already has these already in

Should be handled now

[MoLow]

please remove merge commit. changes generally LGTM, I think this is a great addition to node:assert

[Trott]

@nodejs/assert How sure are we that we want to add this? If it lands, should it be added as experimental for a while to give us the option to change it as we see fit for a while or even remove it entirely without waiting for a major release?

[jasnell]

I'm still not convinced that this is sufficiently different from deepEqual and deepStrictEqual to warrant a new API. At the absolute least the documentation does not provide enough information for someone to be able to decide which to use

[synapse]

@jasnell If the expected object has fewer properties than the actual object, deepEqual will throw an error based on your example. On the other hand, matchObject will compare the existing properties and values in the expected object, allowing for a partial comparison.

[jasnell]

Yeah I get that, I just do not think there's enough justification for a new api. A new option to the existing method could do the same.

[simoneb]

There was some conversation in the original issue about whether it should be:

1. a new method, e.g. matchObject
2. an option to the existing method, e.g. deepStrictEqual(..., { partial: tru })
3. a utility function for the existing method, e.g. deepStrictEqual(..., objectContaining(...))

here @synapse decided to go with option 1, lacking a clear consensus on which option made the most sense

[jsumners]

I'm still not convinced that this is sufficiently different from deepEqual and deepStrictEqual to warrant a new API. At the absolute least the documentation does not provide enough information for someone to be able to decide which to use

I agree. I would love to see node:assert have a match function equivalent to https://github.com/tapjs/tapjs/blob/8f2baa7eb5e1f4cd0fcf8b60da4fb4e41ea680f1/docs/src/content/docs/api/asserts/index.md?plain=1#L318-L338

But the proposal here seems like a less consistent deepStrictEqual.

[ljharb]

these seem flipped - the strict variant should not allow type coercion, and the loose variant should permit it.

[danielbayley]

@synapse See #50399 (comment).

[simoneb]

This PR can be closed as work on the linked issue is being done elsewhere.

[jsumners]

This PR can be closed as work on the linked issue is being done elsewhere.

Where is that work happening?

[simoneb]

This PR can be closed as work on the linked issue is being done elsewhere.

Where is that work happening?

#54630

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[RedYetiDev]

@synapse are you still interested in this PR, given that it is possibly superceded by #54630?

[synapse]

@synapse are you still interested in this PR, given that it is possibly superceded by #54630?

No, I will close this off

[jsumners]

This PR can be closed as work on the linked issue is being done elsewhere.

Where is that work happening?

#54630

Thank you.