Jest 25 invokes setters during toEqual comparison, causing side effects

[gaearon]

This is similar to #9531.

It is not safe to assign a bunch of setters in the passed object. They may throw or they may have side effects. This isn't theoretical because we may add setters like this to React.

Example: https://repl.it/repls/HuskyCandidConfiguration

test('setter object comparison', () => {
  expect({
    get a() {
      return {};
    },
    set a(value) {
      throw new Error('noo');
    },
    b: {},
  }).toEqual({ 
    a: {},
   });
});

This used to work fine in 24: https://repl.it/repls/AllMediumorchidDeals

This regression is currently blocking upgrading React to Jest 25.

[SimenB]

I'm guessing we should just try-catch attempting to override, and if we can't do that just keep going leaving the property as it is? We're just trying to give a nicer diff, if we can't do that we should bail and leave things alone. One downside of that is that then we won't know we failed so can't fix it, but probably worth it. Unless we think this is the only case we haven't covered, which seems optimistic

/cc @WeiAnAn @jeysal @pedrottimark

[gaearon]

It’s not just about catching errors. This setter could perform a side effect. For example

set x(value) {
  renders++;
  _x = value
}

Then merely asserting would change the state of the program.

It’s not safe to call in the first place.

[SimenB]

Right, good point. We should just skip trying to change anything that has a setter then? Would that work?

[SimenB]

@gaearon opened up #9757, would love your thoughts on it

[gaearon]

Can you give me an overview of why we're mutating user-passed objects in the first place? I read the original PRs but I struggle to understand how this relates to cloning or assertion messages.

[gaearon]

I think ultimately there are two constraints:

• None of user code should run
• None of user objects should be mutated

Is this a guarantee we can provide?

[gaearon]

I guess it's expected that getters would run though. So that's an exception.

[SimenB]

We don't mutate the actual object passed in - we make a deep copy and mutate that copy. The reason is that we want to replace the properties that use asymmetric matchers and pass with the value, so that it's not highlighted as failing in the diff view.

It's not an optimal solution, ideally we'd have a smarter object diffing than a full serialization then comparing strings

[gaearon]

What if you have a matcher on a property with a setter? How would that work?

I think maybe to fix this, you'd need to turn setters into "regular" properties during cloning instead of cloning setters and then ignoring them.

[SimenB]

Ooh, just dropping the setter from the descriptor sounds good. We never want to run them anyways, as we only do this stuff after an assertion has failed and we generate a diff

[SimenB]

Pushed that up

[github-actions]

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.