Correct highlighting for asymmetric matchers

[grosto]

Summary

I've opened this PR for #6184 and #6170.

From what I could find, there are two cases where Jest shows a confusing diff:

1. Object properties, which pass asymmetric matchers, are represented as failed fields.
2. Objects, which are checked against objectContaining or arrayContaining , are fully shown in diff instead of only showing the subset of an object.

@pedrottimark I will be waiting for your input regarding what and how :)
I have already read your comment in #7027 (comment).

Test plan

I have added several snapshot tests. I am not sure if this is the best way to illustrate the issue with tests, so the suggestions are highly welcome :)

[pedrottimark]

Yes, thank you for these snapshot tests as baseline so we can discuss desired report.

What do you think about moving expect.objectContaining down one level like hypothetical Flux standard action:

jestExpect({type: 'whatever', payload: {a: 'a', b: 'b', c: 'c', d: 'd'}}).toEqual(
  {
    type: 'whatever',
    payload: expect.objectContaining({
      a: 'x',
      b: expect.any(String),
    }),
  },
)

For your info, a solution to this problem will probably have to be powerful enough to solve another problem of deleted or especially inserted properties:

• during first phase of TDD
• incorrect assumptions about received value

Even though we mitigated the problem with a more efficient diff algorithm, I think Jest can do even better by understanding when properties are “equal” versus deleted or inserted.

Here are some important special cases when changes include both deleted and inserted props:

• key remained the same but value changed
• key changed but value remained the same

Although object properties are the first priority, here is an open-ended thought question for more meaningful diff: realistic scenarios when array items don’t match up. For example, React has key prop to decide when a collection changes, is each child the same versus deleted or inserted versus changed.

[grosto]

Here is a small update for now.

To get some inspiration and make sure that I am not gonna try to reinvent the wheel, I have been reading several codebases for the past week(deep-diff, concordance, jest-diff and some others related to diffing two objects).

Since yesterday I started designing a solution. I mostly use objects to reason about the problem, but I want to make sure that in the future the solution will be able to handle other data structures.

So far, I think I am on track, but I might bother you for some advice later this week.

[SimenB]

Fantastic @grosto! Looking forward to it 🙂

[thymikee]

Hey @grosto, anything we can help with to get this over the line? :)

[grosto]

Hey @thymikee,
I have not had any time to dedicate to this for the past two weeks. Hopefully, I will continue working on it later this week. If this is a high priority, maybe it's better if someone else will take over :)

[pedrottimark]

@grosto no worries, same for me.

[WeiAnAn]

Hey, I have some idea to solve this problem.

When jest-diff compare object, we iterate the object first.
If there is asymmetric matcher, check it is match or not.
If match, copy the asymmetric matcher to corresponding attribute.

Example:

const copyB = {...b};
Object.keys(a).forEach(key => {
  if (a[key].$$typeof === Symbol.for('jest.asymmetricMatcher')) {
    if (a[key].asymmetricMatch(copyB[key])) {
      copyB[key] = Object.assign(a[key], {});
    }
  }
});

And using this new object to diff.
Result:

[grosto]

I am back on this!

Sorry, I started a new job and really could not find a spare second. :)

I will need a day or two to remember the codebases and gather my thoughts again. I will get back with some ideas after that.

[grosto]

After having a fresh look at the problem, I was not really fond of my ideas from a month ago 😅. So I decided to think a bit more about it.

In order to provide a better visual representation of data differences, we have to change our approach from serialize-then-diff to diff-then-serialize. As already mentioned by @pedrottimark.

Below is some idea(In a very very naive Typescript):

const INSERTED = 'INSERTED';
const DELETED = 'DELETED';
const EQUAL = 'EQUAL';
const UPDATED = 'UPDATED';
const TYPE_EQUAL = 'TYPE_EQUAL';

type DiffResult =
  | typeof INSERTED
  | typeof DELETED
  | typeof EQUAL
  | typeof UPDATED
  | typeof TYPE_EQUAL;

