Add updateProps function

[kentcdodds]

As noted in the docs, if I want to update props I simply call render with the same container:

const {container, queryByTestId} = render(<NumberDisplay number={1} />)
expect(queryByTestId('number-display').textContent).toBe('1')

// re-render the same component with different props
// but pass the same container in the options argument.
// which will cause a re-render of the same instance (normal React behavior).
render(<NumberDisplay number={2} />, {container})
expect(queryByTestId('number-display').textContent).toBe('2')

But this is cumbersome and it'd be nice to just have an updateProps function instead. It could have a note to avoid using it when you can, but it would be nice to have.

I'm not 100% certain that I really want it. Just thinking... One of the nice things about this library is what it doesn't give you and how that encourages better tests. But maybe this wont be so bad... It's especially useful for reusable components (like downshift).

[dnlsandiego]

What possible downsides would there be for an updateProps function? In the sense of what possible bad testing practices it might enable? I do think adding an updateProps function or another convenient way to re-render components would be beneficial since re-rendering components is such a core and powerful feature of React.

[kentcdodds]

The downside is that it contradicts the guiding principles:

The less your tests resemble the way your software is used, the less confidence they can give you.

A user never updates your components props. They'll click a button which in will update some state which will cause a component to re-render, which will update props. So what you should normally do is click that button instead.

But this is tricky for highly reusable components like downshift where you don't know what could update the props of the component. It could be anything. This is why I'm thinking about including it...

At the same time, the workaround is so simple I'm not sure it's worth it...

[antsmartian]

When you want to update props, I believe the user should typically call it out like as you have mentioned:

//change from 1 to 2
render(<NumberDisplay number={2} />, {container})

this is natural to react developers and I like it remains the same. Most of the use case, the props gets updated automatically (parents passing state as props to children etc), well in those cases I guess we need to have updateProps as such.

[kentcdodds]

I thought I understood what you were saying until you said this:

in those cases I guess we need to have updateProps as such.

I thought you were saying that you prefer the render with a container method. 😅

[antsmartian]

@kentcdodds : Yup you are right, it was typo :) It should be:

in those cases I guess we no need to have updateProps as such.

[kentcdodds]

Cool 👍

I think I'm going to drop this for now. We'll let someone else resurface it if they want to try to convince us to reverse this decision.

[SavePointSam]

Hey there! I'm here to bring this back up as the very first test suite that I converted from enzyme to RTL hit this.

When developing components for a UI library, I want to have confidence in some of the internals of the componentry. Specifically in that when a consumer provides specific props, the effects of those props can be verified and regulated. Take the following example of a Button component.

// src/components/Button/Button.js

import React, { Component } from 'react';
import styled from 'styled-components';
import classnames from 'classnames';

class Button extends Component<Props> {
  static defaultProps = {
    theme: 'secondary',
    isDisabled: false,
    onClick: () => {},
  };

  render() {
    const { label, isDisabled, theme, onClick } = this.props;

    return (
      <StyledButton
        disabled={isDisabled}
        aria-disabled={isDisabled}
        role="button"
        className={classnames({
          't-primary': theme === 'primary',
          't-secondary': theme === 'secondary' || !theme,
        })}
        onClick={onClick}
      >
        {label}
      </StyledButton>
    );
  }
}

const StyledButton = styled.button`
  box-sizing: border-box;
  min-height: 30px;
  max-height: 30px;
  min-width: 120px;
  max-width: 120px;

  border-radius: 4px;

  line-height: 20px;
  font-size: 14px;
  font-weight: 500;
  vertical-align: middle;

  cursor: pointer;
  user-select: none;

  &.t-primary {
    background-color: steelblue;
    color: #ffffff;
  }

  &.t-secondary {
    background-color: #ffffff;
    color: steelblue;
  }

  &[aria-disabled='true'] {
    background-color: #dfdfdf;
    color: #a8a8a8;

    cursor: not-allowed;
  }
`;

export default Button;

// src/components/Button/__tests__/Button.spec.js

import React from 'react';
import { render, Simulate, flushPromises } from 'react-testing-library';

import Button from '../Button';

const updateProps = (component, container) => render(component, { container });

describe('<Button />', () => {
  const fixtureRequiredProps = { label: 'Search' };
  const defaultProps = { ...Button.defaultProps, ...fixtureRequiredProps };
  const { getByTestId, container } = render(<Button {...defaultProps} />);

  describe('render', () => {
    beforeEach(() => {
      updateProps(<Button {...defaultProps} />, container);
    });

    it('executes without issue when only required props are provided', () => {
      expect(container.firstChild).not.toBeNull();
    });

    it('renders `label` as text', () => {
      expect(container.firstChild.textContent).toEqual(
        fixtureRequiredProps.label,
      );
    });

    it('renders with class `t-secondary` by default', () => {
      expect(container.firstChild.classList.contains('t-secondary')).toEqual(
        true,
      );
    });

    it('renders with class `t-primary` when `theme="primary"`', () => {
      updateProps(
        <Button {...defaultProps} theme="primary" />,
        container,
      );
      expect(container.firstChild.classList.contains('t-primary')).toEqual(
        true,
      );
    });

    it('renders with class `t-secondary` when `theme="secondary"`', () => {
      updateProps(
        <Button {...defaultProps} theme="secondary" />,
        container,
      );
      expect(container.firstChild.classList.contains('t-secondary')).toEqual(
        true,
      );
    });

    it('renders with [disabled] when `isDisabled`', () => {
      expect(container.firstChild.disabled).toEqual(false);
      updateProps(<Button {...defaultProps} isDisabled />, container);
      expect(container.firstChild.disabled).toEqual(true);
    });
  });
});

One of the first things I did was create an updateProps helper. Funny enough once I finished updating these tests, I went to look for a related issue and found that the exact same name was used! haha.

This can also be helpful when testing any controlled component and verification of a11y attributes. I am a strong advocate for having it in the library. However, we do need to be clear in the purpose of why it exists, or people will just abuse it for things it is not intended for.

Unless I'm way off base in my thinking, having been 'brainwashed' by current testing 'best practices', this example should prove the value a updateProps helper can provide. I'd also like to suggest that it be a good idea as in the short lifespan of this project it has been thought about enough to have an example in the README, an issue, and it was the very first (and so far only) shortcoming of this library I encountered. I would hope that this library will grow in the idea of making common patterns easy for users. Unless we really believe this pattern should be frowned upon. I don't believe that to be 100% true.

[kentcdodds]

Thanks for the added perspective @SavePointSam :)

In your case I would actually say that pattern is not a good one. Why do you need it to render to the same container? Your component doesn't necessarily respond differently when its props update. It just happens to render differently based on the current state of props. You could just as easily rewrite that test by rendering the entire component in every individual test:

// src/components/Button/__tests__/Button.spec.js

import React from 'react';
import { render, Simulate, flushPromises } from 'react-testing-library';

import Button from '../Button';

const renderButton = props => render(<Button {...defaultProps} {...props} />)

describe('<Button />', () => {
  const defaultProps = { label: 'Search' };
  // you don't need to include Button.defaultProps in this because that's actually
  // how react works. If those props aren't provided the defaults would be used.
  describe('render', () => {
    it('executes without issue when only required props are provided', () => {
      expect(renderButton().container.firstChild).not.toBeNull();
    });

    it('renders `label` as text', () => {
      expect(renderButton().container.firstChild.textContent).toEqual(
        fixtureRequiredProps.label,
      );
    });

    it('renders with class `t-secondary` by default', () => {
      expect(renderButton().container.firstChild.classList.contains('t-secondary')).toEqual(
        true,
      );
    });

    it('renders with class `t-primary` when `theme="primary"`', () => {
      const {container} = renderButton({theme: 'primary'})
      expect(container.firstChild.classList.contains('t-primary')).toEqual(
        true,
      );
    });

    it('renders with class `t-secondary` when `theme="secondary"`', () => {
      const {container} = renderButton({theme: 'secondary'})
      expect(container.firstChild.classList.contains('t-secondary')).toEqual(
        true,
      );
    });

    it('renders with [disabled] when `isDisabled`', () => {
      expect(renderButton().container.firstChild.disabled).toEqual(false);
      expect(renderButton({isDisabled: true}).container.firstChild.disabled).toEqual(true);
    });
  });
});

In addition, by sharing the container between tests you're inadvertently sharing state between tests which can lead to hard to debug issues (like disabling one test causes others to break).

My example above is just one way to do things, there are others. But I would suggest that the way your test was before was unnecessarily brittle.

The only time you might need an updateProps function is if you have a componentWillReceiveProps (advised against from the react team), componentWillUpdate (also advised against from the react team), or componentDidUpdate (just fine) behavior that you want to test. In this situation an updateProps would be useful. But the use cases are slim and the workaround is so simple. On top of that, an updateProps function apparently encourages tests like the ones you had and we don't want to encourage those kinds of tests as I mentioned. So I'm glad that there's friction for people when writing tests like that. This library is doing its job.

If updateProps didn't encourage those kinds of tests, then I'd be happy to add it to improve the componentDidUpdate testing experience. But as it does encourage poor practices I'm hesitant.

