Correct highlighting for asymmetric matchers

[grosto]

Summary

I've opened this PR for https://github.com/facebook/jest/issues/6184 and https://github.com/facebook/jest/issues/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 https://github.com/facebook/jest/issues/7027#issuecomment-423837270.

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 https://github.com/facebook/jest/issues/8295#issuecomment-483157466

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 :cry: Please don't be discouraged by this, I will get to it sooner or later :upside_down_face: and it's totally fine if you want to wait for feedback before proceeding of course.

[grosto]

No worries. Thanks for replying.

The rest of the work will be based on this foundation and I want to make sure it's a good one before I continue. So I would rather wait for feedback for now.

[mattvb91]

I think this would currently help me figuring out a diff I cant make heads or tails off due to it currently outputting the whole objects instead of the actually difference between them.

[jeysal]

I will get to it sooner or later

Yeah this was later rather than sooner. Fortunately, as this is a new package intended to completely replace existing ones, it does not get out of date :see_no_evil: So, @pedrottimark has not been seen on Jest for a while, but maybe we can get this over the line still? The code is obviously a lot and some pasted so it's hard to review without anyone working full-time on Jest other than scanning for obvious open points, but I suppose if we can just do a run of the expect toStrictEqual test values on this and make sure every non-equal value pair generates a diff on this and every equal pair generates no diff, then that's probably a good enough indicator that it's solid. Plus maybe some of us can link it into some Jest work projects, use it for a while. This is just way too much work and way too important to leave hanging like this, and the DiffObject structure looks like exactly what we need :see_no_evil: wdyt @SimenB

[grosto]

Yeah this was later rather than sooner.

No worries. I am here still to get this to the finish line, I was just not sure if there was still interest. diffComp.test.ts runs some tests to make sure that the behavior of this diff and old diff match in most cases. I understand that there is a lot of code did you manage to read README file? it describes what's missing and how everything is currently implemented. Let me know if you have any immediate feedback from reading it.

[codecov-io]

Codecov Report

Merging #7893 (7538026) into master (b3f9e4a) will increase coverage by 0.38%. The diff coverage is 75.54%.

@@            Coverage Diff             @@
##           master    #7893      +/-   ##
==========================================
+ Coverage   64.24%   64.63%   +0.38%     
==========================================
  Files         308      323      +15     
  Lines       13502    13960     +458     
  Branches     3289     3412     +123     
==========================================
+ Hits         8675     9023     +348     
- Misses       4117     4201      +84     
- Partials      710      736      +26     

Impacted Files	Coverage Δ
packages/jest-deep-diff/src/complex/map.ts	3.33% <3.33%> (ø)
packages/jest-deep-diff/src/complex/utils.ts	50.00% <50.00%> (ø)
packages/jest-deep-diff/src/getType.ts	59.37% <59.37%> (ø)
packages/jest-deep-diff/src/complex/array.ts	70.21% <70.21%> (ø)
packages/jest-deep-diff/src/complex/object.ts	77.08% <77.08%> (ø)
...es/jest-deep-diff/src/plugins/asymmetricMatcher.ts	77.27% <77.27%> (ø)
packages/jest-deep-diff/src/diff.ts	77.77% <77.77%> (ø)
...ackages/jest-deep-diff/src/normalizeDiffOptions.ts	77.77% <77.77%> (ø)
packages/jest-deep-diff/src/primitives.ts	78.75% <78.75%> (ø)
packages/jest-deep-diff/src/index.ts	81.81% <81.81%> (ø)
... and 21 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 b3f9e4a...7538026. Read the comment docs.

[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.