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

expect: Avoid incorrect difference for subset when toMatchObject fails #9005

Merged
merged 5 commits into from Oct 17, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Oct 3, 2019

Summary

Fixes #8947

When the getObjectSubset function copies the relevant subset of object properties recursively, it creates ordinary Object instances. If expected and received values are a subclass or typed array, then the type change of the received subset becomes an incorrect change in the difference when toMatchObject fails.

If corresponding values:

  • are subset match, then return the expected value instead of a copy of the received value
  • are not subset match, then incorrect type difference is still possible

Although the function returns an unnecessary copy for arrays, I left it as is, because the map method returns the same array class as the subset

Background information:

  • toMatchObject does not require same type: like toEqual and unlike toStrictEqual
  • Array.isArray returns false for typed arrays

Test plan

Separate tests for deep equality versus referential identity of returned subset

Add 3 new tests which failed before the code change

See also pictures in following comments: baseline is at left and improved is at right

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Oct 3, 2019

Here is minimal reproduction of reported problem for Uint8Array object type

both expected and received are Uint8Array with equal properties:

Uint8Array both

only expected is UintArray8 and received is Object with equal properties:

Uint8Array expected

only received is UintArray8 and expected is Object with equal properties was correct:

Uint8Array received

The example above was correct, but now avoids unnecessary copy of matching subset

P.S. change counts would be nice in these side-by-side comparisons, heh?

@pedrottimark
Copy link
Contributor Author

Here is another minimal reproduction for object subclass

only expected is SubObject and received is Object with equal properties:

SubObject expected

only received is SubObject and expected is Object with equal properties:

SubObject received

The example above was correct, but now avoids unnecessary copy of matching subset

@pedrottimark
Copy link
Contributor Author

Here is demonstration that the difference is correct for array subclass, because even if a copy in unnecessary, the map method returns the correct subclass of the subset

only expected is SubArray and received is Array with equal properties

SubArray expected

only received is SubArray and expected is Array with equal properties

SubArray received

@codecov-io
Copy link

codecov-io commented Oct 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@fa0eb6d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #9005   +/-   ##
========================================
  Coverage          ?   63.9%           
========================================
  Files             ?     277           
  Lines             ?   11654           
  Branches          ?    2860           
========================================
  Hits              ?    7448           
  Misses            ?    3576           
  Partials          ?     630
Impacted Files Coverage Δ
packages/expect/src/utils.ts 94.93% <100%> (ø)

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 fa0eb6d...7414ae4. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test names could be a bit clearer IMO, other than that LGTM! :)
(sorry for the review delay)

packages/expect/src/__tests__/utils.test.js Outdated Show resolved Hide resolved
packages/expect/src/__tests__/utils.test.js Outdated Show resolved Hide resolved
Co-Authored-By: Tim Seckinger <seckinger.tim@gmail.com>
@pedrottimark
Copy link
Contributor Author

@jeysal Your critique is worth waiting for, my friend.

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Oct 16, 2019

I will merge changes from master to see if #9030 solves test-node-12 failure

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visual diff for functions is broken in conjunction with typed arrays
4 participants