One last thing, please understand that I'm not criticising you or your coding abilities. You're a great person :) Our testing tools up to this point have encouraged sub-optimal testing patterns. No worries :)

[SavePointSam]

Hey! First, I take no offense to your suggestions and actually welcome them! I totally didn't even realize the state leak 😱. Like I said:

Unless I'm way off base in my thinking, having been 'brainwashed' by current testing 'best practices',

And you proved that I was, in fact, incorrect. I'm really happy I hit this problem and thank you for explaining how I can better test my components! That's exactly why I've begun moving to RTL. To have better tests without the misconceptions that exists out there today.

Your explanation about when updateProps could be effectively used as a blanket stance on why we don't have it. I'd recommend linking to your comment from the README.

Thanks!

[kentcdodds]

Your explanation about when updateProps could be effectively used as a blanket stance on why we don't have it. I'd recommend linking to your comment from the README.

Would you like to do that @SavePointSam? :)

[SavePointSam]

Aww. My first contribution. haha

[RareSecond]

Sorry for bringing this up again @kentcdodds, but I'm not sure if I'd really have a need for this function, or if my current approach isn't 'best-practice'.

Case is, I have a DateInput component which is a controlled component that accepts a value prop and and onChange prop to update its parents state.
There's even more functionality, but that goes beyond this issue.

The tests I first wrote, all passed.

describe('DateInput', () => {
  it('renders the correct date', () => {
    const date = new Date('2017-06-13T04:41:20');
    const { getByTestId } = render(
      <DateInput value={date} formatFunction={formatDate} />
    );
    const input = getByTestId('date-input');
    expect(input.value).toBe('13/06/2017');
  });

  it('changes the date on click', () => {
    let date = new Date('2017-06-13T04:41:20');

    const onChange = jest.fn(selectedDate => {
      date = selectedDate;
    });
    const { getByTestId, getByText, rerender } = render(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );
    const input = getByTestId('date-input');
    expect(input.value).toBe('13/06/2017');

    fireEvent.click(getByText('19'));
    rerender(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );
    expect(input.value).toBe('19/06/2017');
  });

  it('switches to previous month when clicking 31', () => {
    let date = new Date('2017-06-13T04:41:20');

    const onChange = jest.fn(selectedDate => {
      date = selectedDate;
    });
    const { getByText, rerender } = render(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );

    fireEvent.click(getByText('31'));
    rerender(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );

    expect(getByText('May 2017'));
  });
});

As you can see, there's a lot of repetition in each test, and since I want my tests to be maintainable, I searched for some examples, and I stumbled upon this.

I tried to refactor my code, by abstracting the initial component render, as such:

const renderDateInput = () => {
  let date = new Date('2017-06-13T04:41:20');

  const onChange = jest.fn(selectedDate => {
    date = selectedDate;
  });

  return render(
    <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
  );
};

describe('DateInput', () => {
  it('renders the correct date', () => {
    const { getByTestId } = renderDateInput();

    const input = getByTestId('date-input');
    expect(input.value).toBe('13/06/2017');
  });

  it('changes the date on click', () => {
    const { getByTestId, getByText, rerender } = renderDateInput();
    const input = getByTestId('date-input');
    expect(input.value).toBe('13/06/2017');

    fireEvent.click(getByText('19'));
    rerender(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );
    expect(input.value).toBe('19/06/2017');
  });

  it('switches to previous month when clicking 31', () => {
    let date = new Date('2017-06-13T04:41:20');

    const onChange = jest.fn(selectedDate => {
      date = selectedDate;
    });
    const { getByText, rerender } = render(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );

    fireEvent.click(getByText('31'));
    rerender(
      <DateInput value={date} onChange={onChange} formatFunction={formatDate} />
    );

    expect(getByText('May 2017'));
  });
});

The first test still passes, but as soon as I started simplifying the second test, I was blocked.
The changed date is of course only scoped to the renderDateInput function, but has to be passed in the rerender function.

So now I'm wondering how to best approach my test, while having next to no repetition.

Thanks a bunch!

[kentcdodds]

Hi @JDansercoer,

Do this:

function TestDateWrapper() {
  const [date, setDate] = React.useState(new Date('2017-06-13T04:41:20'))
  return <DateInput value={date} onChange={setDate} formatFunction={formatDate} />
}

Then render <TestDateWrapper /> instead. It could even accept props itself if you need it to...

[RareSecond]

Aha of course, that makes perfect sense! Thanks for the quick answer!

[RareSecond]

After playing around with it some more, I noticed that my rerender calls are now also obsolete.
So apparently, the component gets autoupdated anyway, so I don't really see what the use of the rerender function would be?
Or am I missing something?