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

False equality with Immutable Records #231

Closed
dclowd9901 opened this issue Sep 12, 2017 · 2 comments
Closed

False equality with Immutable Records #231

dclowd9901 opened this issue Sep 12, 2017 · 2 comments

Comments

@dclowd9901
Copy link

dclowd9901 commented Sep 12, 2017

Recently upgraded to Jest 21.0.2, and some equality tests relating to comparing Immutable Records started failing.

I was able to trace the problem back to this library, in the utils.js file for iterableEquality.

It turns out that iterable equality falsely identifies Immutable Records as iterables (likely because of this line:

  if (a.size !== undefined) { // Immutable Records also have a size prop

since IRs (Immutable Records) have a size property.

It then constructs a new object out of the properties of the compared objects (by combining them in an array and passing that array to the constructor of the a property), but merely compares the size of the newly constructed Record to the old a record and nothing else. It seems that when constructing a new Record out of the merged properties, Immutable uses the first reference listed in the array, not the second, to build the Record.

That means that if you have two Immutable Records of any kind that have the same properties, (nevermind the contents of those properties), this test will erroneously pass and not bother to look deeper in the records.

The real concern here is that the duck-typing utilized in iterableEquality is brittle, and subject to whether or not two constructed objects have a size param, and how they construct themselves.

Workaround: Convert Immutable Records to plain JSON objects and compare those instead, or stringify them and do the same.

@ljharb
Copy link
Collaborator

ljharb commented Sep 12, 2017

Issues with expect v21+ need to be filed in the jest repo.

@ljharb ljharb closed this as completed Sep 12, 2017
@dclowd9901
Copy link
Author

Moved to jestjs/jest#4472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants