-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Comments
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. |
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 |
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 |
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:
|
This also doesn't seem to work if the prop has been set using works when using mount though so maybe just a doc's update to clarify. |
@atmd83 that shouldn't work. With |
@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 |
@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. |
@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. |
defaultProps remain an implementation detail that aren't part of the API. |
@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. |
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 |
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(). |
and...my head hurts after reading this entire thread :P. Did you end up documenting how shallow renders the tree yet? |
@dschinkel If you have a |
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. |
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. |
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. |
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. |
@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 const wrapper = shallow(<Something type="file" />);
const subsection = shallow(wrapper.prop('subsection')); and then I run into the problem described in this thread. The The two solutions to this problem:
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
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? |
@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. |
So our documentation has the following snippet:
http://airbnb.io/enzyme/docs/api/ShallowWrapper/prop.html
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 hereI'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 theshallow
call. Users are doing stuff like:And it's failing because shallow is only looking at the returned
div
which only has aclassName
andchildren
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
The text was updated successfully, but these errors were encountered: