glenjamin / skin-deep

Test assertion helpers for use with React's shallowRender test utils
MIT License
200 stars 40 forks source link

getRenderOutput().props is not accurate #37

Closed hallister closed 8 years ago

hallister commented 8 years ago

The props object of getRenderOuput() appears to be a copy of the input, rather than the actual props for the rendered component. So if I have a Button component with a render function that looks like this:

let { color, ...other } = this.props;

return <Button {...other}>Test</Button>;

What should be happening is getRenderOutput.props should no longer have a color attribute. Instead, getRenderOutput().props is just a copy of the props passed into shallowRender(). As a result, we can't test to see if a proper was properly consumed.

FYI, this problem began after recently updating SD. Prior to the update (I was using 0.6 I believe), this worked as expected.

glenjamin commented 8 years ago

Odd, we've got tests specifically for props being the result of the rendering: https://github.com/glenjamin/skin-deep/blob/master/test/test.js#L39

Can you post a fuller example?

hallister commented 8 years ago

Here's the example I'm using (I had to remove some stuff to keep it simple, but this should work):

class TestComp extends React.Component {
    static propTypes = {
        consume: React.PropTypes.bool
    }

    render() {
        let { consume, ...other } = this.props;
        return React.createElement(
            'div'
            other
        );
    } 
}

and the test:

    let tree = sd.shallowRender($(Element, { consume: true ));
    let vdom = tree.getRenderOutput();

    expect(vdom.props).not.to.have.property('consume');

The test fails becaue vdom.props.consume exists. It shouldn't, because I'm never passing the consume prop to the rendered div, I'm passing it to TestComp, which consumes it. To avoid es6, you could just give React.createElement an empty object instead of other and the problem remains: the output of tree.getRenderOutput() is effectively saying that all props sent to TestComp are also props on the output of its render function, which isn't true.

This may be a bigger fix than it's worth, in which case I'll just use a deep render for this test, but technically the output of tree.getRenderOutput doesn't accurately describe what the rendered value would look like in the DOM.

hallister commented 8 years ago

For the record, my use case is to avoid a bug that also affected ReactTransitionGroup: https://github.com/facebook/react/issues/4954.

glenjamin commented 8 years ago

That test is definitely showing a bug - can you add a console.log after the destructuring and confirm that other definitely does not contain consume?

getRenderOutput() just passes through to React's shallowRender system directly, so any bug is unlikely to be with skin-deep itself.

are you using babel to transpile the destructuring?

glenjamin commented 8 years ago

I tried to reproduce with your sample code, but I don't get an error:

https://gist.github.com/glenjamin/02d7f6465e5eccf35e8a

hallister commented 8 years ago

Hmm, just to make sure I npm installed mocha/skin-deep and a few other test related packages and now it passes without problem. Not sure what it was, but oh well, case closed :).