[Bug]: New SERIALIZABLE_PROPERTIES is only used by matcher-utils

stephenh commented 3 months ago

Version

30.0.0-alpha.4

Steps to reproduce

Setup a test with the new SERIALIZABLE_PROPERTIES feature:

  it("tests foo", () => {
    class Volume {
      constructor(amount, unit) {
        this.amount = amount;
        this.unit = unit;
      }

      get label() {
        throw new Error("Not implemented");
      }
    }
    Volume.prototype[SERIALIZABLE_PROPERTIES] = ["amount", "unit"];
    expect(new Volume(10, "L")).toEqual(new Volume(10, "L"));
  });

And set a breakpoint in the expect-utils eq function.

Expected behavior

I expected toEquals and eq to use the SERIALIZABLE_PROPERTIES for the equality checked, but it is only checked for the diff formatting.

Actual behavior

SERIALIZABLE_PROPERTIES is actually only checked in the printDiffOrStringify method.

Additional context

I work with an ORM that creates deeply-interlinked entities, and so have been wanting to customize the toEqual functionality for awhile now--particularly our diffs toEqual failures can get huge.

The new SERIALIZABLE_PROPERTIES looked really neat, but I was surprised to see it's only used by the diff printer, and not the core eq logic itself.

I admittedly do not have a bug reproducing exactly why that's problematic, but it does just seem unintuitive? Either from two perspectives:

The name SERIALIZABLE_PROPERTIES to me infers it will be used "for serialization" which I would have expected to apply to both "the toEqual check" as well as "the toEqual diff", but also
If the core expect-utils eq logic did not need SERIALIZABLE_PROPERTIES to avoid blowing up on the getter from the original PR's use case (which FWIW we've definitely hit that bug too, that's why I care about this), then why did matcher-utils need a new custom property? Couldn't matcher-utils have just used expect-utils logic?

I guess I don't have a true bug per se, so happy to close this out @georgekaran / @SimenB , but it just seems a little disjoint, i.e.:

custom equality checkers are hook into toEqual eq but not the diff creation
SERIALIZABLE_PROPERTIES hooks into the diff creation, but not toEqual eq

I totally get evolving ubiquitously-used software over-time is basically impossible :sweat: , but just seems like there is a lot of disjointness, and maybe SERIALIZE_PROPERTIES as a user-observable flag wasn't a necessary addition, if matcher-utils-the-diff and expect-utils-the-equality reused more of their core functionality.

Again, not a bug per se, just an observation that SERIALIZABLE_PROPERTIES doesn't work the way I expected, and I think for our ORM usage we'll either have to do both custom equality checker + SERIALIZEABLE_PROPERTIES to get toEqual to do what we want. Which is fine too.

Thanks!

Environment

Jest v30.0.0-alpha.4

SimenB commented 3 months ago

I think it makes sense to align, but I'll defer to @georgekaran

georgekaran commented 3 months ago

Hey @stephenh

Your points make a lot of sense when it comes to the SERIALIZABLE_PROPERTIES. To be honest, maybe the name SERIALIZABLE_PROPERTIES wasn't the best choice 😅, which might have led to some confusion as it seems to imply a broader scope than it currently has.

That being said, I think we could go to either two directions:

Rename this property to make it clear that is only used by the diff printer;
Apply this same variable to equality checkers.

Personally the second option makes more sense to me, but is probably gonna take a lib bit more time than just a renaming.

Let me know what you guys think, so I can start working on a draft to address this issue and aim to include it in a future pull request.

github-actions[bot] commented 2 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

Fryuni commented 2 months ago

It seems the misunderstanding was here:

The name SERIALIZABLE_PROPERTIES to me infers it will be used "for serialization" which I would have expected to apply to both "the toEqual check" as well as "the toEqual diff", but also

Specifically, the "toEqual check". There is no serialization there; what is being compared is the runtime values. If it weren't for that, many values that are supposed to be different would pass as equals and vice-versa.

For example, a sparse array of length 2 (Array(2)) has the same value in all positions as an array of two undefined elements ([,,]), but those two don't serialize to the same thing ([ <2 empty items> ] vs [undefined, undefined]). This even happens in other cases like objectContaining, where the two objects being compared can be of different classes and, as such, will serialize differently while matching the test. The diff might show all the lines that are different, even though only a few of them might actually be causing the matcher to fail.

If the matcher was .serializesEquallyAs(other), then it would make sense to serialize the value for comparison.

jestjs / jest