New shape matcher proposal #2202
Comments
|
@Spy-Seth Thank you for proposing match shape. Good idea to make it a new issue. Can you give some examples of JSX that are incorrect for the assertion? Can you confirm if these examples are correct for the assertion, or not:
|
|
I wonder if this could be done using |
|
@cpojer Interesting, a quick test suggests that Jest https://jasmine.github.io/edge/introduction#section-Custom_asymmetric_equality_tester Although I am not suggesting solutions yet, is that relevant to the use case in this issue? |
|
Thanks for taking time for discutting this.
Extra props should be allowed by default. If needed it could exist an option/syntax that will be more strict.
Exactly, as we match a shape no warranty on the exact level on the DOM.
Good questions! As we allow props, I guess that we have to allow sibling and child. Here come incorrect vdom for the previously proposed shape ( // No <Bar> component
<Foo /> // <Foo> in not a parent of <Bar>
<Bar who="John" />
// Or:
<div>
<Foo />
<Bar who="John" />
</div>// No props `who`
<Foo>
<Baz />
</Foo>// Wrong value for props `who`
<Foo>
<Bar who="Michael" />
</Foo> |
|
👍 Love the idea, in particular I see a use case when we want to ensure that a HOC (which is tested separately with snaps) effectively decorates another component. |
|
Just for fun, tried a minimal shape match for deep renderer from As you would expect, an object subset match deals with extra props: import React from 'react';
import renderer from 'react-test-renderer';
const Foo = ({children, ...rest}) => (<div {...rest}>{children}</div>);
const Bar = ({children, ...rest}) => (<span {...rest}>{children}</span>);
describe('object subset', () => {
const pattern = renderer.create(<Foo><Bar who="John" /></Foo>).toJSON();
it('does match extra props', () => {
const tree = renderer.create(<Foo><Bar who="John" what="person" /></Foo>).toJSON();
//expect(tree).toMatchObject(pattern);
expect(compare(pattern, tree)).toBe(true);
});As you might expect, an object subset match does not automatically deal with:
|
|
Too much fun, so went a bit farther. Extended a copy of
Original code is 42 lines, extended code is 73 lines with naive traversal algorithms. Given EDIT: Forgot to say that error output for import React from 'react';
import renderer from 'react-test-renderer';
const A1 = ({children, ...rest}) => (<article {...rest}>{children}</article>);
const A2 = ({children, ...rest}) => (<section {...rest}>{children}</section>);
const P = ({children, ...rest}) => (<div {...rest}>{children}</div>);
// Components render flow elements above and phrasing elements below
const C = ({children, ...rest}) => (<span {...rest}>{children}</span>);
const D1 = ({children, ...rest}) => (<strong {...rest}>{children}</strong>);
const D2 = ({children, ...rest}) => (<em {...rest}>{children}</em>);
describe('function props', () => {
it('match too exactly in default object subset match', () => {
const subtree = renderer.create(<P><C who="John" how={() => {}} /></P>).toJSON();
const tree = renderer.create(<P><C who="John" how={() => {}} /></P>).toJSON();
//expect(tree).not.toMatchObject(subtree);
expect(compare(subtree, tree)).toBe(false);
});
it('must match as equivalent in a useful subtree match', () => {
const treeFalse = renderer.create(<P><C who="John" how={() => false} /></P>).toJSON();
const treeTrue = renderer.create(<P><C who="John" how={() => true} /></P>).toJSON();
expect(treeFalse).toEqual(matcher(treeTrue));
});
});
describe('subtree', () => {
const subtreeDefault = renderer.create(<P><C who="John" /></P>).toJSON();
it('does match extra props in parent and child', () => {
const tree = renderer.create(<P who="Marie"><C who="John" what="person" /></P>).toJSON();
expect(tree).toEqual(matcher(subtreeDefault));
});
it('does not match missing prop in child', () => {
const tree = renderer.create(<P><C /></P>).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match different prop in child', () => {
const tree = renderer.create(<P><C who="Michael" /></P>).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match missing child', () => {
const tree = renderer.create(<P />).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match missing parent', () => {
const tree = renderer.create(<C who="John" />).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match missing missing child but another occurs', () => {
const tree = renderer.create(<P><D1 /></P>).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match missing parent which occurs as sibling', () => {
const tree = renderer.create(<A1><P /><C who="John" /></A1>).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does not match separated parent and child', () => {
const tree = renderer.create(<P><D1><C who="John" /></D1></P>).toJSON();
expect(tree).not.toEqual(matcher(subtreeDefault));
});
it('does match parent as descendant in tree', () => {
const tree1 = renderer.create(<A1><P><C who="John" /></P></A1>).toJSON();
expect(tree1).toEqual(matcher(subtreeDefault));
const tree2 = renderer.create(<A1><A2>preceding</A2><A2><P><C who="John" /></P></A2><A2>following</A2></A1>).toJSON();
expect(tree2).toEqual(matcher(subtreeDefault));
});
it('can match child which has children', () => {
const tree = renderer.create(<P><C who="John"><D1>grandchild</D1></C></P>).toJSON();
expect(tree).toEqual(matcher(subtreeDefault));
});
it('can match child which has preceding sibling', () => {
const tree = renderer.create(<P><D1>preceding</D1><C who="John" /></P>).toJSON();
expect(tree).toEqual(matcher(subtreeDefault));
});
it('can match child which has following sibling', () => {
const tree = renderer.create(<P><C who="John" /><D2>following</D2></P>).toJSON();
expect(tree).toEqual(matcher(subtreeDefault));
});
it('can match adjacent children which have siblings', () => {
const subtreeDefault = renderer.create(<P><C who="Paul" /><C who="Mary" /></P>).toJSON();
const tree = renderer.create(<P><C who="Peter" /><C who="Paul" /><C who="Mary" /><C who="John" /></P>).toJSON();
expect(tree).toEqual(matcher(subtreeDefault));
});
}); |
|
I'm following this here (a bit busy with other stuff) and like what you guys are coming up with. Happy to review PRs for Jest if that makes sense. |
Created a special issue #2215 about making this better to read. |
|
Thanks you @pedrottimark for taking so many time to play around this! 👍 I'm not entering too much in the implementation discussions as I do not manage any of the internals details of jest or react. |
|
Selecting or traversing has simmered in my mental slow cooker for several days after reading:
Now it seems like I mistakenly mentioned matching flexibility not implied in the original comment. If we assume that you select or traverse in the tree to the relevant subtree for Following up on #2213 (comment) I would welcome any thoughts from @just-boris or examples from experience with |
|
The
Examples are for the following components:
1. content without functionsHere is a better solution than I suggested in #2197 (comment) describe('Todo', () => {
it('renders text and uncompleted style', () => {
const text = 'Install Jest';
const completed = false;
const textDecoration = 'none';
// react-test-renderer
expected = renderer.create((<li style={{textDecoration}}>{text}</li>).toJSON();
received = renderer.create(<Todo onClick={onClick} text={text} completed={completed} />).toJSON();
/*
// or Enzyme shallow rendering (for this component, it renders only DOM elements)
received = enzymeToJSON(shallow(<Todo onClick={onClick} text={text} completed={completed} />));
// or Enzyme full DOM rendering
received = mountedToObject(mount(<Todo onClick={onClick} text={text} completed={completed} />));
*/
expect(received).toMatchObject(expected); // expected omits onClick
});
});helper functionsWe need these now. Don’t take too seriously (not recommending as the future design :)
RE-EDIT: in the following #2202 (comment)
2. result of interactiondescribe('TodoList', () => {
it('changes completed style' => {
const state0 = [todo0, todo1, todo2];
const index = 1;
const {text} = state0[index];
const textDecoration = 'line-through';
let state = state0;
const onTodoClick = jest.fn((index) => { state = todos(state, toggleTodo(index)); });
const wrapper = mount(
<TodoList
onTodoClick={onTodoClick}
todos={state0}
/>
);
const li = wrapper.find('li').at(index);
li.simulate('click');
expect(onTodoClick).toHaveBeenCalledTimes(1);
expect(onTodoClick).toHaveBeenCalledWith(index);
// simulate re-render because props changed
wrapper.setProps({todos: state});
// ordinary assertion via Enzyme
expect(li.prop('style').textDecoration).toEqual(textDecoration);
// 3 alternative ways to declare the assertion
expect(mountedToObject(li)).toMatchObject(renderToObject(
<li style={{textDecoration}}>{text}</li>
)); // JSX can omit onClick but not text, if received has text
expect(mountedToObject(li)).toMatchObject(renderToObject(
React.createElement('li', {style: {textDecoration}}, text)
)); // React.createElement can omit onClick but not text, same song, second verse
expect(mountedToObject(li)).toMatchObject(literalToObject({
type: 'li',
props: {style: {textDecoration}},
})); // object literal can omit onClick and text
});
});A more convincing example when to write a declarative subset assertion would have:
Here are some assertions just to illustrate how the matching works: expect(mountedToObject(li)).not.toMatchObject(renderToObject(
<li style={{textDecoration}}></li>
)); // JSX cannot omit text from expected, if received has text
expect(mountedToObject(li)).toMatchObject(renderToObject(
React.createElement('li', null, text)
)); // JSX or React.createElement can omit all props from expected
expect(mountedToObject(li)).not.toMatchObject(renderToObject(
React.createElement('li')
)); // but cannot omit text, same song, second verse
expect(mountedToObject(li)).toMatchObject(literalToObject({
type: 'li',
})); // object literal can omit props and children
expect(mountedToObject(li)).toMatchObject(literalToObject({
props: {style: {textDecoration}},
})); // or could even omit the type of the element |
|
Goal of this iteration: easily delete irrelevant parts from expected object for subset match. Let’s take a minute to look at the
|
|
Example for Create #2202 (comment) if record is incorrectly inserted as last row instead of first: <tbody>
<tr>
<td />
<td>
- 2017
+ 2016
</td>
<td>
- ECMAScript 2017
+ ECMAScript 7
</td>
<td>
- async or swim, not much longer to await
+ small but powerful: ha * ha === ha ** 2
</td>
</tr>
<tr />
<tr />
</tbody> |
|
@cpojer What do you think about the following difference between
Where I ran into the difference was tests comparing received from On the other side of the argument, this seems like a breaking change for snapshot tests: exports[`empty string is rendered by react-test-renderer 1`] = `
<span>
</span>
`;
exports[`if empty string is not rendered by react-test-renderer 1`] = `<span />`;To take it one step farther, a codemod to fix this case could make a subtle (though perhaps unlikely in practice) incorrect change to the identical snapshot which is arguably correct for elements which have conditional logic like The reason I say that empty string as an explicit child could be considered consistent in these cases: when there are two or more child expressions,
By the way, the conditional logic example is what put me onto this issue. The In summary, what do you think about this case when the children on a React element consists of exactly one empty string:
If you think there is a case to propose a change, are you willing to take the role of “champion” if I am willing to do the work? Aha, at this very moment, the situation has become more complex:
As an alternative, the proposed |
|
@cpojer While reading the code for Flow type omits
What do you think? Does it matter? EDIT: Submitted facebook/react#8816 |
|
I'm not super up-to-date on this discussion and react related conversation should probably happen somewhere else (react repo?) but here is one pointer: enzymejs/enzyme#742 enzyme is looking into this, too. |
|
@cpojer Thank you for that link. For now, the helper function will take care of the difference. |
|
@pedrottimark I'm a bit unclear on this issue what the action items are. What can we do in Jest to improve this? The shape matcher proposal seems to make sense to me; it appears to me that pretty-printing React component diffs through I understand you have concerns with the behavior of the react-test-renderer. I would like to ask you to discuss that in the react repo, as the Jest team isn't responsible for this piece :) |
|
Yes, it’s time to make things clear enough for action! Please pardon the length of this comment. @cpojer Would you like to craft an API from the following summary from discussion items?
After that, I will work through the remaining details for each item that survives. Part A seems useful to test update operations. Part B seems useful to test create or delete operations. So they are independent decisions, but the whole is more than the sum of the parts :) Part A: differential snapshotDecision 1: Is expect(diffSnapshot(prev, next)).toMatchSnapshot();Decision 2: Is expect.addSnapshotSerializer('path/to/diffSnapshotSerializer');Alternative 2: a standard built-in plugin? For example, In case it helps you to know, the serializer does the hard work. The function returns an identifiable object which consists of the args Part B: expected test object for relevant subset matchAlthough Jest team doesn’t decide, it seems like you are co-stakeholder with React team concerning developer experience in Jest. According to guidance here, I can open an issue there. Opinion 3: Is render function or method needed? What to call it? How to import it? // Given an element, for example:
const element = <tr><td>{irrelevant}</td><td>{recordCreated.when}</td><td /><td /></tr>;
// A possibility is a separate named export
import {irrelevant, renderAsRelevantObject} from 'react-test-renderer';
expect(…).toMatchObject(renderAsRelevantObject(element));
// Another possibility is a method of the default export
import renderer, {irrelevant} from 'react-test-renderer';
expect(…).toMatchObject(renderer.create(element).toRelevantObject());The function calls
Among the reasons to use
Part B: received test object for relevant subset matchAlthough Jest team doesn’t decide, we can have friendly influence on Opinion 5: Is import {mount} from 'enzyme';
import {mountedDomToObject} from 'enzyme-to-json';
const wrapper = mount(/* component in its initial state */);
// simulate interaction, and then compare component to relevant subset:
expect(mountedDomToObject(wrapper)).toMatchObject(…);The new named export transforms as follows an
In summary of Part B, a delicate interaction that Jest depends on but doesn’t control: P.S. Update to previous comment about |
|
I can't speak as much about Part B/C but for the first one:
You are also bringing up the question on how we should diff: it seems better to do that on the object level, but it may be slow because it has to be recursive. |
|
An example came to mind this week why to let the test take full responsibility for state. As a warm up, pseudo code for a test of a Redux reducer. Although we don’t assume everyone uses Redux, many intermediate level React devs have probably heard about immutability. it('reducer changes state in response to action', () => {
const prev = /* initial state */
const next = reducer(prev, /* action */);
expect(diffSnapshot(prev, next)).toMatchSnapshot();
});Imagine that you double-click a table cell to edit it. A test for this update operation might have 3 states: prev, updating, next (that is, the edited result). Because a mounted it('table updates a cell', () => {
const wrapper = mount(/* Table component in its initial state*/);
const prev = mountedDomToObject(wrapper.find(/* cell selector */);
wrapper.find(/* cell selector */).simulate('doubleClick');
const updating = mountedDomToObject(wrapper.find(/* cell selector */);
// interaction to edit the input element in the cell
const next = mountedDomToObject(wrapper.find(/* cell selector */);
expect(diffSnapshot(prev, updating)).toMatchSnapshot();
expect(diffSnapshot(prev, next)).toMatchSnapshot(); // instead of (updating, next)
});What I did at first was diff the adjacent pairs of states. As if Jest diffs the states implicitly.
It hit me this week that the second diff is clearer and shorter if its purpose is to compare the initial and final cell contents, independent of how the interaction happened. So this is a long way around to suggesting it is better for the test to handle state explicitly. How the trial implementation works: the serializer calls its |
|
In this proposal, how will we customize diffing? Will Also, I think this is a lot about how we can highlight this in the output as well. Should diff snapshots show different output than regular toMatchSnapshot. |
|
Am happy to receive your direction on any or all of:
What is an example an optional diff argument? That sounds more like the original API: expect(diffSnapshot(prev, next, options?)).toMatchSnapshot();in which
Here’s an example of snap and error from #2197 (comment) to get your critique of format: exports[`CheckboxWithLabel changes attribute and text after click (diff) 1`] = `
<label>
<input
< checked={false}
> checked={true}
onChange={[Function]}
type="checkbox" />
< Off
> On
</label>
`; <label>
<input
-< checked={false}
-> checked={true}
+ checked={false}
onChange={[Function]}
type="checkbox" />
-< Off
-> On
+ Off
</label>The original API could be a third-party extension to Jest. To become a standard part of Jest:
The alternative API with
Current status:
|
|
Hm, interesting, I actually thought we could push it into toMatchDiffSnapshot: However, The question is, how would the differ work? Would it receive two pretty-printed strings and operate on those (line-by-line) or would it receive the plain data and require users to diff them, based on what object they are? If so, we could have a default recursive-diff feature in toMatchDiffSnapshot. I also encourage you to see past what we have now with |
|
Whilst this discussion has moved from the original suggestion to diffing diffs, I thought it might be worth mentioning another project that does exactly what was originally suggested - maybe you can borrow some ideas. From the issue: unexpected-react does exactly this, and can ignore extra props and child elements if desired. You can also select parts of the tree to compare against the jest snapshot. e.g. with the above example This will validate the snapshot, but only of the Bar subcomponent. Works on shallow and full DOM rendering too. Zooming in like this is also a neat way to limit the scope of tests. |
|
@bruderstein Thank you for the link. I am still interested in partial match. @cpojer On the subject of partial match, in addition to
Yes indeed, we could. Optional options and snapshot name made me timid. Me either.
Trial implementation is a hybrid of the 2 suggestions: https://gist.github.com/pedrottimark/24f182ff6249f83658b1f68a77b2b125 I will compare if it has (or could have) similar algorithm to
Sounds interesting, can you tell me more? |
|
I guess I didn't consider this so much an extension for pretty-format as I consider the implementation of this to exist on top of jest-snapshot in some way. Instead of diffing line-by-line I thought it makes sense to diff JavaScript objects before printing. I see value in both approaches, unsure which one is the best yet. |
|
@cpojer +1 for diffing the object - that seems likely to produce better output than the stringified version from |
|
@cpojer unexpected-react diffs objects, and there are a number of advantages:
The only disadvantage is serialisation and particularly deserialisation for comparison in snapshots - that's obviously a bit trickier, and much more difficult to do generically. |
|
@bruderstein sorry for what is probably an uninformed question - but aren't snapshots already stored as JSON? |
|
@developit In jest, they're stored as strings in the snapshot files. If you're using unexpected-react with jest, they're stored as JavaScript objects, with a string representation as a comment to assist in seeing the diff in version control. |
|
Ah interesting. |
|
I think this concept is quite interesting and it is really worth exploring... I'm a bit worried of how complex the API can get here. I kind of like the idea of just starting by using the latest snapshot as a base diff for the next snapshot, with as close to zero options as possible. This will allow us to just create the minimum API to support the most common use case of this feature and then incrementally make it more flexible as it is needed. I would love to leak the lest amount of implementation detail on how snapshots are stored or how the |
|
Yes, it sounds super to diff objects and serialize separately as late as possible.
I am exploring what it would look like to write a test that combines diff and subset match. |
|
Reading through this thread is a bit of a trip, since the past week I've independently followed pretty much every bit of the same winding path, starting from wanting a shape matcher, moving to a diff of diffs approach, then trying to just Most of the time, it ellipsizes common props, children, and subtrees and produces lovely minimal-context diffs as desired like: Unfortunately, without going into the details, this approach of collapsing the trees before diffing is inherently flawed and has some subtle bugs and poorly-handled edge-cases. During the process though it helped me to realize that reinventing the wheel is not the right approach here. As @pedrottimark mentioned, React already has a reconciliation algorithm that takes Apart from meaning we don't have to write our own diffing algorithm (surprisingly tricky in practice), this also adds a completely new value proposition to the tool: the ability to test the optimality of rendering changes. For example, if I don't specify Useful references for creating a new renderer: https://github.com/iamdustan/tiny-react-renderer/tree/master/src/stack and https://github.com/facebook/react/blob/master/src/renderers/testing/ReactTestRendererFiber.js |
|
@0x24a537r9 That sounds really interesting and further proof that we need to do something here. Are you planning on creating a proof-of-concept renderer for React? |
|
@cpojer So, yes and no. Yes: I'm most of the way through building a diffReact module that uses the simpler No: I'm not planning a proof-of-concept renderer--I tried looking into it, but quickly found that it's way above my current React abilities. I only started using React two months ago. That said, the core of my diffReact proof-of-concept (the smart-serialization algorithm) should be fairly transferable into a React renderer by someone familiar with the innards of React. As input it just needs some virtual DOM representation of the two trees and an object with the details of what changed between them (e.g. props changes, element insertion, element removal, children reordering, text changing). I'll be sure to post a gist back to this thread once it's ready--it might be worth bringing into Jest even in that form, since it should actually be consistent with React's transformations in theory ( |
|
Starting from Fiber (available as 1 Some functions might have had additional arguments added to them since I wrote this, but it worked fine with |
|
Although I see much potential in the diff path of this discussion, the original subset match seems to make the biggest difference (pardon pun :) as a next step forward. The small amount of remaining work is outside the control of Jest. Here is the current status:
|
|
Component shape matching would be really powerful for testing required relationships between ancestor/child HTML elements. For example, a menuitem element is required to exist in the context of a group, menu or menubar. I'd love to be able to declare generic shapes for these semantic relationships and test them against component in a library. |
|
@jessebeach What do you think about a custom tester as expected value of https://jasmine.github.io/edge/introduction#section-Custom_asymmetric_equality_tester Could you make a gist of a minimal realistic example, so that I can make sure to understand? |
|
Interesting, I hadn't considered an asymmetric equality tester. I guess it's always possible to write a custom matcher. I guess I was hoping for something a little more templatic :) Keep in mind that I'm not asking you to write this. It just strikes me that I might get what I need for free from the work you're doing. The relationship between DOM fragment and the Accessibility Tree (AX Tree) looks like this: DOM elements that have no impact on the AX Tree are ignored. So for the purposes of asserting ancestor --> child relationships for valid accessible components, we really just want to assert that an element with a role Valid: Invalid: As I write all of this, I'm coming to the conclusion that this is way out of scope for what you're trying to achieve, right? |
|
@jessebeach Thank you for the example. It suggests a descriptive test for a component. Imagine writing a generic A snapshot test of the subset seems like a regression test for accessibility relationships.
import React from 'react';
import renderer from 'react-test-renderer';
describe('Component', () => {
describe('for … set of props', () => {
const element = <Component {...props} />;
const received = renderer.create(element).toJSON();
it('renders …', () => {
expect(received).toMatchSnapshot();
});
it('renders … (accessibility)', () => {
expect(accessibleSubset(received)).toMatchSnapshot();
});
});
});Oversimplified baseline snapshot for test: Oversimplified diff when test fails: <div role="menubar">
+ <div role="article">
<div role="menuitem">Save</div>
<div role="menuitem">Edit</div>
+ </div>
</div>So, as is typical of snapshot testing, it’s specific to a component and therefore must be reviewed in context by humans, instead of being a generic “template” applied to a specific instance. P.S. If the |
|
As a quick follow up for those that are interested, I finished my diffing module and have released it as an npm package, I don't want to distract from @pedrottimark's point though, because I agree that the original subset matcher has merit of its own, so if anyone has any questions on |
|
The sort of thing that @jessebeach was suggesting is exactly what unexpected-react does (just not with the accessibility tree), and can confirm it makes for great non-brittle tests <Connect(MyComponent)>
<div className="foo bar baz">
<BigText>
<span>one</span>
</BigText>
<BigText>
<span>two</span>
</BigText>
</div>
</(Connect(MyComponent)>Can be asserted to match <div className="bar">
<span>one</span>
<span>two</span>
</div>And if it fails, you get a JSX diff (note the components identified as just wrappers are greyed out)
|
|
I'm going to close this issue but feel free to keep the discussion going and create new issues for actionable things that come out of this. |
I was answering #2197 but I found this was more a proposal than a answer to the discussion. So here we are in a dedicated issue.
For me, one of the main issue with the current implementation of the snapshot testing is that when used with big component (with deep childhood), it do not help to clearly understand witch part of the snapshot is important for the current test: what part of the snapshot I'm testing. Any change will lead to snapshot invalidation (witch is fine in simple component) and manual approval. Mocking sub module helps to reduce this, but it's not always possible or wanted. So we have to manage lot's of snapshot manual update to validate an unrelated change.
One solution could be to specify assertion on the shape of the rendered vdom (not the detailed part):
This will ignore other change than the one who will make this assertion falsy.
This could lead to more robust tests assertion and less maintenance to upgrade the snapshot.
That's a quick thought. Feel free to discuss or closing the issue.
The text was updated successfully, but these errors were encountered: