mobx-react 6 shallow testing regression

[sergei-startsev]

What is the current behavior?

With mobx-react 6 upgrade regressions are detected in unit tests:

// Foo.jsx
class Foo extends React.Component {
  //...
  render() {
    return <div>Test</div>;
  }
}
export default inject('store')(observer(Foo));

//Foo.test.jsx
describe('Foo', () => {
  test('shallow', () => {
    const component = shallow(<Foo.wrappedComponent />);
    expect(component).toMatchSnapshot();
  });
})

Snapshot:

<Observer>
  <Component />
</Observer>

Here's the repository with repro steps: https://github.com/sergei-startsev/mobx-react-6-regression

What is the expected behavior?

Expected snapshot:

<div>
  Test
</div>

---

I believe the issue is caused by added <Observer> wrapper:

mobx-react/src/observer.js

Line 108 in 4904b62

return <Observer>{() => baseRender.apply(undefined, arguments)}</Observer>

See 1ea40e2#diff-08f3e9ab539253c87ea111cc61c69e29R156.

[danielkcz]

This is one of the documented changes. You have to update your snapshots, it's not a regression.

https://github.com/mobxjs/mobx-react/blob/master/CHANGELOG.md#600

For observer based components, there will now be an additional Observer component in the tree.

[sergei-startsev]

@FredyC These snapshots are useless:

<Observer>
  <Component />
</Observer>

[sergei-startsev]

At the moment shallow rendering doesn't provide any useful results when testing observable components, only the <Observer> is rendered. So how can I setup shallow testing with recent changes?

[danielkcz]

Ok sorry, I was too quick on close here. I will leave it open for someone else to chime in.

Personally, I don't use shallow rendering. It's kinda frowned-upon practice anyway because it makes the test more fragile. Testing a whole component tree makes it easier as you don't need to unit test each tiny component, just test the bigger ones that matter to user. Do yourself (and your team) a service and buy it ... https://testingjavascript.com/

[sergei-startsev]

Enzyme team disagree:

Shallow rendering is useful to constrain yourself to testing a component as a unit, and to ensure that your tests aren't indirectly asserting on behavior of child components.

Testing a whole component tree makes tests fragile since any changes in child components affect your test while unit testing is about testing a module itself, not its dependencies.

[danielkcz]

Well, that's why I don't use Enzyme 😆 That would be a long talk and it's definitely the wrong place to debate over testing practices. In short, unit testing is overrated, integration tests bring much better value overall.

For now, I would advise you to stick to V5 unless V6 is solving something else for you.

[urugator]

Not familiar with Enzyme, checked the API briefly, would this work?

const component = shallow(<Foo.wrappedComponent />).find(Observer).dive();
expect(component).toMatchSnapshot();

EDIT: hm... you actually need to dive into the original unexported component, rather than Observer right?

[sergei-startsev]

@urugator
Yes, you'll get a snapshot of your component, not Observer wrapper. Since you can pass component’s displayName as a selector, the following example will also work:

const component = shallow(<Foo.wrappedComponent />).find('Observer').dive();
expect(component).toMatchSnapshot();

Snapshot:

<div>
   Test
</div>

But I'm not sure if it's a good idea to let unit tests know anything about the Observer and its internal tree. I'm also not sure that changing existing tests is an option here though...

[mweststrate]

I think this issue is another good argument to revert to the old "mixin" based observer implementation for classes?

[mweststrate]

Fixed by 6.1.0

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.