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

Immutable.js equality #3574

Closed
philipp-spiess opened this issue May 15, 2017 · 4 comments · Fixed by #3651
Closed

Immutable.js equality #3574

philipp-spiess opened this issue May 15, 2017 · 4 comments · Fixed by #3651

Comments

@philipp-spiess
Copy link
Contributor

philipp-spiess commented May 15, 2017

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

The following test will unexpectedly fail because Immutable.js sometimes creates a new reference, although the value is the same. Look at this test for example:

it('works with immutable', () => {
  const directlyCreated = new Immutable.Map([['foo', 'bar']]);
  const indirectlyCreated = (new Immutable.Map()).set('foo', 'bar');
  const fn = jest.fn();
  fn(directlyCreated, indirectlyCreated);

  expect(fn).toHaveBeenCalledWith(indirectlyCreated, directlyCreated);

  expect(() =>
    expect(fn).not.toHaveBeenCalledWith(indirectlyCreated, directlyCreated),
  ).toThrowError();
})

Although directlyCreated and indirectlyCreated have the same content and are treated the same by Immutable.js, the reference of the objects differ since one is created directly and another one is created by modifying an empty map. This errors with a very strange message:

Expected spy to have been last called with:
  [Immutable.Map {foo: "bar"}, Immutable.Map {foo: "bar"}]
But it was last called with:
  [Immutable.Map {foo: "bar"}, Immutable.Map {foo: "bar"}]

What is the expected behavior?

Our matches should be aware of Immutable.js objects and use Immutable.is or xyz#equals() in that case.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.

This happens in the current master version and happens because the eq method taken from jasmine does not support Immutable.is for comparison. Now that we control those matchers, we might add support for that.

If this is something we want to support, I'd love to work on it!

Edit: I run into this a lot and whenever I do, I have to use fn.mock.calls[0][0] to do the comparison, which, when using the toEqual matcher, strangely works.

Edit2: Can it be that the matchers for toHaveBeenCalledWith compare the array of arguments using equals() and since this is a regular type, it uses a different code path then, when equals() is called with the Immutable.js type directly?

@anilreddykatta
Copy link
Contributor

@philipp-spiess I guess we can add custom matchers like toHaveBeenCalledWithImmutable(map1, map2).

\\ Pretty hacky way to do it
expect.extend({
  toHaveBeenCalledWithImmutable(actualFn, expected) {
    const actual = actualFn.mock.calls
    const pass = actual.every((actualCall, callIndex) => {
      return actualCall.every((actualParam, paramIndex) => {
        return actualParam.equals ? actualParam.equals(expected[callIndex][paramIndex]) : actualParam === expected[callIndex][paramIndex]
      })
    })
    const message = pass
      ? `expected ${actual} to be not equal to ${expected}`
      : `expected ${actual} to be equal to ${expected}`;

    return {message, pass};
  }
})

I guess we can have extensions to jest matchers. If jest supports .equals that would be awesome 👍

@thymikee
Copy link
Collaborator

What about creating a node module supporting that first? As soon as it gets stable enough we could merge it into Jest

philipp-spiess added a commit to philipp-spiess/jest that referenced this issue May 25, 2017
While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.
philipp-spiess added a commit to philipp-spiess/jest that referenced this issue May 25, 2017
While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.
@apaatsio
Copy link

There is already this project: https://github.com/unindented/custom-immutable-matchers

It could be just extended with toHaveBeenCalledWithImmutable

philipp-spiess added a commit to philipp-spiess/jest that referenced this issue Jun 12, 2017
While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.
philipp-spiess added a commit to philipp-spiess/jest that referenced this issue Jun 27, 2017
While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.
cpojer pushed a commit that referenced this issue Jun 27, 2017
* Use iterableEquality in spy matchers

While working on custom matchers to solve #3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.

* Add spy matchers tests using ES6 Map and Set
tushardhole pushed a commit to tushardhole/jest that referenced this issue Aug 21, 2017
* Use iterableEquality in spy matchers

While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.

* Add spy matchers tests using ES6 Map and Set
@github-actions
Copy link

This issue 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 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants