Jest: `toHaveBeenCalledWith` `objectContaining` issue after patch update 26.6.1

[scagood]

🐛 Bug Report

Tests that contain Buffers in expect.objectContaining calls started failing after a patch version update.

To Reproduce

A private repo received a dependabot PR for the following update: Bump jest from 26.6.0 to 26.6.1

Previously this test would have passed:

test('issue demo', async () => {
    const func = jest.fn();

    func({ test: Buffer.from('test'), some: 'thing' });

    expect(func).toHaveBeenCalledWith(
        expect.objectContaining({ test: Buffer.from('test') }),
    );
});

That test now throws:

   TypeError: Cannot set property parent of [object Object] which has only a getter

     4 |     func({ test: Buffer.from('test'), some: 'thing' });
     5 |
   > 6 |     expect(func).toHaveBeenCalledWith(
       |                  ^
     7 |         expect.objectContaining({ test: Buffer.from('test') }),
     8 |     );
     9 | });

     at Object.<anonymous> (buffer-issue.test.js:6:18)

Expected behavior

Tests to not start failing after a patch update :sweat_smile:

envinfo

Before:

 $ npx envinfo --preset jest
npx: installed 1 in 0.642s

  System:
    OS: Linux 5.3 Ubuntu 19.10 (Eoan Ermine)
    CPU: (24) x64 AMD Ryzen Threadripper 2920X 12-Core Processor
  Binaries:
    Node: 12.19.0 - ~/.nvm/versions/node/v12.19.0/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v12.19.0/bin/npm
  npmPackages:
    jest: ^26.5.3 => 26.6.0 

After:

 $ npx envinfo --preset jest
npx: installed 1 in 0.681s

  System:
    OS: Linux 5.3 Ubuntu 19.10 (Eoan Ermine)
    CPU: (24) x64 AMD Ryzen Threadripper 2920X 12-Core Processor
  Binaries:
    Node: 12.19.0 - ~/.nvm/versions/node/v12.19.0/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v12.19.0/bin/npm
  npmPackages:
    jest: ^26.5.3 => 26.6.1 

[SimenB]

I think this is a duplicate of #10689. Can you attempt to apply the fix from #10711 and see if it fixes it?

[scagood]

Thanks for the speedy response!
I updated the built package to match

But alas this does not seem to have fixed it

$ npx jest --no-coverage buffer-issue.test.js
 FAIL  ./buffer-issue.test.js
  ✕ issue demo (5 ms)

  ● issue demo

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: ObjectContaining {"test": {"data": [116, 101, 115, 116], "type": "Buffer"}}
    Received: {"some": "thing", "test": {"data": [116, 101, 115, 116], "type": "Buffer"}}

    Number of calls: 1

      4 |     func({ test: Buffer.from('test'), some: 'thing' });
      5 | 
    > 6 |     expect(func).toHaveBeenCalledWith(
        |                  ^
      7 |         expect.objectContaining({ test: Buffer.from('test') }),
      8 |     );
      9 | });

      at Object.<anonymous> (buffer-issue.test.js:6:18)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        0.632 s, estimated 1 s
Ran all test suites matching /buffer-issue.test.js/i.

[SimenB]

Right, so the weird error is gone, but the assertion doesn't work anymore. We should fix that!

/cc @jeysal @ninevra

[scagood]

Would the solution here be something along the lines of only recursively check objects and arrays that are vanilla objects/arrays?

I'm not 100% sure on the best way to even do that, this is what I came up with:

const expected =
  typeof this.sample[property] === 'object' &&
  (
    Object.getPrototypeOf(this.sample[property]) === Object.prototype ||
    Object.getPrototypeOf(this.sample[property]) === Array.prototype
  )
    ? objectContaining(this.sample[property] as Record<string, unknown>)
    : this.sample[property];

[SimenB]

If that passes the tests I'm happy 🙂 Send a PR?

If it doesn't work we might have to revert #10508

[jeysal]

Repost from #10720:
Is this still any different from what toMatchObject does? I think it's the same now? In that case, this asymmetric matcher is just a matter of

const pass = equals(received, expected, [iterableEquality, subsetEquality]);

and we'd be re-using existing battle-proven code for this...

[ZachMayry]

Experiencing the same error. Apparently our test didn't even need expect.objectContaining so it was easily fixable.

[ninevra]

@jeysal

Repost from #10720:
Is this still any different from what toMatchObject does? I think it's the same now? In that case, this asymmetric matcher is just a matter of

const pass = equals(received, expected, [iterableEquality, subsetEquality]);

and we'd be re-using existing battle-proven code for this...

That's complicated. As far as I can tell: 1) no, that's not equivalent; 2) yes, that passes all existing tests, more or less (implying that a large number of cases are not covered by current tests); and 3) I think that does solve this issue's particular bug, so if they _were_ equivalent, reimplementing in terms of equals() would close this issue.

One difference between toMatchObject and objectContaining is in their handling of Arrays. toMatchObject requires that two Arrays have the same length, i.e. expect([1,2,3]).toMatchObject([1,2]) throws. objectContaining currently treats Arrays like any other object, and recurses on their enumerable properties. Since length is not enumerable, expect([1,2,3]).toEqual(expect.objectContaining([1,2]) does not throw. Whether this behavior is desired or accidental is not clear to me.

Similar differences should arise (though I have not tested this) wherever equals() special-cases a type of object, for example Date and RegExp. objectContaining appears to hijack handling for all these types.

These differences existed prior to #10508 (probably since the original implementation of objectContaining()), but they affected only the value directly compared to the matcher, making them somewhat intuitive. [1] is an object and contains the property "0" with value 1, so it could be expected to match objectContaining({0: 1}) (or even objectContaining({length: 1})). However, since #10508, this behavior is applied recursively, to every typeof "object" child of the sample, meaning that objectContaining()'s comparisons no longer substantively resemble those used elsewhere in jest.

Another difference is that objectContaining(null) matches all values, just like objectContaining({}), because null is an object by typeof and has no enumerable properties. This dates back probably all the way to the original implementation. By contrast, toMatchObject(null) throws with Matcher error: expected value must be a non-null object.

There may be other differences that I didn't catch.

On master, reimplementing in terms of equals() passes all tests for objectContaining, but not those for not.objectContaining, due to the later's undocumented behavior; on top of #10708, it passes all tests for both.

Also passes the cross-realm input test from https://github.com/ninevra/jest/commit/0553005ae367b20e2fe5c670e65c67c0e4fa5f6b (though that's hardly exhaustive).

It fails @scagood's tests for recursing into Buffers (https://github.com/facebook/jest/pull/10720/commits/6ff06e6392ecef8da941f6b9ccc4432b603f8c9c), but only because they rely on the existing behavior with respect to Arrays; I _think_ reimplementing in terms of equals() does actually fix the problem with Buffers, but it remains unclear to me how or why that is.

@SimenB @jeysal What's the route forward?

[jeysal]

TBH, and I've said this to @SimenB a couple of days ago, I think we should revert https://github.com/facebook/jest/pull/10508 and change the documentation instead. It was merged without discussion and to implement it properly would require something as complex as our subset equality check, except different in a variety of ways. I think it's totally reasonable for objectContaining to have a different use case than toMatchObject work without any recursion - 'object containing' to me sounds like a flat list of properties being checked, and I'm used to writing constructs like objectContaining({arr: arrayContaining([42])).

[SimenB]

Yeah, I'm leaning towards reverting and perhaps updating the docs to be explicit about depth

[SimenB]

Will revert tomorrow (Monday), /cc @ioancole

[ioancole]

Will revert tomorrow (Monday), /cc @ioancole

Sure, seems like this change was a lot more involved that I initially expected.

Thanks @scagood @ninevra @jeysal for looking into this. Looks like reverting the original PR #10508 and changing the docs is the best thing to do for now.

[SimenB]

Revert released in https://github.com/facebook/jest/releases/tag/v26.6.2

[drudv]

I spend literally a day trying to figure out why my tests failing. At the end of the day I found strange behaviour in other places was caused by

expect(request).toHaveBeenCalledWith(expect.objectContaining(...));

I checked out the latest changes in Jest and Voila! Someone has fixed it today in 26.6.2. Thank you! I can finally go to sleep :)