Adding jest and jest babel preset to the react-native init command #9719

kentaromiura · 2016-09-02T16:07:02Z

Adding jest and its presets to the react-native init command

Test plan (required)
run react-native init foo (using npm link to use the local react-native version)
inside foo there are now a .babelrc file and the package.json is set up as described by
https://facebook.github.io/jest/docs/tutorial-react-native.html#setup

ghost · 2016-09-02T16:07:22Z

By analyzing the blame information on this pull request, we identified @ide and @bestander to be potential reviewers.

davidaurelio · 2016-09-02T16:23:48Z

Maybe it’s only me, but I don’t think this should be part of the init command. Or at least be behind a flag.

satya164 · 2016-09-02T16:28:34Z

cc @cpojer

We were talking about this today.

kentaromiura · 2016-09-02T16:29:27Z

We're doing the same with create react app: https://twitter.com/dan_abramov/status/771496951858733057
This is setting up the jest configuration, allowing people to write tests (with snapshots) against their components with 0 configuration needed.
It doesn't change anything as you need to run npm test to run it and it will be easy to swap if someone wants to use a different test runner.

cpojer · 2016-09-02T16:29:28Z

@davidaurelio create-react-app ships like this too and I believe it is a really good default. Most people are confused about testing RN and Jest is the only framework that can provide a real unmocked version of react-native. The mocks we provide for components help with snapshot testing and a lot of people are adopting it.

Our open source software should be all about integrated experiences. It shouldn't be about Jest or packager or any one tool, but literally just be one thing that seamlessly works together. If testing isn't a part of this, then that is a decision we have to make, but then we also have to discuss why we ship a packager, dev server and bundler with a UI framework in this repo ;)

Ideally I would like the react native CLI to go the same way that create-react-app is going. I think that would be ideal and we could hide this dependency completely from the user. And alternative approach for this PR could be to ship jest as a dependency instead of devDependency for react-native and then npm test would simply be react-native test or something like that. I'm also hoping we can merge the jest-react-native preset into react-native completely (and have it be versioned together with rn). That way we get even fewer dependencies.

cc @vjeux @gaearon @mkonicek for their thoughts.

cpojer · 2016-09-02T16:38:57Z

local-cli/generator/index.js

+        packageJSONPath
+      )
+    );
+    packageJSON.scripts.test = 'jest'


davidaurelio · 2016-09-02T16:44:42Z

After personal discussion with @kentaromiura and @cpojer: I would be happy with an opt-out flag.

kentaromiura · 2016-09-02T16:44:44Z

After discussing this offline I'll add an opt out flag any suggestion for the name?

davidaurelio · 2016-09-02T16:45:15Z

--minimal ? --without-jest?

cpojer · 2016-09-02T16:47:50Z

--without-jest sounds good. I totally agree that an opt-out is a good idea – I don't want people to be locked in into any one test framework. That would be terrible. However, I care about providing the best experience to people out of the box and for RN, there isn't another tool that works and shares infra with packager.

satya164 · 2016-09-02T16:50:44Z

Worth mentioning that we were talking about adding different kind of templates. I think a minimal template would be useful if we want to have one without jest.

cc @bestander @mkonicek

bestander · 2016-09-02T16:51:09Z

We need to rework CLI to have responsive/progressive output first.
People get really confused with 5 minutes install command.

bestander · 2016-09-02T16:51:47Z

Also please add opt-out to e2e tests

cpojer · 2016-09-02T16:56:00Z

I would really like to decouple adding a test framework from the CLI rewrite because no one is actively working on this and people are continuously asking how to setup react-native testing – this does exactly that for them. With an opt-out, it will be the same as before.

In the future we can rewrite the cli and make it more awesome; add separate templates to choose from and more options. For now, I'd like to figure out a way to get this in that makes most people happy so we can move forward and iterate towards the ideal CLI later.

I also wanna point out the Jest 15 improvements we made for the react-native preset: http://facebook.github.io/jest/blog/2016/09/01/jest-15.html#jest-react-native-improvements

ghost · 2016-09-02T17:22:31Z

@kentaromiura updated the pull request - view changes

cpojer · 2016-09-02T17:25:12Z

local-cli/generator/index.js

@@ -27,6 +26,11 @@ module.exports = yeoman.generators.NamedBase.extend({
      type: Boolean,
      defaults: false
    });
+    this.option('skip-jest', {
+      desc: 'Skip installing jest',


"Jest" (capitalized) in writing please. jest is the name of the cli tool for "Jest".

bestander · 2016-09-02T17:49:59Z

@kentaromiura and @cpojer, that is a huge improvement to the platform, having an idiomatic test tool, I am sure the community will be happy.

A few concerns:

How will people find out that there is a full working Jest coming with RN?
Could you do a small Blog post as intro?
What keeps it from degrading?
This setup may break when babel and jest subdependencies go out of sync for some reason.
We should add a simple test in out e2e scenario that runs a dummy jest test on a fresh install.

because no one is actively working on this

That is my point.
Because no one could be blamed for having a bad CLI why the one who wants to add extra weight on it does a little bit of improvement?
My current problem is that we don't have any output while npm install is running.
Is it possible to fix this within current PR?

cpojer · 2016-09-02T18:15:45Z

@bestander I think what you recommend makes sense. We should add some more progress output during npm install, but that should happen in a separate PR.

We can do a blog post on the RN blog but both facebook.github.io/jest/blog/2016/07/27/jest-14.html and facebook.github.io/jest/blog/2016/07/27/jest-15.html mention it. Let's figure that out after landing it – we'll have time until the release.
Jest itself has an integration test that is run for every change that makes sure that jest works with the latest released version of react-native. We can also add one that makes sure Jest works with the latest pre-release version of react native. Should be simple :)

bestander · 2016-09-02T18:33:43Z

Ok, PR with a bit better progress is expected from @kentaromiura ;)
Does it make sense to add a sample jest test to the templates in this PR? Then we could run it on CI and we have all infra prepared, see https://github.com/facebook/react-native/blob/master/scripts/run-ci-e2e-tests.js

cpojer · 2016-09-02T18:36:58Z

Oh yeah. We should literally just make sure the default component doesn't throw: https://github.com/facebookincubator/create-react-app/blob/master/template/src/App.test.js#L5 – that is an awesome example and doesn't overwhelm people.

ghost · 2016-09-05T12:57:33Z

@kentaromiura updated the pull request - view changes

mkonicek · 2016-09-05T19:16:58Z

I would really like to decouple adding a test framework from the CLI rewrite

@cpojer I totally agree, ship this now and we can add templates later.

Thanks a lot for doing this @kentaromiura.

bestander · 2016-09-06T13:43:00Z

I would really like to decouple adding a test framework from the CLI rewrite
@cpojer I totally agree, ship this now and we can add templates later.

There are three issues here:

CLI rewrite
Adding more dependencies to init command, making it perceived more slow
Adding e2e tests

I agree that 1 is not an option for this PR.
2 & 3 are affected by this change and it does not take too long to fix them.
I'll be happy to help addressing those things, the PR is quite awesome indeed.

kentaromiura · 2016-09-06T15:28:54Z

it's not in the scope of this PR
there are more command running so it will show some progress, the dependencies added are pretty small compared to react-native, the benefit is that one can then run npm test and run/write tests
is not in the scope of this PR, e2e will be addressed later, I'm adding a basic it works snapshot test (for both android and ios). we'll definitely can address that on a separate PR.

bestander · 2016-09-06T15:34:38Z

Re 3, If you add a simple it test then adding an npm test here will take you 10 more seconds and 3 lines of code.
How come is it out of scope of this PR?

ghost · 2016-09-09T15:06:02Z

@kentaromiura updated the pull request - view changes

ghost · 2016-09-09T15:16:23Z

@kentaromiura updated the pull request - view changes

bestander · 2016-09-09T15:28:32Z

That is amazing, @kentaromiura, so easy to create a snapshot test now!
Let's wait for tests to pass and shipit

cpojer · 2016-09-09T15:34:12Z

This is awesome indeed.

I have one recommendation: a lot of beginners use react and a lot of people in general aren't sure what the value of testing is. Snapshot testing is definitely the right answer to test react components, however I believe we shouldn't ship with a default one. I did this in my initial PR to create-react-app but @gaearon brought up some really good points that they will be confusing. One concern is that undoubtedly this test will fail as soon as someone starts modifying their app and there isn't much value in this initial test. Instead, can we change it so it doesn't have an assertion at all and simply makes sure rendering doesn't throw? See https://github.com/facebookincubator/create-react-app/blob/master/template/src/App.test.js

We can then add documentation on how to use snapshot testing on the website and upsell people into creating snapshots. I think they aren't very valuable when you are just getting started to write code and are much more useful once you are done with a few components. What do you think?

bestander · 2016-09-09T16:16:36Z

Also need to add local-cli/generator/ to jest ignore

ghost · 2016-09-09T16:22:36Z

@kentaromiura updated the pull request - view changes

ghost · 2016-09-09T16:42:44Z

@kentaromiura updated the pull request - view changes

bestander · 2016-09-09T18:55:42Z

@facebook-github-bot shipit

ghost · 2016-09-09T18:56:14Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

cpojer · 2016-09-09T19:02:24Z

@bestander can you follow up and let us know what kind of documentation or announcement we should be preparing and for when? :)

cpojer · 2016-09-09T19:02:53Z

@bestander can you follow up and let us know what kind of documentation or announcement we should be preparing and for when? :)

bestander · 2016-09-09T19:09:59Z

@cpojer easy :)
We just need a blog post in http://facebook.github.io/react-native/blog/.
It is a matter of writing a markdown with code snippets and maybe a video/gif of a successful run.
@kentaromiura are you up to educate the people about the greatness of snapshot testing?

Summary: Adding jest and its presets to the react-native init command **Test plan (required)** run react-native init foo (using `npm link` to use the local `react-native` version) inside foo there are now a .babelrc file and the package.json is set up as described by https://facebook.github.io/jest/docs/tutorial-react-native.html#setup Closes facebook/react-native#9719 Differential Revision: D3843037 Pulled By: bestander fbshipit-source-id: 004e27ebd3f257a202ed43f378d6fe6cc23ced52

Adding jest and jest babel preset to the react-native init command

292605c

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 2, 2016

cpojer reviewed Sep 2, 2016
View reviewed changes

local-cli/generator/index.js

packageJSONPath

)

);

packageJSON.scripts.test = 'jest'

Copy link

Contributor

cpojer Sep 2, 2016

Choose a reason for hiding this comment

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

;

Adding an opt-out --skip-jest flag

56b8930

cpojer reviewed Sep 2, 2016
View reviewed changes

Capitalise the name of the tool.

f4ce034

bestander added the GH Review: needs-revision label Sep 6, 2016

Adding a default snapshot test

9309215

Add e2e test

596a125

bestander added GH Review: accepted and removed GH Review: needs-revision labels Sep 9, 2016

remove default __snapshots__ folder

9c31f2b

Address latest comments

d2ecd74

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 9, 2016

ghost closed this in 8689b0f Sep 10, 2016

This pull request was closed.

Adding jest and jest babel preset to the react-native init command #9719

Adding jest and jest babel preset to the react-native init command #9719

Conversation

kentaromiura commented Sep 2, 2016

ghost commented Sep 2, 2016

davidaurelio commented Sep 2, 2016

satya164 commented Sep 2, 2016

kentaromiura commented Sep 2, 2016

cpojer commented Sep 2, 2016

cpojer Sep 2, 2016

Choose a reason for hiding this comment

davidaurelio commented Sep 2, 2016

kentaromiura commented Sep 2, 2016

davidaurelio commented Sep 2, 2016

cpojer commented Sep 2, 2016

satya164 commented Sep 2, 2016

bestander commented Sep 2, 2016

bestander commented Sep 2, 2016

cpojer commented Sep 2, 2016

ghost commented Sep 2, 2016

cpojer Sep 2, 2016

Choose a reason for hiding this comment

bestander commented Sep 2, 2016 • edited Loading

cpojer commented Sep 2, 2016

bestander commented Sep 2, 2016

cpojer commented Sep 2, 2016

ghost commented Sep 5, 2016

mkonicek commented Sep 5, 2016 • edited Loading

bestander commented Sep 6, 2016 • edited Loading

kentaromiura commented Sep 6, 2016

bestander commented Sep 6, 2016

ghost commented Sep 9, 2016

ghost commented Sep 9, 2016

bestander commented Sep 9, 2016

cpojer commented Sep 9, 2016

bestander commented Sep 9, 2016

ghost commented Sep 9, 2016

ghost commented Sep 9, 2016

bestander commented Sep 9, 2016

ghost commented Sep 9, 2016

cpojer commented Sep 9, 2016

cpojer commented Sep 9, 2016

bestander commented Sep 9, 2016

bestander commented Sep 2, 2016 •

edited

Loading

mkonicek commented Sep 5, 2016 •

edited

Loading

bestander commented Sep 6, 2016 •

edited

Loading