interface PrimitiveValuesDiff {
  diffResult: DiffResult;
  path?: string;
  rhsValue?: PrimitiveValue;
  lhsValue?: PrimitiveValue;
  // there will be more fields needed for sure
}

interface ComplexValuesDiff {
  lhsConstructorName?: string;
  rhsConstructorName?: string;
  path?: string;
  diffResult: DiffResult;
  propertyDiffs: (ComplexValuesDiff | PrimitiveValuesDiff)[];
  // there will be more fields needed for sure
}

// example of two object diff
const expected = {
  a: 1,
  b: {
    c: expect.any(Number),
    d: 2,
    e: 3,
  },
};

const received = {
  a: 1,
  b: {
    c: 1,
    e: 4,
  },
};

diffComplexValues(expected, received, options);

{
  lhsConstructorName: 'Object',
  rhsConstructorName: 'Object',
  diffResult: TYPE_EQUAL,
  /* type equality is sufficient for complex values */
  propertyDiffs: [
    {
      path: 'a',
      rhsValue: 1,
      lhsValue: 1,
      diffResult: EQUAL,
    },
    {
      path: 'b',
      lhsConstructorName: 'Object',
      rhsConstructorName: 'Object',
      diffResult: UPDATED,
      propertyDiffs: [
        {
          path: 'c',
          diffResult: EQUAL,
          lhsValue: expect.any(Number),
          rhsValue: 1,
        },
        {
          path: 'd',
          diffResult: DELETED,
          lhsValue: 2,
        },
        {
          path: 'e',
          diffResult: UPDATED,
          lhsValue: 3,
          rhsValue: 4,
        },
      ],
    },
  ],
};


`
- Expected
+ Received

  Object {
    "a": 1,
    "b": {
      "c": 1,
  -   "d": 2,
  -   "e": 3,
  +   "e": 4,
    }
  }
`

The ComplexValuesDiff will be sufficient to produce a string representation of data differences/commonalities. With some extra fields, this data structure can be also used to describe diffs of Arrays, Maps, and Sets.

Important Note: I am not suggesting to create a big diff object and then pass it to another function to generate a diff string. We are probably going to create a diff string as we are traversing an object. The only reason why I made these types is that it's easier for me to illustrate the idea.

It is crucial that diff function is consistent with the equality one. We can extract the smaller functions from current equals. Then use these small functions to create a consistent equality and diff functions.
The new diff function can be used instead of compareObjects function in jest-diff, or it could be part of expect package.

I am still trying to solve expect.objectContaining. It only has asymmetricMatch method, which does the comparison and returns a boolean. The traversal of the object happens inside the asymmetric matcher, so there is no way to see which fields pass/failed from outside this class. I have some ideas on how to solve this, but I am not sure about them. Any suggestions are welcome :)

Glad to hear any feedback :)

[jeysal]

@grosto Super excited that you're working on tackling this long-standing design limitation!

Quick question: What's the difference between UPDATED and TYPE_EQUAL? Paths [] and ['b'] look like they should be the same case?

expect.* will probably supply functions that generate a custom diff specific to the matcher, right? If so, expect.objectContaining could generate an EQUAL node with the received value for properties that match or were unspecified, and UPDATED for mismatches?

[pedrottimark]

Thank you for sharing your thoughts!

I am not suggesting to create a big diff object and then pass it to another function

Great minds think alike: Scott Hovestadt is working on streaming interface for Jest reporters

I am still trying to solve expect.objectContaining

Yes, asymmetric matchers were a puzzle to me. Lack of API came up recently #8295 (comment)

It is crucial that diff function is consistent with the equality one …

Yes indeed, this paragraph is brilliant

Yes, let’s start with descriptive data structure to prototype enough parts to illustrate a few of the most desired improvements. I experimented with tuples, but hard to debug when I messed up.

Then use these small functions to create a consistent equality and diff functions

A question in my mind is how closely to couple the small functions to compare and format:

• loosest coupling is format functions match according to lhsConstructor and rhsConstructor properties, similar to serializer plugins which have a test method
• tightest coupling is compare function assigns format function as a property

I think the solution will be looser, but a vague idea for asymmetric matchers is to be able to provide formatter as optional property of the comparison data structure.

Which reminds me to throw out for discussion possible end goal for packaging:

• move equality (after deep cleaning of edge cases and breaking changes) into separate package like jest-equal
• create corresponding comparison into new package in Jest monorepo
• create corresponding formatting into new package in Jest monorepo

[pedrottimark]

Forgot to say about consistent functions, as comparison corresponds to equality, format functions borrow the best from pretty-format and jest-diff and overcome their limitations.

In your example, be able to display the change lines for updated primitive value:

• property key [EDIT: and punctuation] in dim color
• property values in green versus red

-   "e": 3,
+   "e": 4,

Also I think default --no-expand option will mean something other than number of adjacent lines.

[pedrottimark]

While thinking about formatting that is possible given comparison, here is a use case to consider

Highlight which properties in received object match expected object for negative toMatchObject

Example picture baseline at left and improved at right:

[grosto]

@jeysal Thank you for your input!

Quick question: What's the difference between UPDATED and TYPE_EQUAL?

UPDATED means that primitive values are not equal. When it comes to complex values, we do not need to know if they are deep equal or not. All we care about is if their type is equal. if it is, we continue checking their keys, otherwise we can mark them as UNEQUAL and move on. I see how this is confusing with current types.

@pedrottimark Thank you for your feedback. I am glad that we have the same vision and thank you for providing more problems to keep in mind while solving this problem.

Highlight which properties in received object match expected object for negative toMatchObject

I have been thinking about an interface to create a subset diff for toMatchObject /objectContaining and in general, make diff more customizable.

Currently, I am working on extracting smaller functions from equals and how to compose new diff and equality with them.

[pedrottimark]

While trying to write a minimal formatter, here are thoughts about the descriptive data structure:

• distinguish “complex” from “basic” comparison objects by presence or absence of propertyDiffs
• for lhsConstructorName and rhsConstructorName replace string with actual object as value
• and then have consistent keys in “complex” and “basic” comparison object as lhs and rhs or even consider as terse as a and b following convention in jasmineUtils.ts and utils.js

[jeysal]

if it is, we continue checking their keys, otherwise we can mark them as UNEQUAL and move on.

But do you not want to mark them properly once we've finished recursing into them? If TYPE_EQUAL could mean EQUAL and UPDATED it e.g. won't allow you to collapse a part of the diff where you know everything is equal.

[pedrottimark]

@grosto @jeysal

• Maybe reverse the reasoning so TYPE_UNEQUAL (EDIT: or UNEQUAL_TYPE :) is value for non comparable complex values, like Object and typed array, for which you might short-circuit the comparison?
• An interesting case is comparison of basic versus complex value, when it seems like UPDATED (EDIT: or UNEQUAL_TYPE is a judgment call, not sure pro and con) and no need for propertyDiffs but the formatter might display properties of the complex value (this is evidence why the object instance is needed as a value in comparison object)
• In my rough prototyping, a complex comparison function kept local propertyDiffs array, but returned it as property value only if diffResult was UPDATED not for EQUAL

These cases are evidence against what I suggested about absence of propertyDiffs as a hint ;)

When no relevant prop diffs for complex value, possible formatting convention could include ellipsis:

• {…} for plain object or […] for plain array
• MyClass {…} for derived class or Int32Array […] typed array

[SimenB]

Is this fixed now that we've landed #9257?

[grosto]

Hi,

The scope of this PR was intended to be larger than just asymmetric matchers, it's more about changing the jest's diff solution. Is this still something that jest's team considers doing? I have not had free time to contribute to open source for the past months, but I would love to come back to this problem. Also, I think this PR contains valuable context if somebody else would pick it up.

