Skip to content
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

Document testing #538

Merged
merged 6 commits into from
Sep 1, 2016
Merged

Document testing #538

merged 6 commits into from
Sep 1, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Sep 1, 2016

Coming soon!

@ghost ghost added the CLA Signed label Sep 1, 2016

### Command Line Interface

When you run `npm test`, Jest will launch in the watch mode. Every time you save a file, it will re-run the tests, just like `npm start` recompiles the code.
Copy link
Contributor

@kentcdodds kentcdodds Sep 1, 2016

Choose a reason for hiding this comment

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

I would recommend against watch mode being the default. Many CI systems will run npm test by default. In my experience, it's more common to have npm test run the tests once (with --coverage) and have a watch:test script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh dang. I just saw the note about how jest is smart and wont watch on CI... That's a super feature!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's something we do in react-scripts as suggested by @ForbesLindesay but maybe Jest could do that by default! cc @cpojer

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty amazing what you can do when you just forward npm scripts onto your own binary :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Dan: wanna send a PR for that? I'm happy to ship that as a default. As we expand interactive watch, there are probably few reasons to not use it as a default.


### Writing Tests

To create tests, add `it()` blocks with the name of the test and its code. You may optionally wrap them in `describe()` blocks for logical grouping but this it not required.
Copy link
Contributor

Choose a reason for hiding this comment

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

I might even add (nor is it recommended). Here's why I feel that way..

@ghost ghost added the CLA Signed label Sep 1, 2016
// ...
"scripts": {
// ...
"test": "react-scripts test --env=jsdom"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting! I haven't followed CRA too closely, but this appears to be the first instance of anything that resembles config. Is this the decided upon method of config? Is this something to be concerned about? I realize that this is a pretty useful optimization. Just wanted to call it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s mostly there to stick out so people are incentivized to figure out ways to test their stuff without jsdom. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future, as mentioned, we’ll be recommending snapshot testing, and when it becomes the default test we ship, we’ll remove env=jsdom from the default test script.

@kentcdodds
Copy link
Contributor

This is fantastic 👏

@ghost ghost added the CLA Signed label Sep 1, 2016
@@ -144,7 +151,8 @@ Currently it is a thin layer on top of many amazing community projects, such as:
* [Babel](http://babeljs.io/) with ES6 and extensions used by Facebook (JSX, [object spread](https://github.com/sebmarkbage/ecmascript-rest-spread/commits/master), [class properties](https://github.com/jeffmo/es-class-public-fields))
* [Autoprefixer](https://github.com/postcss/autoprefixer)
* [ESLint](http://eslint.org/)
* and more.
* [Jest](http://facebook.github.io/jest) (it’s [not bad anymore!](http://facebook.github.io/jest/blog/2016/09/01/jest-15.html))
Copy link
Contributor

Choose a reason for hiding this comment

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

haha, I'm not sure what to think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

haha, yeah, perhaps changing the wording to be more kind and positive. Afterall, real people worked hard on Jest before too.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, the thing is, most people that will read this have never heard of Jest and this gives them a negative impression. The people that knew about Jest being terrible are the ones that used React years ago already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could also say "Jest" and highlight the two latest release blog posts with explanation of great features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it 😄


>Note: this feature is available with `[email protected]` and higher.

Create React App uses [Jest](https://facebook.github.io/jest/) as its test runner. To prepare for this integration, we did a [major revamp](https://facebook.github.io/jest/blog/2016/09/01/jest-15.html) of Jest so if you heard bad things about it, give it another try.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too. "We did a major revamp to improve new user experience"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did we not? “Jest is bad” is well and alive, no need to look further than the recent issue. I do think we need to address this concern in text. I don’t think it makes us look bad, just honest about past problems and our willingness to solve them.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that makes sense. Can we maybe say "The only resemblance of Jest with the test framework from a year ago is its name?" That sounds more uplifting. I just wanna avoid giving people a bad impression if they haven't even heard about Jest before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s discuss in a follow up PR if you’d like to make a pass over this. But as you know, it’s better to under-promise and over-deliver. I would give it a few months and then revisit when the perception is turned around thanks to word of mouth.

@gaearon gaearon merged commit 58195ec into master Sep 1, 2016
@gaearon gaearon deleted the gaearon-patch-1 branch September 1, 2016 23:40
});
```

This test mounts a component and makes sure that it didn’t throw during rendering. Tests like this provide a lot value with very little effort so they are great as a starting point, and this is the test you will find in `src/App.test.js`.

Choose a reason for hiding this comment

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

"lot of value"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR? :-)

@trevordmiller
Copy link

Great stuff. Looking forward to jsdom getting ripped out and Node only tests + snapshots becoming the default 💪

stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
* Document testing

* Update README.md

* Update README.md

* Clarify our recommendations on testing

* Okay, that was too much. :-)

* Add a few more things
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Document testing

* Update README.md

* Update README.md

* Clarify our recommendations on testing

* Okay, that was too much. :-)

* Add a few more things
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants