Material-ui: Material UI components complicate testing of other (general) components significantly

[bingomanatee]

Description

Remove the dependency on muiTheme

The fact that UI components constantly require a theme context makes general testing significantly more complex.

Steps to reproduce

Here is a sample test not focused on MUI components.

import React from 'react'
import { Header } from 'components/Header/Header'
// import classes from 'components/Header/Header.scss'
// import { IndexLink, Link } from 'react-router'
import { shallow } from 'enzyme'
import muiTheme from 'themes/base';

describe('(Component) Header', () => {
  let _wrapper;

  beforeEach(() => {
    _wrapper = shallow(<Header />, {context: {muiTheme}});
  });

  describe('Navigation links...', () => {

    it('Should render a logo header', () => {
      expect(_wrapper.text()).to.equal('<IndexLink /><Connect(LoginButtonContainer) /><Connect(EatTwitterButtonContainer) />');
    });
  });
});

Consistently having to inject themes where they are not relevant for testing adds a significant overhead to the process. If the MUI classes could just neutralize style rendering where no theme is present the components would have a lot less overhead in the testing context.

Dave

[nathanmarks]

@bingomanatee The example above works for me without putting a theme on context, what error are you getting?

[nicolasiensen]

@nathanmarks I agree that the example above should work, perhaps what @bingomanatee was pointing out was that we should not have to worry about passing MUI theme in the context when testing components that relies on MUI

[nathanmarks]

@nicolasiensen you don't need to unless you're rendering an entire component tree

[bingomanatee]

the above example works; however, to have to put a muiTheme in context for EVERY unit test is a bit of a chore, when the point of the tests have nothing to do with the muiTheme. That also means, for instance that all the expectations have to look for the style node that muiTheme produces, further cluttering the test code.

My proposal is that when a mui class finds that there is no muiTheme in context, no style node should be produced at all. Its not likely to happen in production and would streamline testability a lot.

[nathanmarks]

@bingomanatee you don't need to put muiTheme in context for every unit test

[nathanmarks]

have you got a complete example of the issue with unit tests?

[nathanmarks]

component

import React, { Component } from 'react';
import RaisedButton from 'material-ui/RaisedButton';

export default class Home extends Component {

  render() {
    return (
      <div>
        <img src="/assets/images/logo.svg" />
        <h1>Helloooo</h1>
        <RaisedButton label="woof" />
      </div>
    );
  }
}

test

/* eslint-env mocha */
import React from 'react';
import { assert } from 'chai';
import { shallow } from 'enzyme';
import Home from './Home';

describe('Home', () => {
  it('should render', () => {
    const wrapper = shallow(<Home />);
    assert.ok(wrapper);
  });
});

result

> cross-env NODE_ENV=test babel-node test/index.js


  Home
    ✓ should render


  1 passing (12ms)

[oliviertassinari]

@bingomanatee I'm not sure to understand what the actual issue is.
I feel like the most actionnable thing than we can do is to put a note on the documentation regarding integration testing.
For unit tests you shouldn't need to worry about the actual implementation using context or not.

[nathanmarks]

@oliviertassinari In next, people will be able to use this:

https://github.com/callemall/material-ui/blob/next/test/utils/createMountWithContext.js

I'm wondering if that, (and the shallow wrapper in another file) should babelified and published under material-ui-test-utils. Or just published in a folder material-ui/test-utils

[Szarko]

@nathanmarks - Use mount not shallow in your example and it fails.

[nathanmarks]

@Szarko the example was for shallow, see my last reply RE mount as it applies to the next branch

[acpower7]

^Thanks. But can you offer a solution to this branch? How do you resolve the context muitheme issue (with mount)?

[gfarrell]

I'm having this problem having updated MaterialUI, for example in List.js you have

const {prepareStyles} = this.context.muiTheme;

This breaks my tests unless I include some muitheme in the context, which is itself extraneous to the tests.

[igorrmotta]

