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

Clarify shallow rendering behavior with props #445

Open
aweary opened this issue Jun 8, 2016 · 21 comments
Open

Clarify shallow rendering behavior with props #445

aweary opened this issue Jun 8, 2016 · 21 comments

Comments

@aweary
Copy link
Collaborator

aweary commented Jun 8, 2016

So our documentation has the following snippet:
http://airbnb.io/enzyme/docs/api/ShallowWrapper/prop.html

const wrapper = shallow(<MyComponent foo={10} />);
expect(wrapper.prop('foo')).to.equal(10);

The issue is that, unless this foo prop is being passed as props to a child element then this will actually fail. @lelandrichardson's explanation of shallow rendering here

@nfcampos @aweary if you want to really understand the reason, it can be stated like this:

  • Consider the full react render tree as a tree in 3-dimensional space
  • Consider the root component of a shallow call (ie, Foo in the case of wrapper()) as z=0 in this space
  • In the case of shallow, we choose to look only at z=1, which includes essentially the full output of render() of the component.
  • In the case of Foo being a DOM node, there is no z=1.

As a result, instead of making this an error case, we simply fall back to looking at z=0 instead of z=1.

I'm not sure if I consider this behavior wrong or less than ideal, but I do think that the fact that a large subset of our tests happen to test this case rather than the composite component case is the real problem.

I'm of the opinion that we should be looking at z=0 in all cases to properly support props that are defined on the root component in the shallow call. Users are doing stuff like:

class Foo extends Component {
  render () {
    return <div className='foo'>{this.props.foo}</div>
  }
}

...

const wrapper = shallow(<Foo foo="bar" />)
expect(wrapper.prop('foo')).to.equal('bar')

And it's failing because shallow is only looking at the returned div which only has a className and children prop.

We need to decide if this should be supported or not, and if so how we can do it. Any discussion/feedback would be great :)

@nfcampos @blainekasten @lelandrichardson @ljharb

@ljharb
Copy link
Member

ljharb commented Jun 8, 2016

I see why this is confusing, but why would you need to make assertions about the immutable props you literally just passed in? The assertions are on the wrapper, not on the element itself.

@nfcampos
Copy link
Collaborator

nfcampos commented Jun 8, 2016

as @ljharb said, there's no use for getting a wrapper around the element you just rendered. The only reason I'd see for changing the behaviour of shallow would be to not confuse users who use both shallow and mount, as it can be easy to forget that when using mount you need to select the root rendered node to assert stuff about it (do what exactly I don't know, see #446) but with shallow that's what we return.

@aweary
Copy link
Collaborator Author

aweary commented Jun 8, 2016

That's a good point, I can't think of a use case where you can't just assert on the value before its being passed via props. If we can't find a good reason to support this use case we should definitely update the docs and possibly do a write-up on how shallow builds the render tree.

@AshCoolman
Copy link

AshCoolman commented Jun 9, 2016

Ah, I've just started using this Enzyme, and I did find this [a bit confusing] :)(http://stackoverflow.com/questions/37704979/enzymes-shallow-text-with-react-native-doesnt-work-as-i-expected)

Notice there is a suggestion to amend the docs in the SO comments:

Returns a string of the rendered text of the current render tree. This function should be looked at with skepticism if being used to test what the actual HTML output of the component will be. This only works if you don't have a React Component in there. As soon as you do, it will return the .toString method of the component, which is usually just the name.
– ZekeDroid

@atmd83
Copy link

atmd83 commented Jul 12, 2016

const wrapper = shallow(<MyComponent foo={10} />);
expect(wrapper.prop('foo')).to.equal(10);

This also doesn't seem to work if the prop has been set using setDefaultProps.

works when using mount though so maybe just a doc's update to clarify.

@ljharb
Copy link
Member

ljharb commented Jul 12, 2016

@atmd83 that shouldn't work. With shallow, you're testing with MyComponent renders - not the element you passed into shallow. You literally just passed it a "foo" prop of 10, why would you need to test that it has a "foo" prop of 10?

@antz29
Copy link

antz29 commented Aug 2, 2016

@ljharb if you are doing TDD then you want to specify your public API, which for a component is it's props. This would also include verifying that defaultProps are the values you expect them to be etc. you are not validating that it works, you are testing that it hasn't changed and also documenting it's functionality. TDD is more about documentation and design that it's about testing, the tests are a nice side effect. /cc @atmd83

@ljharb
Copy link
Member

ljharb commented Aug 2, 2016

@antz29 you should be testing the effect of your defaultProps, not the literal value of those props. TDD is about documenting the effect of your code - not its internal implementation, which is what defaultProps are.

@antz29
Copy link

antz29 commented Aug 2, 2016

@ljharb I disagree, for me, like I said, the documentation value of tests is critical. Documenting The public API of a component is important as it's both what it's users will use and the contract it has with other components. I open the test and and I see what props are defined, what the default values are (if any) and how I should use them. Without that, I need to open up the components code to find that. Tests give me confidence that the API I'm working with works the way it's specified in the test (or spec). In TDD I'm not really testing anything, I'm specifying it.

@ljharb
Copy link
Member

ljharb commented Aug 2, 2016

defaultProps remain an implementation detail that aren't part of the API.

@antz29
Copy link

antz29 commented Aug 5, 2016

@ljharb defaultProps an implementation detail? If I'm checking the docs for a method, I definitely care what the default values for arguments are. That documents what the engineer perceived the most likely / useful values would be. It tells me a lot about their assumptions. It's exactly the same for Components. The public API for a component is the defined props AND their default values if any, if default values were not part of the public API they wouldn't be documented.

@ljharb
Copy link
Member

ljharb commented Jan 17, 2017

I'm not saying they don't matter. I'm saying that a default function value and an "||" inside the function are equivalent, albeit one is more easily discoverable. That's why I said here that you should indeed be testing the effect of the defaults.

In the above example, if you pass a 10 or if you pass nothing, you should be testing only how that effects the render output - similarly to testing the return value of a pure function. Testing details of how things are defaulted separate from that is testing implementation details, and that makes for brittle, tightly coupled tests. I should be able to refactor freely between defaultProps, and this.props.a || whatever, without breaking any tests or requiring any test changes.

@dschinkel
Copy link

So I TDD. If a parent component is passing props to a child, I'd wanna know that it successfully did so. As you know, a parent could have logic in it. That could be as simple as if someone say took that prop you passed into the parent, and lets say they set it to another variable for whatever reason then used that other variable as the value to be passed into a child's props. OR maybe they're taking a subset of the data from the parent's props, or manipulating the data before sending it off to a child's props.

In any case, there will be logic sometimes in your parent. There's a need to test that the parent successfully passed the expected data to child props...because if any of that behavior stated above in the parent fails, the child props may not receive the data you expected hence you've just discovered a bug or someone or you just broke your code if that test of passing props to child fails, you have something what went wrong in your parent.

So I agree with don't test the props you just sent into the wrapper...makes no sense. Test the outcome of the parent's render. So if you have the parent rendering a child, you'd wanna shallowly test that the child got the right props from the parent. I haven't found a way though to do that with child as you can't use instance() on a child component, only the root component with shallow().

@dschinkel
Copy link

dschinkel commented Mar 13, 2017

and...my head hurts after reading this entire thread :P. Did you end up documenting how shallow renders the tree yet?

@ljharb
Copy link
Member

ljharb commented Mar 14, 2017

@dschinkel If you have a <Parent> that renders a <Child>, you shallow-render the Parent, then do wrapper.find(Child).props() and assert on that.

@dschinkel
Copy link

dschinkel commented Mar 14, 2017

yea in issue 848 that I just posted, I stated that I've tried that already before posting all this, and it's not working for me...

resolved, see 848, yes it's working fine.

@AndrewSouthpaw
Copy link

My use case is that I'd like to pass in mock functions by default and not hold onto their reference. Usually I do something like this:

const shallowComponent = (props = {}) => (
  shallow(<Button onCancel={jest.fn()} onSave={jest.fn()}/>)
)

Then in my test I want to be able to say something like...

expect(wrapper.props().onCancel).toHaveBeenCalledTimes(1)

Right now I have to hold onto a reference of the mock function, or do a full mount.

... I realize it's not a huge use case, but I find it quite handy.

@ljharb
Copy link
Member

ljharb commented Jun 24, 2017

That use case would be analogous to calling a function with arguments and expecting to be able to retrieve a reference to those arguments later from the function.

Definitely just hold on to a separate reference. It's clearer anyways.

@AndrewSouthpaw
Copy link

I hear what you're saying, I guess I'm viewing it more from an OOP perspective, where it's ordinary to access properties you passed in on initialization.

@davidkuen
Copy link

@ljharb in response to your question "why would you need to make assertions about the immutable props you literally just passed in?" here's an example that's pretty common in the codebase I'm working in:

const Item = ({ type }) => {
    const itemType = type === 'file' ? 'File' : 'Folder';
    const subsection = <Subsection description={ itemType } />;
    return <Something subsection={ subsection } />;
}

how do I test that the description prop on the subsection is correct (without doing a full mount)? My first approach would be to shallow render the subsection:

const wrapper = shallow(<Something type="file" />);
const subsection = shallow(wrapper.prop('subsection'));

and then I run into the problem described in this thread. The subsection wrapper in this example actually shallow-renders the children of Subsection rather than Subsection itself (e.g. subsection.is('Subsection') returns false, and subsection.prop('description') doesn't return what I want it to).

The two solutions to this problem:

  1. Wrap the component with a span:
const Item = ({ type }) => {
    const itemType = type === 'file' ? 'File' : 'Folder';
    const subsection = <span><Subsection description={ itemType } /></span>;
    return <Something subsection={ subsection } />;
}

not great for obvious reasons

  1. (What I've actually been doing) directly access the React instance in the test:
const wrapper = shallow(<Something type="file" />);
const subsection = wrapper.prop('subsection');
assert.equal(subsection.props.description, 'File');

which is fine, but then you're no longer using enzyme and lose out on all its other features

is there a better solution to this problem that I'm missing?

@ljharb
Copy link
Member

ljharb commented Dec 21, 2017

@davomgz ah, that's a different case. In that case, you're doing something more like the following:

const wrapper = shallow(<Item type="file" />);
const subsection = wrapper.prop('subsection');
const subsectionWrapper = shallow(() => subsection);
// make your assertions here.

#1444 may be relevant to your question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants