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

Make testing a dependency, not built in #331

Closed
ryanflorence opened this issue Aug 2, 2016 · 23 comments
Closed

Make testing a dependency, not built in #331

ryanflorence opened this issue Aug 2, 2016 · 23 comments

Comments

@ryanflorence
Copy link

You can't introduce testing w/o introducing religion, unfortunately. For example, I refuse to test my UI in an environment where I can't actually render it in a browser and step through problems in real browsers to fix bugs. Asking folks to eject because they don't like your religion seems a little harsh.

Seems like for testing we ought to be able to have a module on npm called react-scripts-jest, require it in the app's package.json, and then have npm test be something like:

{
  "scripts": {
    "test": "react-scripts-jest"
  }
}

That would be the default or create-react-app, but other folks can create react-scripts-karma or just roll their own inside the project.

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

Who would maintain react-scripts-karma?
If we make breaking changes to react-scripts that break it, whose job will it be to fix it?

@kennetpostigo
Copy link

This sorta seems familiar to hyperterm plugins. Which most people seem to like.

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

I understand the allure of this, I’m just not sure what the plan is with regards to maintenance.

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

Also, I’m curious: would you rather prefer we don’t ship testing than we ship browserless testing?

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

It seems that if something like react-scripts-karma is maintained by the community, they could implement this proposal even today.

Is changing "test": "react-scripts-jest" to "test": "react-scripts-karma" better in some way than changing "test": "react-scripts test" to "test": "react-scripts-karma"? Since in both cases it’s a community effort and not supported out of the box, I don’t really see the difference, but maybe I’m missing something and I’d love to learn what you think.

@AvivRubys
Copy link

As I understand this, there's nothing stopping any community effort from replacing "test": "react-scripts test with "test": "some-community-testing-package".
The only benefit switching create-react-app to a multi package model would give you, is being able to opt out of jest being installed in your project when you're in development. Which doesn't seem like a very big problem.
At the end of the day, it's an opinionated vs configurable sort of issue, but in this case the opinion is easily overridable.

@cpojer
Copy link
Contributor

cpojer commented Aug 4, 2016

I think what a lot of people are missing is that we don't have a single line of config inside of the created app that is related to testing, except of course for the scripts entry to run react-script's test command. Anyone can just simply install whatever they like and update the test command with no additional overhead. I think this is reasonable.

@mxstbr
Copy link
Contributor

mxstbr commented Aug 4, 2016

I agree with @cpojer, I don't understand what the fuss here is about the above is possible already – replace a single command in the package.json and you can use your own test runner!

@ForbesLindesay
Copy link
Contributor

The problem (in as much as there is a problem) is that you cannot easily access the babel/webpack config from another test runner. Any test runner would need to be specialised to react-scripts/create-react-app and know to look up config by doing require('react-scripts/config') which would be super brittle.

I think the solution to this would be something akin to the "configurator" field suggested by @gaearon in #215. With something like this in place, we could easily make other test runners compatible (and make it easy to share code with node.js apps or use eslint plugins in your IDE etc.)

@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

I think that with time, as things shake out, we might start exporting readonly Babel config for interested tools. Not now though.

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

I agree with making testing a dependency not a built-in. You don't need testing to start creating an app, but it is something that you eventually want to add in. It would be nice that when you get ready to add it, that you could add the suite of preference (like mocha, karma-mocha, jest, ava, ...).

Having the ability to use your favorite test environment is a huge win. If something is built-in and you have to eject to change it that's a terrible experience, plus you now have to cleanup all the unwanted dependencies that it may have taken 10 minutes to download and compile.

Each test environment community could support keeping their dependency up to date, that wouldn't be a burden put on create-react-app. You simply need to define how the dependency/plugin needs to work and those who want to use it can build it and maintain it.

PS. If you still decide to make jest a built-in, please allow for a way to opt-out.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

@jeffbski

Have you had a chance to read through my earlier replies? I don’t really see what the issue is, and unfortunately @ryanflorence hasn’t had time to clarify his concerns yet.

You can use another test runner. There won’t be any code that checks your dependencies and punishes you for using Mocha 😄 . I’m just saying we intend to ship Jest by default, and then it’s up to you if you want to use something else—just replace test command with what you want and fully configure it yourself. You can even extract that to a package and maintain compatibility yourself.

Is there something I’m missing that extracting Jest integration to a separate package would do for your use case? As I said above I don’t see the difference except for weird package layout where some things are in one dependency, and other things are in another dependency. If you ask us to maintain alternative integrations, sorry, we won’t do it—it’s more work than we could put on this project. But I don’t understand how splitting Jest integration into another package can possibly affect you (either positively or negatively) if you want to use Mocha. npm test is yours, you can change your scripts to do whatever.

As @ForbesLindesay noted, you would have to replicate our Babel config (or rely on an internal module that might change), but this would be a problem in any other case except if we start exposing it via an official API. Is this what this issue is about? I’m not sure because it seems focused on us shipping Jest integration via a separate package, rather than us exposing a Babel config.

What is it that you would like to see, exactly?

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

[...] that wouldn't be a burden put on create-react-app. You simply need to define how the dependency/plugin needs to work [...]

Implementing a dependency/plugin system is a burden we are not comfortable taking right now. This is exactly what Webpack, Babel, ESLint, and all those other tools do. This is where configuration, incompatibilities, and painful upgrade paths come from. Plugin systems are hard.

In an ideal world we would love to have a plugin system, but we just aren’t there with the default experience yet. Plugin system makes it hard for us to freely change internals, add features without worrying about breaking some third party setup, swap tools for better ones, et cetera. We may do this in a year, but definitely not in the first month.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Each test environment community could support keeping their dependency up to date, that wouldn't be a burden put on create-react-app.

As I mentioned earlier in this thread, there is nothing preventing this from happening right now except that the community would have to either rely on internal modules (which means potential breakage), or copy-paste our configs (and try to keep them up to date).

This is not ideal but given that we are not ready to expose a plugin system, reasonable doable if anyone intends to do it. And we can expose a read-only Babel configuration if this is everything people need. However this should be discussed in a separate issue focused on just that (exposing Babel config), with specific proposals, and specific people interested in implementing Mocha (and other) integrations. I would be glad to discuss this—but I’m not sure how separating jest integration into a separate package has any relation to that goal.

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

@gaearon yes, I had read through the replies and tried to follow though I'm not as familiar with the internals of how create-react-app is constructed so I'm basically just voicing my concerns so the team can make good choices.

Assuming that it is pretty easy to add our own testing then let's just have a way to opt out of installing jest. Installing 66MB that I don't want or use and waiting for compiling/building is painful. So please, please have a way to opt out of including jest when creating an app.

create-react-app needs to be quick to get up and running. It needs to work well in training or class environments where lots of people will be trying to use it at the same time, so adding an additional 66MB for everyone even if they don't need testing is a big burden on the network and all of their machines. They could be running this on older machines that take a while to compile.

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

I guess in some of our minds, if it is in a separate package then it could be excluded easily or replaced for another package.

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

Assuming we can't have it as a separate package due to technical difficulties, maybe a better approach than providing a way to opt out is to make this something that you opt into after creating your app.

So create-react-app would work as it does today not installing a test environment. Then you run another command to install jest and hook it in. Seems like it would install some packages and then setup the package.json script for test.

This keeps the initial install smaller and you only need to pull in the test stuff if you want it.

Also once this initial version of this test install for jest has been created, it becomes the pattern for how you would build alternative ones for karma, ava, etc.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

create-react-app needs to be quick to get up and running. It needs to work well in training or class environments where lots of people will be trying to use it at the same time, so adding an additional 66MB for everyone even if they don't need testing is a big burden on the network and all of their machines.

Where does this number come from?
I compared tarballs before and after adding Jest, and the size increased from 9.7 MB to 14.1 MB.

What is your testing methodology?

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

I created an empty project and did npm install -D jest. Then I tar'd the project to see what size the project was and it came out to 66MB

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Interesting. I just ran npm install [email protected], and measured the “received bytes”:

screen shot 2016-08-05 at 18 01 55

This is consistent with what I see if I run ./node_modules/.bin/bundle-deps (we do this as release step) and then do npm run create-react-app ../whatever which runs npm pack as part of its script:

screen shot 2016-08-05 at 18 03 24

screen shot 2016-08-05 at 18 03 29

Perhaps it’s bundledDependencies that makes the difference, I’m not sure.

@jeffbski
Copy link

jeffbski commented Aug 5, 2016

I was quoting size on disk 66MB, the gzipped size is 12MB. You are probably right about bundled deps though too.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

I understand that increasing the download size is frustrating.
However I’m not sure the difference between ~10 MB and ~15 MB download is significant enough to warrant:

  • Spending effort into splitting packages and automating additional configuration steps.
  • Making already created apps depend on an internet connection for activating some features. (We want to move in the exact opposite direction: Offline mode #373.)
  • Creating an impression that -jest is just one of the options when there is nobody who volunteered to create and support other options.

I am closing this because I don’t see how this is immediately actionable, and who is willing to lead such effort. I am happy to revisit this if:

  • Somebody has created a community-supported integration package for another test runner and has demonstrated the willingness to maintain it.
  • This person can give us specific feedback on what hooks they would like to see exposed from our side to make such integration easier to support, and is willing to prototype such support as a PR.
  • A significant count of our users tell us they are happier with that configuration than the one we ship by default. (Actually, in this case, we might just switch Jest to something else here rather than allow more choice.)

In the meantime, we will be improving Jest in response to our users’s feedback. If this doesn’t answer your concerns, perhaps these alternatives could work better for your use case.

@gaearon gaearon closed this as completed Aug 5, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

FYI, 0.3.0 is out with support for testing.
Read the usage guide and the migration instructions.

We hope you’ll like it.

💜

@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

No branches or pull requests

8 participants