It solves the problem.

childContextTypes:{
  muiTheme: React.PropTypes.object.isRequired,
},
getChildContext() {
  return {muiTheme: getMuiTheme(baseTheme)};
},

I don't like to have this boilerplate code into all my components. But, it is what it is.

[oliviertassinari]

Or just published in a folder material-ui/test-utils

@nathanmarks I really like that idea! I think that I will continue my documentation improvement effort on the next branch with a Writing Tests section. I like the way redux is documenting it.

[BelfordZ]

Could we please have material-ui not throw if you haven't wrapped your components in a theme?

this is pretty much a deal breaker for using material-ui. Im not adding any code to my components to make material-ui work in tests. Nor am I interested in having boiler plate in my tests.

[Valentin-Seehausen]

Yap, that is unnecessary. If you mount a Component, Material-UI throws an error.

[Valentin-Seehausen]

Just for people that need a fast solution: http://stackoverflow.com/questions/38264715/how-to-pass-context-down-to-the-enzyme-mount-method-to-test-component-which-incl

But the issue remains: This is actually unnecessary boilerplate.

[PolGuixe]

The approach above doesn't work when using:

import muiThemeable from 'material-ui/styles/muiThemeable';

To inject the muiTheme as a property to the component.

If you need to access muiTheme from inside the component you need to do it through the context.

@oliviertassinari mentioned that using muiThemeable is recommend over context.

[PolGuixe]

In my case:

Component to test:

import React, { PropTypes } from 'react';
import _ from 'lodash';
import { List, ListItem, Avatar, Subheader } from 'material-ui';
import muiThemeable from 'material-ui/styles/muiThemeable';

const getDefaultStyles = palette => ({
  price: {
    backgroundColor: 'none',
    color: palette.textColor,
  },
});


class CartList extends React.PureComponent {
  // FIXME not recomended, better to use muiThemeable, however not compatible with testing
  static contextTypes = {
    muiTheme: React.PropTypes.object.isRequired
  };

  render() {
    const { items, subheader } = this.props;

    const defaultStyles = getDefaultStyles(this.context.muiTheme.palette);

    return (
      <List>
        {subheader
          ? <Subheader>{subheader}</Subheader>
          : null}
        {_.map(items, (item, index) => (
          <ListItem
            key={index}
            primaryText={item.name}
            secondaryText={item.info}
            leftAvatar={<Avatar src={item.thumbnail} />}
            // TODO not use and avatar if we need to add more price information, discounts, etc
            rightAvatar={
              <Avatar
                backgroundColor={defaultStyles.price.backgroundColor}
                color={defaultStyles.price.color}
              >
                {item.price}
              </Avatar>
            }
          />
        ))}
      </List>
    );
  }
}

export default CartList;

Test:

import React from 'react';
import { shallow } from 'enzyme';
import chai, { expect } from 'chai';
import chaiEnzyme from 'chai-enzyme';
import { Subheader } from 'material-ui';
import getMuiTheme from 'material-ui/styles/getMuiTheme';

import CartList from '../CartList.jsx';

const { describe, it } = global;
chai.use(chaiEnzyme());

const muiTheme = getMuiTheme();
const shallowWithContext = node => shallow(node, {
  context: { muiTheme },
  childContextTypes: { muiTheme: React.PropTypes.object },
});


const productList = [
  {
    name: 'XXXX',
    info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
    thumbnail: '/images/default_caroussel.jpg',
    price: '29€',
  },
  {
    name: 'YYY',
    info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
    thumbnail: '/images/default_caroussel.jpg',
  },
  {
    name: 'ZZZ',
    info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
    thumbnail: '/images/default_caroussel.jpg',
  },
];

const subheader = 'subheader test';

describe('CartList', () => {
  it('should render with subheader', () => {
    const cartList = shallowWithContext(<CartList items={productList} subheader={subheader} />);
    expect(cartList).to.contain(<Subheader>{subheader}</Subheader>);
    expect(cartList).to.have.text(subheader);
  });
});

Error:

AssertionError: expected <CartList /> to have text 'subheader test', but it has '<List />' HTML: Not available due to: Cannot read property 'prepareStyles' of undefined

_disclaimer: I am new to testing, so I might be doing something else wrong_

[Valentin-Seehausen]

I think you are totally right.

But still this is overhead an some people want their tests to be as lean as
possible.

PolGuixe [e-mail] schrieb am So., 05.03.2017, 8:26:

In my case:

Component to test:

import React, { PropTypes } from 'react';
import _ from 'lodash';
import { List, ListItem, Avatar, Subheader } from 'material-ui';
import muiThemeable from 'material-ui/styles/muiThemeable';

const getDefaultStyles = palette => ({
price: {
backgroundColor: 'none',
color: palette.textColor,
},
});

class CartList extends React.PureComponent {
// FIXME not recomended, better to use muiThemeable, however not compatible with testing
static contextTypes = {
muiTheme: React.PropTypes.object.isRequired
};

render() {
const { items, subheader } = this.props;

const defaultStyles = getDefaultStyles(this.context.muiTheme.palette);

return (
  <List>
    {subheader
      ? <Subheader>{subheader}</Subheader>
      : null}
    {_.map(items, (item, index) => (
      <ListItem
        key={index}
        primaryText={item.name}
        secondaryText={item.info}
        leftAvatar={<Avatar src={item.thumbnail} />}
        // TODO not use and avatar if we need to add more price information, discounts, etc
        rightAvatar={
          <Avatar
            backgroundColor={defaultStyles.price.backgroundColor}
            color={defaultStyles.price.color}
          >
            {item.price}
          </Avatar>
        }
      />
    ))}
  </List>
);

}
}

export default CartList;

Test:

import React from 'react';
import { shallow } from 'enzyme';
import chai, { expect } from 'chai';
import chaiEnzyme from 'chai-enzyme';
import { Subheader } from 'material-ui';
import getMuiTheme from 'material-ui/styles/getMuiTheme';

import CartList from '../CartList.jsx';

const { describe, it } = global;
chai.use(chaiEnzyme());

const muiTheme = getMuiTheme();
const shallowWithContext = node => shallow(node, {
context: { muiTheme },
childContextTypes: { muiTheme: React.PropTypes.object },
});

const productList = [
{
name: 'XXXX',
info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
thumbnail: '/images/default_caroussel.jpg',
price: '29€',
},
{
name: 'YYY',
info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
thumbnail: '/images/default_caroussel.jpg',
},
{
name: 'ZZZ',
info: 'ajksdnfakjsfndkjsf/akhad/kjsnka',
thumbnail: '/images/default_caroussel.jpg',
},
];

const subheader = 'subheader test';

describe('CartList', () => {
it('should render with subheader', () => {
const cartList = shallowWithContext();
expect(cartList).to.contain({subheader});
expect(cartList).to.have.text(subheader);
});
});

Error:

AssertionError: expected to have text 'subheader test', but it has '' HTML: Not available due to: Cannot read property 'prepareStyles' of undefined

disclaimer: I am new to testing, so I might be doing something else wrong

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/callemall/material-ui/issues/4664#issuecomment-284195281,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKG_Z5EI-fuFH-9QF7H80BKieg0W_bdEks5rigErgaJpZM4JIqLq
.

[oliviertassinari]

@PolGuixe True, the more the context is abstracted for users, the better.
@Valentin-Seehausen I only see two options going forward:

1. We expose the test helpers we are using internally and we document it
2. We make the context optional for rendering the components and we document it

I don't think that option 1. and 2. are mutually exclusive.

• The option 1. allows to simplify the integration tests with the mount API, unless you also want to unit test our component, then you will need the shallow helper too.
• The option 2. will make it even no brainer, users won't have to look into the documentation/issue as soon as they realize that they need something more to tests their component. However, that's code branching, they might not catch everything
  I have tried that with the avatar on the next branch I only had to change one line:

-const classes = context.styleManager.render(styleSheet);
+const classes = context.styleManager ? context.styleManager.render(styleSheet) : {};

[PolGuixe]

@oliviertassinari where can I find the test helpers you are using? I am still having problems.

[oliviertassinari]

I'm referring to those two helpers.

[Valentin-Seehausen]

@oliviertassinari Awesome.

We could warn, when the context is not provided.

[jtrein]

@PolGuixe's above test worked for me using the helper he created to provide context. So, thank you for that. To be fair, I don't know a lot about React context (only a bit from React Router).

My question(s) is: why do we need the theme's context unless we're running Jest snapshot testing? I just want to do "simple" unit-testy things. Is this a React limitation of not being able to de-couple a context from a lib component? Curious.

[jtrein]

Here's what I'm using based on the helpful above code. I have it stored in a testUtils.js file

import { mount, shallow } from 'enzyme';
import getMuiTheme from 'material-ui/styles/getMuiTheme';
import PropTypes from 'prop-types';

// enzyme MUI Test Helpers
// - https://github.com/callemall/material-ui/issues/4664

const muiTheme = getMuiTheme();

/**
* MuiMountWithContext
*
* For `mount()` full DOM rendering in enzyme.
* Provides needed context for mui to be rendered properly
* during testing.
*
* @param {obj}    node - ReactElement with mui as root or child
* @return {obj}   ReactWrapper (http://airbnb.io/enzyme/docs/api/ReactWrapper/mount.html)
*/
export const MuiMountWithContext = node => mount(node, {
  context: { muiTheme },
  childContextTypes: { muiTheme: PropTypes.object },
});

/**
* MuiShallowWithContext
*
* For `shallow()` shallow rendering in enzyme (component only as a unit).
* Provides needed context for mui to be rendered properly
* during testing.
*
* @param {obj}     node - ReactElement with mui
* @return {obj}    ShallowWrapper (http://airbnb.io/enzyme/docs/api/ShallowWrapper/shallow.html)
*/
export const MuiShallowWithContext = node => shallow(node, {
  context: { muiTheme },
  childContextTypes: { muiTheme: PropTypes.object },
});

[gnapse]

I bumped into this problem recently, and hence into this thread. What's up with that next coming up? Most links in this thread to it are dead. What's the final word about how to approach this problem? Is either of these two approaches available?

1. We expose the test helpers we are using internally and we document it
2. We make the context optional for rendering the components and we document it

[oliviertassinari]

@gnapse Oops, I have forgotten to close the issue, we went with option 1. by exposing test helpers. We have considered option 2. but it's making the implementation more complex. We think that option 1. is a better tradeoff. Here is the new documentation section on the next branch.

[gnapse]

@oliviertassinari thanks a lot for the info. And actually, thanks a lot for exposing me to this new/next re-implementation and the corresponding documentation website. How stable is this next thing? I love material-ui but I'm having a hard time accepting some things we dislike in our team about it (styles-in-js being a big one) and a quick glance tells me this next thing gets away from that, right? We were just starting to integrate this into our very young app, and I'd rather work with the next version than the current master one.

[oliviertassinari]

How stable is this next thing?

From an API point of view, we release breaking changes from one release to another. You can have a look at the releases notes to see if you are comfortable with that.
From a code point of view, I do no longer use the master branch in production, I have moved to the next version.

right?

Right.

[RobertHeim]

Since our tests most often do not test the mui-wrapper but the actual component we added up a dive() to jtrein's suggestion above:

export const MuiShallowWithContext = node => shallow(node, {
  context: { muiTheme },
  childContextTypes: { muiTheme: PropTypes.object },
}).dive(); // shallow-render the actual component

[oliviertassinari]

The v1-beta branch do no longer require a context to be present. It's transparent now.