-
-
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
Support convert shallow wrapper to format compatible with pretty-format and jest-snapshot #541
Support convert shallow wrapper to format compatible with pretty-format and jest-snapshot #541
Conversation
The new `.json()` method convert the shallow node to compatible object. This allow us to use enzyme+shallow with jest-snapshot: ``` const Foo = () => <div className="in-foo" />; const Bar = ({ value }) => ( <div className="in-bar"> <Foo fooProp={value} /> </div> ); const wrapper = shallow(<Bar value={ 'xxx' } />); expect(wrapper.json())..toMatchSnapshot(); ``` will create a snapshot as: ``` exports[`example test 1`] = ` <div className="in-bar"> <Foo fooProp="xxx" /> </div> `; ```
👍 I think this is a great idea. This basically does what |
props: omit(this.props(), 'children'), | ||
children: (childrens.length) ? childrens : null, | ||
$$typeof: Symbol.for('react.test.json'), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this format specified by jest-snapshot
? If so, maybe we should call this snapshot()
instead of json()
to make it clear that this method is specifically for snapshot tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format is defined by pretty-format
(https://github.com/thejameskyle/pretty-format/blob/master/plugins/ReactTestComponent.js), which jest-snapshot uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding a // @see _URL_
directive for this function so we can maintain interop and know where to look to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be worth only installing this method if jest
is globally available? As it won't provide much benefit in other test runners currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about when Symbols aren't available?
Using a registry symbol is certainly a novel way to use globals, but I'm very loath to add one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we warn if they're calling json()
in an environment without Symbol.for
? It's available in Node since 0.11
, and this wouldn't be very useful in a browser environment. That's another reason I think it should be called snapshot
, since it's not meant to be used as a general-purpose JSON
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it were, it would need to be called toJSON
anyways. I agree that the name json
isn't appropriate.
The catch is that enzyme, and its tests, need to run in many browsers and all supported nodes, if we want to be able to test how our React components actually work in these environments. Relying on Symbols hurts that goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, Symbol.for
is an issue. pretty-format
relies on it, though.
I agree now that the json()
name can be confusing, I'm liking snapshot()
more but usage will be a little weird: expect(wrapper.snapshot()).toMatchSnapshot()
(or something similar)
Yes @aweary, it does the same thing that |
Since this API awkwardly only is supported in one test environment, would it be a crazy and/or bad idea to effectively replicate import { matchesSnapshot, shallow } from 'enzyme';
const wrapper = shallow(<div />);
wrapper.matchesSnapshot(); // this is the assertion We would then have to write to a file, have them commit it to source, and do compare with that file. I'm not sure if this is possible however, just spit-balling. |
In general, I'm not a fan of snapshot testing. The whole point of enzyme is to NOT do snapshot testing - ie, jest-snapshot is a step in the wrong direction. |
@blainekasten I think this can be better implemented in the test-runner/assertion framework, but I wouldn't mind changing to this approach if there is a feasible way to implement this here. @ljharb I think snapshot is a good alternative in react testing ecosystem because it automatizes some (if not most) of the Let me know if you think this would be better as an external lib, it doesn't rely on nothing internal to shallow wrapper right now. |
I think snapshot testing is great in general, but I can see why @ljharb might not be as excited about integrating it with Enzyme. Enzyme is a tool for traversing component structure, so adding snapshot testing would be moving away from that, making it more general purpose. I think ideally Enzyme would be used in conjunction with another tool that is responsible for snapshot testing--or it would be extended by an external lib. Soon enough Enzyme will work fine together with |
I could set up snapshot testing with Enzyme right away, without any extra changes. Let's create a test shallow component: const component = shallow(<MyCmp title="Hello!" config={{test: 'config'}} />) There are several options how to get a snapshot of it.
expect(component.debug()).toMatchSnapshot();
//produces the following
exports[`test renders test component 1`] = `
"<div config={{...}}>
<h1>Hello!</h1>
<p>Test content</p>
</div>"
`; But look the config
expect(component.get(0)).toMatchSnapshot();
//prints the following
exports[`test renders test component 2`] = `
<div config={
Object { "test": "config" }
}>
<h1>Hello!</h1>
<p>Test content</p>
</div>
`; That's better, object is preserved, but it looks like private API usage. Do this or not is up to you, you can choose between this and option above.
|
With update: For anyone interested in using enzyme with snapshot testing, there is a lib that provide |
The new
.json()
method convert the shallow node to compatible object.This allow us to use enzyme+shallow with jest-snapshot:
will create a snapshot as:
More info: pretty-format and jest snapshot