[docs] Add unit testing guide #6678
Conversation
|
Deploy preview for using-postcss-sass failed. Built with commit 01898bb https://app.netlify.com/sites/using-postcss-sass/deploys/5b69625702ed8362a8bbd799 |
|
Deploy preview for using-drupal ready! Built with commit 01898bb |
|
Deploy preview for gatsbygram ready! Built with commit 01898bb |
|
Well done! Really like it 👍 You described the setup I currently also have. I could write about:
We'd need to figure out how to structure this. I'd propose a section on the Guides with the title Testing. We then would have your Unit testing. How could we group/add the other two bulletpoints there? I'll try to understand the testing on Gatsby's GraphQL etc. with @kkemple help. That could also go into your Unit testing then. |
|
Great. Yes, I agree a new testing section would be best, and end-to-end testing would be really useful. Something on CI might be good too. We need to work out where to best draw the line, as this obviously isn't meant to be a general guide to React testing, but rather a Gatsby-specific guide. I'd like to do a guide on Storybooks, as they're really useful and need some particular Gatsby setup. |
Agree! The guides on Styling could be a good reference for the rest as there's also only really little to setup (but it still is getting explained). So for Cypress it would be more of an explanation of something like:
|
|
I've not tried testing components that include graphql. You may need to add babel-plugin-remove-graphql-queries to the preprocessor. |
|
Woah, this is great! I wonder if we can add something to
/cc @shannonbux for input on structuring docs |
|
@pieh that would be really useful. We are onboarding a number of developers into Gatsby and every single one has hit that wall. |
docs/docs/unit-testing.md
Outdated
| install Babel 7 as it's required by Jest. | ||
|
|
||
| ```sh | ||
| yarn add -D jest babel-jest react-test-renderer 'babel-core@^7.0.0-0' @babel/core identity-obj-proxy @babel/plugin-proposal-class-properties @babel/plugin-proposal-optional-chaining |
There was a problem hiding this comment.
You should change yarn to npm as that's what is used all through the docs.
|
Deploy preview for gatsbyjs failed. Built with commit 01898bb https://app.netlify.com/sites/gatsbyjs/deploys/5b69625602ed8362a8bbd78d |
| For this guide, you will be starting with `gatsby-starter-blog`, but the | ||
| concepts should be the same or very similar for your site. | ||
|
|
||
| First you need to install Jest and some more required packages. You need to |
There was a problem hiding this comment.
Is this actually necessary with v2 since Gatsby is using Babel 7 already? Seems you would only need to install the presets and plugins? Also, looks like presets are missing from install step unless they are already present for Gatsby itself.
There was a problem hiding this comment.
Good point: the presets are missing, and do need installing. I'll add them. Even though, as you say, Babel 7 is installed by Gatsby, it seems that unless it is added as a dependency of the project, the wrong version is picked-up. Likewise plugins are not found unless specifically included. All my testing was done on the latest beta.
There was a problem hiding this comment.
As for your point about whether this is required: yes, it is, as jest doesn't know that it needs to use babel, as there's no .babelrc. You could add one, but of course that is then also used in the Gatsby build. Using a Jest preprocessor seems to be the best way (and is the way used in the Gatsby codebase). I tried to stick as close as possible to the way it's done in the Gatsby site itself.
There was a problem hiding this comment.
Oh yeah, using preprocessor for sure! I meant having to install babel 7 since it's already used by gatsby, but seems that it is in fact needed!
There was a problem hiding this comment.
Great stuff! Thanks @ascorbic 👍
I think we can merge this now, and then link it up to the docs once we know the best place to put it?
As suggested above, Guides -> Testing -> Unit Testing seems like a sensible place.
docs/docs/unit-testing.md
Outdated
| matching. | ||
|
|
||
| If you make changes that mean you need to update the snapshot, you can do this | ||
| by running `npm test -- -u`. |
There was a problem hiding this comment.
I believe with npm version > 5 the -- is unnecessary for passing additional args
There was a problem hiding this comment.
Let's stick with -- here, as Gatsby supports Node v6 which ships with npm v3.
docs/docs/unit-testing.md
Outdated
|
|
||
| ```js | ||
| import React from "react" | ||
|
|
There was a problem hiding this comment.
Spacing here should be consistent with the code further down. L230. No new line between package imports
| extension `.test.js` or `.spec.js`. You are telling Jest to ignore any tests in | ||
| the `node_modules` or `.cache` directories. | ||
|
|
||
| The `moduleNameMapper` section works a bit like webpack rules, and tells Jest |
There was a problem hiding this comment.
I just changed this in my last review :) webpack is officially written as 'webpack' rather than 'Webpack'. Awkward, I know! See usage on https://webpack.js.org/
docs/docs/unit-testing.md
Outdated
| how to handle imports. You are mainly concerned here with mocking static file | ||
| imports, which Jest can't handle. A mock is a dummy module that is used instead | ||
| of the real module inside tests. It is good when you have something that you | ||
| can't or don't want to test. You have two types that you mock: stylesheets, and |
There was a problem hiding this comment.
Why not just
You can mock anything
docs/docs/unit-testing.md
Outdated
| } | ||
| ``` | ||
|
|
||
| This means you can now run tests by typing `npm test`. If you want you could |
There was a problem hiding this comment.
npm run test for verboseness
docs/docs/unit-testing.md
Outdated
| also add a script that runs `jest --watchAll` to watch files and run tests when | ||
| they are changed. | ||
|
|
||
| Now, run `npm test` and you should immediately get an error like this: |
docs/docs/unit-testing.md
Outdated
| the snapshot being written. This is created in a `__snapshots__` directory next | ||
| to your tests. If you take a look at it, you will see that it is a JSON | ||
| representation of the `<Bio />` component. You should check your snapshot files | ||
| into your SCM repository so that anyone can see if the tests have stopped |
There was a problem hiding this comment.
Wording.
so that any changes are tracked in history
There was a problem hiding this comment.
And that tests can run by CI solutions like travis (which could have it's own guide that could be linked in the future)
docs/docs/unit-testing.md
Outdated
| "use strict" | ||
| import React from "react" | ||
| const gatsby = jest.genMockFromModule("gatsby") | ||
| gatsby.Link = ({ to, ...props }) => <a href={to} {...props} /> |
There was a problem hiding this comment.
I encountered this issue because my NavItem component uses activeClassName on Link:
Warning: React does not recognize the `activeClassName` prop on a DOM element. If you intentionally want i
t to appear in the DOM as a custom attribute, spell it as lowercase `activeclassname` instead. If you accident
ally passed it from a parent component, remove it from the DOM element.
So we should write something like?
gatsby.Link = ({ to, activeClassName, activeStyle, ...props }) => (
<a href={to} className={activeClassName} style={activeStyle} {...props} />
);There was a problem hiding this comment.
It also changes the emotion snapshots. It removes the styling of the activeClassName content. I think it doesn't recognize in my test that it now should display the active style because the to prop is the same as the current location.
There was a problem hiding this comment.
Removing the mocking for gatsby.Link just caused more errors :/
There was a problem hiding this comment.
Yes, it will change snapshots because it's replacing a component with a mock. That's expected.
There was a problem hiding this comment.
Sorry, wasn't clear enough. It changes the snapshot of my styles (via jest-emotion). The removed styles are the ones from activeClassNames.
FAIL src/elements/__tests__/NavItem.js
● NavItem › renders with active style
expect(value).toMatchSnapshot()
Received value does not match stored snapshot "NavItem renders with active style 1".
- Snapshot
+ Received
@@ -60,37 +60,12 @@
margin: 1.5rem 0.25rem;
padding: 6px 16px;
}
}
- .emotion-1 {
- color: #765e31;
- background: rgba(238,205,81,0.2);
- }
-
- .emotion-1:focus {
- color: #765e31;
- }
-
- @media (max-width:800px) {
- .emotion-1 {
- color: white !important;
- background: none;
- margin-bottom: 0;
- padding-bottom: 0.5rem;
- }
-
- .emotion-1:after {
- -webkit-transform: translateY(0px);
- -ms-transform: translateY(0px);
- transform: translateY(0px);
- }
- }
-
<a
- aria-current="page"
- class="emotion-0 emotion-1"
+ class="emotion-0"
data-testid="navitem-bilbo"
href="/"
>
Brand
</a>
27 | );
28 |
> 29 | expect(container.firstChild).toMatchSnapshot();
| ^
30 | });
31 |
32 | test('renders with correct attributes', () => {
at Object.<anonymous> (src/elements/__tests__/NavItem.js:29:34)
|
Same snapshot error like in #6678 (review) described occurs |
|
You can't expect the snapshots to match, as they're being generated in different ways. The important things is whether they change in future when your code changes. |
|
The specific issue here is that the mock doesn't have the logic to know if it's the active link or not. |
|
Alright, I understand that. Just wished that I still could test that (with the mock — the „old“ MemoryRouter way still works of course). Then I‘d add one or two sentences about the fact that this way you can’t test the active state. |
There was a problem hiding this comment.
@ascorbic looks great! just left one comment
@pieh It seems like Testing ought to be a new category now. I'll do that and put these docs there.
@S-unya, have you chatted with anyone at Gatsby about your experience onboarding devs to Gatsby? It'd be super valuable to hear what major roadblocks you've noticed calendly.com/shannon-soper <--sign up there (others are welcome to, too!)
|
|
||
| Here is the example: | ||
|
|
||
| ```js |
There was a problem hiding this comment.
What kind of file should someone use this code example for? Maybe readers will just know, but wanted to make sure if it's supposed to go in a specific type of file or you have a suggestion, make it clear here as you did for other code examples!
There was a problem hiding this comment.
As in where they'd be using StaticQuery rather than a page query? No problem.
|
@shannonbux That's great. Yes, I definitely think testing needs a category. I have more to come! Storybooks I think might be next, and CI/CD would be a good one. I've learnt stuff just from seeing the setup you have here in the gatsby repo, tbh. Regarding onboarding, @S-unya and I are colleagues, and we've been running workshops for the rest of our team. A lot of these docs have come out of our experiences with this. It would be great to chat about that at some point. |
|
Deploy preview for using-styled-components failed. Built with commit 17b86d2 https://app.netlify.com/sites/using-styled-components/deploys/5b64140e3813f06148ecae01 |
|
Deploy preview for using-contentful failed. Built with commit 17b86d2 https://app.netlify.com/sites/using-contentful/deploys/5b64140e3813f06148ecae04 |
|
Thanks for being proactive and creating these docs when you see a need for
them! Feel free to schedule something with me when you're ready to chat
about anything else you think we can improve (once you schedule it, I'll
loop in an internal dev too so we can have a well-rounded conversation):
calendly.com/shannon-soper
…On Fri, Aug 3, 2018 at 2:10 AM, Matt Kane ***@***.***> wrote:
@shannonbux <https://github.com/shannonbux> That's great. Yes, I
definitely think testing needs a category. I have more to come! Storybooks
I think might be next, and CI/CD would be a good one. I've learnt stuff
just from seeing the setup you have here in the gatsby repo, tbh.
Regarding onboarding, @S-unya <https://github.com/S-unya> and I are
colleagues, and we've been running workshops for the rest of our team. A
lot of these docs have come out of our experiences with this. It would be
great to chat about that at some point.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6678 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Ae9o2rZlKRGx0_3N81VMVadUFtWoVgppks5uNAVmgaJpZM4VbAKU>
.
|
|
Thank you very much for this write-up! Would have been lost without it in my new project that I based on both Storybook and Jest (and what a weird time to start a project like that with Babel moving to v7, Storybook moving to v4 and Gatsby moving to v2) After the hours spent getting everything working few days ago I decided to save anyone trying this in the future the hassle and made a Storybook/Jest starter On that note, I would be very interested in helping writing up future test documentations whether for jest or storybook or otherwise as testing is an enthusiasm of mine And truly kudos to Gatsby, Storybook, Jest, Babel, Enzyme and all other open-source projects for your continuous and commendable work ❤️ |
|
@Mathspy thanks for making this a starter! It was on my list of things to do once we figured out the best approach for testing. So awesome! 🙌👏 |
docs/docs/unit-testing.md
Outdated
| TypeError: Cannot read property 'history' of undefined | ||
| ``` | ||
|
|
||
| This is an error in `gatsby-link`, although `react-router-dom` would complain |
There was a problem hiding this comment.
This needs updated now that we've moved to @reach/router
There was a problem hiding this comment.
OK, done. It simplifies it quite a bit, as @reach/router's Link is a lot happier running in a test environment without a Router than react-router was.
| ], | ||
| "globals": { | ||
| "__PATH_PREFIX__": "" | ||
| }, |
There was a problem hiding this comment.
I received the following error:
jestjs/jest#6766
testURL: 'http://localhost' solves this issue 👍
There was a problem hiding this comment.
Good spot. Added, along with explanation.
|
FYI, switching to @reach/router solved the issue with the snapshot tests for the |
Because of jestjs/jest#6766
| ```js | ||
| // loadershim.js | ||
|
|
||
| global.___loader = { |
There was a problem hiding this comment.
what's ___loader? and what is enqueue? Where do they come from originally?
There was a problem hiding this comment.
They're part of Gatsby's runtime. They were exposed as globals for some reason or another a while ago and need refactored to a normal module interface.
|
@shannonbux let's get these into the link structure ASAP :) |
* Add unit testing guide * Typo fixes * Proofreading * Add info on testing gatsby-link * Updates for style * Switch yarn to npm * Add babel presets to npm install * Small edits * Small edits from review * Add loader shim and __PATH_PREFIX__ global * Add testing graphql doc * Rename * Mock Gatsby rather than removing graphql and using MemoryRouter * Add filename for gatsby mock * Update LInk mock so all props can be displayed * Details on why you might not want to mock Link * Add not on use of StaticQuery, and a little on component purity * Update for @reach/router * Add testURL to Jest config Because of jestjs/jest#6766
* Add unit testing guide * Typo fixes * Proofreading * Add info on testing gatsby-link * Updates for style * Switch yarn to npm * Add babel presets to npm install * Small edits * Small edits from review * Add loader shim and __PATH_PREFIX__ global * Add testing graphql doc * Rename * Mock Gatsby rather than removing graphql and using MemoryRouter * Add filename for gatsby mock * Update LInk mock so all props can be displayed * Details on why you might not want to mock Link * Add not on use of StaticQuery, and a little on component purity * Update for @reach/router * Add testURL to Jest config Because of jestjs/jest#6766
* Add unit testing guide * Typo fixes * Proofreading * Add info on testing gatsby-link * Updates for style * Switch yarn to npm * Add babel presets to npm install * Small edits * Small edits from review * Add loader shim and __PATH_PREFIX__ global * Add testing graphql doc * Rename * Mock Gatsby rather than removing graphql and using MemoryRouter * Add filename for gatsby mock * Update LInk mock so all props can be displayed * Details on why you might not want to mock Link * Add not on use of StaticQuery, and a little on component purity * Update for @reach/router * Add testURL to Jest config Because of jestjs/jest#6766
Add a unit testing doc