Skip to content

simplify with React Testing Library #1

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

Open
wants to merge 2 commits into
base: testing-docs
Choose a base branch
from

Conversation

kentcdodds
Copy link

Hey Sunil!

I'll add some inline comments, but here's how I want you to think about these changes. This is what I would do if I were 100% responsible for writing this documentation. I understand that you operate under different constraints and may not be able to accept all of these contributions. That's fine. Please take what you like and don't "throw the baby out with the bathwater." (That's a terrible phrase... I need a new one).

Several of the tests/components had bugs. So I pulled all the code out to a separate repo to make sure everything worked: https://github.com/kentcdodds/testing-docs-rtl-examples

Happy to iterate on this. I really want people getting into React to have stellar resources for testing! Thanks!

}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you know that the project is currently not setup to auto-format markdown (and all the code examples in the markdown). It would work if you remove "parser": "flow", from this file though :)

<span>Hey, stranger</span>
) : (
<h1>Hello, {props.name}!</h1>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was switched (incorrect) before

import { render } from "react-dom";
import { act } from "react-dom/test-utils";

let container = null
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 bye bye boilerplate.


act(() => {
render(<Hello name="Flarnie" />, container);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this additional test gives us no more confidence, so I don't have it in my version.

) : (
<details>
<summary>{user.name}</summary>
<strong>{user.age}</strong> years old
<summary aria-label="name">{user.name}</summary>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the labels, the data would be difficult to understand for people using screen readers. Honestly, in a real app you'd probably have real labels and/or icons for this data (which would also have labels). So I'm adding these here so the tests can resemble what this kind of thing would look like in the real world.

clearTimeout(timeoutID);
};
},
[props.onSelect]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gave a linting warning because we didn't include props, so I destructured onSelect instead.

},
[props.onSelect]
);
return [1, 2, 3, 4].map(choice => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The { needed to be a (


it("should select null after timing out", () => {
const yields = [];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically it's better to use mock functions because you can assert on the calls specifically. I'd rather encourage using mock functions.

return root.toJSON();
}
test("preserves its structure", () => {
expect(render(<Hello />).container).toMatchInlineSnapshot(`
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using toMatchInlineSnapshot, people avoid one of the biggest dangers of snapshots: "Huge snapshots that nobody reviews." If your snapshot is inline, then you're less likely to let it get huge.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(working on a lint rule to enforce snapshot size for inline snaps jest-community/eslint-plugin-jest#244)

});
```

### Snapshot testing {#snapshot-testing}

Frameworks like Jest also let you save 'snapshots' of data. Combined with react-test-renderer, we can save component structures, and verify they don't break in the future. It's typically better to make more specific assertions than to use snapshots (or at least ensure that the snapshot is small) because these kinds of tests include implementation details meaning they break easily and teams can get desensitized to snapshot breakages.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using react-test-renderer because snapshots of that includes a TON of implementation details. Snapshotting the DOM isn't terrific either, but it doesn't include nearly as many implementation details. Therefore you'll get fewer false negatives.

};
jest.spyOn(global, "fetch").mockImplementation(() =>
window.fetch.mockImplementationOnce(() =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes to mocks, I usually set resetMocks or clearMocks globally in Jest. There is rarely a reason to maintain mock state across tests.

https://jestjs.io/docs/en/configuration#resetmocks-boolean

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a comment about that so people can look into it if they want to reduce the boilerplate

@threepointone threepointone force-pushed the testing-docs branch 12 times, most recently from e3d3125 to 16e166d Compare August 8, 2019 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants