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

Proposal: assert.matchObject #50399

Closed
arthurfiorette opened this issue Oct 25, 2023 · 24 comments · Fixed by #54630
Closed

Proposal: assert.matchObject #50399

arthurfiorette opened this issue Oct 25, 2023 · 24 comments · Fixed by #54630
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@arthurfiorette
Copy link

What is the problem this feature will solve?

(I tried searching for related issues, but couldn't find one, sorry if this is a duplicated)

One thing I miss when using node:test and node:assert from jest related tests is the expect(A).toMatchObject(B) api. Within large production systems, a we usually have large objects with multiple deep properties, with assert.matchObject we could test the same way as assert.deepStrictEqual, but only specifying expected properties we want to test out with a much nicer DX. Obviously a assert.notMatchObject should also be present.

Current behavior:

const myObjectToTest = {
  a: 1,
  b: 2,
  c: {
    d: 3,
    e: 4,
    f: {
       g: 5
    }
  }
}

// For my specific test, I want to assert `myObjectToTest.b` and `myObjectToTest.c.f.g`.
assert.equal(myObjectToTest.b, 2);
assert.equal(myObjectToTest.c.f.g, 5);

// If I wanted to check against another object or using a object (better DX)
// I must specify ALL fields (even the one that doesn't matter for my specific test)
assert.deepStrictEqual(myObjectToTest, {
  a: 1,
  b: 2,
  c: {
    d: 3,
    e: 4,
    f: {
      g: 5
    }
  }
});

What is the feature you are proposing to solve the problem?

With assert.matchObject:

assert.matchObject(myObjectToTest, {
  b: 2,
  c: {
    f: {
      g: 5
    }
  }
});

It should ensure equality only for properties assigned to the second parameter. myObjectToTest could have 10000 other attributes or be literally the same as the second parameter, it would pass. Whenever a property declared inside the second object is not present in the first one, an AssertionError should be thrown.

Of course this is a small example that could be tested separately, but it is a perfect DX fit for large objects.

What alternatives have you considered?

I'm creating this feature request with a jest like API in mind, however any alternatives that also fixes this problem are welcome :) I'm also open to working on this, but first I need some kind of guidance :)

@arthurfiorette arthurfiorette added the feature request Issues that request new features to be added to Node.js. label Oct 25, 2023
@tniessen tniessen added the assert Issues and PRs related to the assert subsystem. label Oct 26, 2023
@targos
Copy link
Member

targos commented Oct 26, 2023

/cc @nodejs/assert. This doesn't seem like a bad idea to me.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2023

so the difference between this and deepEqual is just that it only checks present properties on the "actual" against the "expected"? could this just be an option to deepEqual?

@arthurfiorette
Copy link
Author

so the difference between this and deepEqual is just that it only checks present properties on the "actual" against the "expected"?

yes.

could this just be an option to deepEqual?

Well, I guess so, but i'm more convinced that it should be the same as many other testing frameworks as a dedicated method.

@aduh95
Copy link
Contributor

aduh95 commented Oct 26, 2023

To me "assert to match object" is really not something I could understand without reading its docs, so I would prefer if we picked a different name. I think adding allowSuperset option to deepStrictEqual or something like that would make the most sense, and we can always expose an alias later if we feel the need (just my two cents, feel free to disagree). PRs welcome :)

@MoLow
Copy link
Member

MoLow commented Oct 26, 2023

+1 on that. assert.throws already supports something similar

@Pyrolistical
Copy link

Pyrolistical commented Oct 26, 2023

could this just be an option to deepEqual?

imo no. the current signature of deepEqual is assert.deepEqual(actual, expected[, message]), and if we were to maintain backwards compatibility, we would need to add the option after message. but in order for people to use this new option, they would be required to define a message which they may not want to.

A way to make this would would require a soft breaking change with a new signature of:

deepEqual<T>(actual: unknown, expected: T, messageOrOption?: string | {
  message?: string;
  superset?: boolean;
})

@Pyrolistical
Copy link

Pyrolistical commented Oct 26, 2023

@aduh95 I agree matchObject isn't a great name if it were to work with arrays (as does jest toMatchObject), but I also don't like adding an option to deepStrictEqual for the same reason as my previous comment.

How about deepSupersetStrictEqual? We still want the primitives to be compared using ===. Maybe never add legacy deepSupersetEqual to node:assert but only add it to node:assert/strict?

@ljharb
Copy link
Member

ljharb commented Oct 26, 2023

I think it would be fine to make the optional third argument an options object if not a string, as you suggested - and will ensure future compatibility for additional options.

@bakkot
Copy link

bakkot commented Oct 26, 2023

"superset" is a confusing name for this behavior; "allowExtraProperties", maybe?

@arthurfiorette
Copy link
Author

Just extending the deepStrictEqual into this signature would not result in a breaking change:

function deepStrictEqual<T>(actual: unknown, expected: T, message?: string | Error): asserts actual is T;
function deepStrictEqual<T>(actual: unknown, expected: T, options?: { superset?: boolean }, message?: string | Error): asserts actual is T;

I'm still not sure if an attribute would have a better DX:

// attribute
assert.deepStrictEqual(
  obj,
  {
    a: 1,
    b: 2
  },
  {
    superset: true
  }
);

// custom method
assert.deepSupersetEqual(obj, {
  a: 1,
  b: 2
});

@MoLow
Copy link
Member

MoLow commented Oct 27, 2023

what is the downside of a new method? @aduh95

@ljharb
Copy link
Member

ljharb commented Oct 27, 2023

It's verbose, and another thing to learn, and having two methods that are identical except for one aspect is strange when it could be one method with an option.

@arthurfiorette
Copy link
Author

It's verbose, and another thing to learn

But wouldn't this behavior align with many other testing frameworks?

@ljharb
Copy link
Member

ljharb commented Oct 27, 2023

Not all of them - iirc, in jasmine you’d use a special matcher wrapper around the object to indicate a partial match

@arthurfiorette
Copy link
Author

https://jasmine.github.io/api/edge/jasmine.html#.objectContaining

@ljharb
Copy link
Member

ljharb commented Oct 27, 2023

Either way tho, by and large there simply isn't a single cohesive ecosystem consensus around any part of testing. The downside of that is that node runs the risk of conflating patterns and confusing users; the upside is that node is free to make its own decisions.

@arthurfiorette
Copy link
Author

I agree, either way we should make a decision, right? What options are being considered? Should I make a poll?

@Pyrolistical
Copy link

I don't want to build by committee. I rather have one person do the leg work and make a recommendation. People are then free to criticize and make another recommendation if they so please.

@arthurfiorette
Copy link
Author

arthurfiorette commented Oct 30, 2023

I'd love to work on this feature, could anyone give me a heads up on where to start? Apparently I should start supporting the keyCheck function used in innerDeepEqual, to just compare properties of val2 in val1 and not in both sides

@reconbot
Copy link
Contributor

That sounds right. It's called assertSubset in a few other libraries btw. A wonderful feature if you're ever working with objects with timestamps.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 18, 2023

I have a working implementation and I'll open a PR for it soon (I need to finish updating the docs, add more tests and make sure the error message is not weird).

The error message can't display a diff for now as the object tested for might highly deviate and we need a new implementation for the diff.

How to expose it can still be discussed. I think it's easiest and most user friendly to have a separate API next to deepStrictEqual() due to the current APIs arguments, the implementation deviates in multiple spots in a way that documenting that would become difficult and the use cases are just different (full equality vs. partial).
Having e.g., assert.deepStrictEqual(actual, assert.partial(expected)) is also not as readable as assert.contains(actual, expected).

@arthurfiorette
Copy link
Author

Looking at it without context, assert.contains would probably imply, because it is very similar, that we are looking whether a string is contained within another string. I'd expect assert.contains(a, b) to be the same as assert.ok(a.contains(b)), given a and b are strings.

I still think the name deepSupersetEqual fits better.

@onlywei
Copy link

onlywei commented May 29, 2024

Has anyone ever considered a DX like the following?

assert.deepStrictEqual(something, assert.objectContaining(expected))

@danielbayley
Copy link
Contributor

danielbayley commented Aug 19, 2024

assert.matchObject/matchObjectStrict

This way, if we also wanted a corresponding method to match array subsets, it would have to be called assert.matchArray or something…

Given that we currently have deepEqual for comparing objects and arrays for exact equality, match for matching substrings, and considering existing parlance… I think we should go with the name deepMatch, to assert that A (object or array) contains subset B. We could/should also have an assert.includes method that mirrors the behaviour of the native JS .includes (string or array).

assert.contains

‘contains’ makes sense, but the naming should match existing JavaScript parlance, which, for reasons, went with .includes instead. Aiming for consistency here…

To clarify, something like:

import assert from "node:assert/strict"

assert.deepMatch({ a: 1, b: 2, c: 3 }, { b: 2 }) // pass
assert.deepMatch([1, 2, 3], [2]) // pass

Then, for assert.includes, something like:

import assert from "node:assert/strict"

assert.includes ??= (a, b) => assert.equal(a.includes(b), true)

assert.includes("abc", "b")   // pass
assert.includes([1, 2, 3], 2) // pass

@arthurfiorette @targos @ljharb @MoLow @reconbot @synapse @simoneb What do you guys think?

puskin94 added a commit to puskin94/node that referenced this issue Aug 29, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 20, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 23, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 23, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 23, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 23, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 24, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 24, 2024
puskin94 added a commit to puskin94/node that referenced this issue Sep 24, 2024
puskin94 added a commit to puskin94/node that referenced this issue Oct 1, 2024
puskin94 added a commit to puskin94/node that referenced this issue Oct 13, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 6, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 6, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 6, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 7, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 7, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 7, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 8, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 12, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 12, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 15, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 20, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
puskin94 added a commit to puskin94/node that referenced this issue Nov 20, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Nov 26, 2024
Fixes: nodejs#50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
PR-URL: nodejs#54630
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this issue Nov 26, 2024
Fixes: #50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
PR-URL: #54630
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Fixes: #50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
PR-URL: #54630
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Fixes: #50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
PR-URL: #54630
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Fixes: #50399

Co-Authored-By: Cristian Barlutiu <[email protected]>
PR-URL: #54630
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
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. feature request Issues that request new features to be added to Node.js.
Projects
Archived in project