[SimenB]

it's more about changing the jest's diff solution. Is this still something that jest's team considers doing?

If it's improved/more usable, we're always open to it 🙂 It's at least been documented now: https://github.com/facebook/jest/blob/master/packages/jest-diff/README.md

---

Happy to keep this open, I just wondered if it might have been solved 👍

[grosto]

Hello,

I have finally found some time for this. Sorry for keeping it for so long. I have pushed partial implementation with tests and README.md. Will be waiting for the feedback.
There is a test file for compatibility with jest-diff too.

[grosto]

To clarify I am waiting for a feedback on this one to continue.

[jeysal]

I'm so sorry you haven't received feedback on this yet :(
I'm very interested in this topic (in fact I considered exploring an impl of this myself) but atm I'm barely getting my work on other things (the babel-plugin-jest-hoist rewrite, the prettier-less snapshots) done 😢
Please don't be discouraged by this, I will get to it sooner or later 🙃 and it's totally fine if you want to wait for feedback before proceeding of course.

[grosto]

Now that architecture is somewhat stable(at least API of diff object), I wanted to implement some new thing and also fix some skipped tests. I also observed some interesting properties/tradeoffs of diff and then serialize approach.

Let's start with the pros:
I implemented expect.objectContaining and you can see the differences(the top one is old diff):

const a = {
      payload: {a: 'a', b: 'b', c: 'c', d: 'd'}, 
      type: 'whatever'
};
const b = {
    payload: expect.objectContaining({
        a: 'x',
        b: expect.any(String),
    }),
    type: 'whatever',
 };

This is field updated example. Inserted, Deleted and Unequal Type fields all have big improvements too.

We can also correctly understand inverse matches

const a = {
  payload: {a: 'a', b: 'b', c: 'c', d: 'd'}, 
  type: 'whatever'
};

const b = {
  payload: expect.not.objectContaining({
    a: 'x',
    b: expect.any(String),
  }),
  type: 'whatever',
};

const a = {
  payload: expect.not.objectContaining({
    a: 'a',
    b: expect.any(String),
  }),
  type: 'whatever',
};

const b = {
  payload: {a: 'a', b: 'b', c: 'c', d: 'd'},
  type: 'whatever',
};

I also expect to see the big improvements on arrayContaining too. Have to implement it first.

Downsides:
Map diff is harder to implement because keys can have diffs too not just equality. So it means that we have to compare each key in the map to other keys until we find the closest match. For now, I am only comparing two keys with Map.has().
It's easier for old diff to do this because just looks for differences in strings and it does not have any notion of data structures. To make it visual

const a = new Map([[{a: 1}, 2]]);
const b = new Map([[{a: 2}, 2]]);

Currently, I am not traversing the complex keys of the map, so formatter cannot display them properly. That's easily solvable. Bigger complexity is the diff algorithm.

The second downside is easier to see than to explain. I am not even sure if this is a downside, but there are some similarities that old diff picks up that we cannot pick up(easily at least)

const a = {
  searching: '',
  whatever: {
    a: 1,
  },
};

const b = {
  searching: '',
  whatever: [
    {
      a: 1,
    },
  ],
};

Let me know what you think and I will continue working on this.
I still have to implement the remaining asymmetricMatchers, DOM and Set. Improve Map. Add back React Element and finally add the same options as old diff had

[codecov-commenter]

Codecov Report

Merging #7893 (5b1daef) into master (2abd495) will increase coverage by 0.59%.
The diff coverage is 80.54%.

@@            Coverage Diff             @@
##           master    #7893      +/-   ##
==========================================
+ Coverage   64.19%   64.79%   +0.59%     
==========================================
  Files         308      323      +15     
  Lines       13519    14033     +514     
  Branches     3293     3443     +150     
==========================================
+ Hits         8678     9092     +414     
- Misses       4126     4191      +65     
- Partials      715      750      +35     

Impacted Files	Coverage Δ
packages/jest-deep-diff/src/complex/utils.ts	50.00% <50.00%> (ø)
packages/jest-deep-diff/src/complex/map.ts	56.09% <56.09%> (ø)
packages/jest-deep-diff/src/complex/array.ts	61.70% <61.70%> (ø)
packages/jest-deep-diff/src/getType.ts	65.62% <65.62%> (ø)
...ackages/jest-deep-diff/src/normalizeDiffOptions.ts	77.77% <77.77%> (ø)
...es/jest-deep-diff/src/plugins/asymmetricMatcher.ts	78.12% <78.12%> (ø)
packages/jest-deep-diff/src/complex/object.ts	79.16% <79.16%> (ø)
packages/jest-deep-diff/src/index.ts	81.81% <81.81%> (ø)
packages/jest-deep-diff/src/diffObject.ts	86.66% <86.66%> (ø)
packages/jest-deep-diff/src/diff.ts	86.95% <86.95%> (ø)
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2abd495...5b1daef. Read the comment docs.

[jeysal]

I think the second 'downside' is totally okay, we don't need to show similarities that happen to be somewhere deep down in the structure even if what's wrapping them is totally different.
The map thing is annoying indeed, I totally didn't have it in my mind that you could have these kinds of keys to a map...

One thing struck me as odd:

In this diff, the new impl generates a 'no visual difference'? Isn't that less than ideal?

[grosto]

They are equal so there is no difference to show. Wording could be changed. I copied it the old diff, I think.

[jeysal]

Are they? It's expecting an object not containing b: any string, and getting an object with b: 'b' isn't it?

[grosto]

I even check in the test that they are equal.
https://github.com/facebook/jest/pull/7893/files#diff-93c714843afcedbd3f472f3dc8da523e81a2c23630a6e2e299fa80c4423aaac1R314

I get where you are coming from. I don't think that's how objectContaining behaves. It just checks if one object is subset of another.

[jeysal]

Ah right yeah. Sorry. We even had a debate or some changes around how .not.objectContaining works, but apparently I had the wrong idea in my mind about how it works now after all 😅

[github-actions]

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

[SimenB]

@grosto @jeysal hi! are you still interested in landing this PR? I've merged in main, but I don't know what else is missing

[jeysal]

Funny, just yesterday I was thinking of this because I was myself experimenting with deep diffing algorithms.
I think this is still in the top 5 reworks that Jest needs, along with config, ESM, faster default transforms, etc.

I mean, LOL at this dumb diff that obviously knows nothing but string diffing:

Concordance (AVA) gets these cases better already for a long time.

---

Integrating it into expect, pretty-format, jest-diff as a behind the scenes implementation, that is I think where the majority of the remaining effort lies.
I would also want to hammer some fast-check tests at all the matchers using new and old impl to be really sure that passes and fails are unchanged (only formatting/diff should be rightfully changed).

In terms of implementation IIRC this was looking pretty good to me the last time I looked at it.
Since it is not yet actually used (it only implements the new package) there is little harm in merging it. Yes it will be dead code, sort of, for now. But if we ever continue this rework, this should be the code that we use.

[jeysal]

As you know @SimenB I've been completely inactive on Jest this entire year, and aside from a blog post mostly inactive in 2021 too.

I'm currently taking a couple months break from work traveling etc. I've been thinking about what I want to do next. One of my thoughts was whether we could utilize some of that OpenCollective money to actually "employ" me, either just for a while until I start a more "classic" job again, or part-time next to some other job, so that we can get some of these big projects moving, get the issue tracker under control, etc.

Some food for thought...in about a month or so I will probably have to decide what to do next.

[SimenB]

Oooh, I think it's a great idea if we could get somebody on top off the repo on a more consistent basis than my "I do what seems interesting, sometimes"! Let's discuss on discord

[github-actions]

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

[github-actions]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.