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

Add a test runner and template Enzyme test. #216

Closed
wants to merge 1 commit into from

Conversation

wdhorton
Copy link
Contributor

I put this together to show how we can add a test runner to the project. Discussion of exactly which test runner to use is ongoing, so I don't want to focus too much on that part. The Mocha-specific part of this is relatively minor, and could be swapped for a different framework relatively easily.

The main part is setting up a test config for Webpack and creating an entry point to bundle all the tests. Note that the first argument to require.context has to be a constant string (see issue here), which is why I made three different entry point files to handle where the file is in relation to the app's src directory. Definitely open to a more straightforward solution if anyone has suggestions.

The test plan: right now I'm planning to add on to the existing e2e.sh test, since the other scripts use that.

I think getting testing that "just works" is an important part of this project, and I'm excited to see where we can go with this!

@ghost ghost added the CLA Signed label Jul 26, 2016
@wdhorton
Copy link
Contributor Author

Also important to note: in keeping with the project's purpose, it exposes zero config to the user prior to ejecting.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

What are the pros and cons of using Webpack as test builder vs just using Node + Babel?

@ForbesLindesay
Copy link
Contributor

node + babel won't work if you want to test a component that imports css or images etc. because you cannot import those in node by default.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

This depends on the runner, e.g. Jest has moduleNameMapper.
It’s not quite “node” though so I see your point.

@ForbesLindesay
Copy link
Contributor

Jest only has moduleNameMapper to solve a Facebook specific problem though; it's not the norm.

Personally I'm strongly opposed to testing frameworks that integrate the totally separate things of:

  1. running a bunch of JavaScript files in node/the browser
  2. structuring tests and assertions within a file

My feeling is that we should provide 1 as part of create-react-app and maybe make a recommendation for 2. This would rule out jest and mocha though.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Jest only has moduleNameMapper to solve a Facebook specific problem though; it's not the norm.

Importing CSS/images is not quite “the norm” either but we do it here (so we can have dependencies managed by a single tool), we do it in React Native, and many Webpack users in the community do it as well. So I think it’s fair to say at this point that if it’s not the norm, it’s at least a very common (and useful) deviation from the norm.

@ForbesLindesay
Copy link
Contributor

Importing css/images is (I think) rapidly becoming the norm. What I meant was that it's not the norm for test runners to support anything other than the node.js resolution algorithm. Jest does because we don't use the node.js resolution algorithm at Facebook. We still use the node.js resolution algorithm for CSS dependencies and files here. Using a different algorithm for resolving dependencies is exceptionally unusual.

@wdhorton
Copy link
Contributor Author

Personally I'm strongly opposed to testing frameworks that integrate the totally separate things of:
running a bunch of JavaScript files in node/the browser
structuring tests and assertions within a file

Not sure I understand, exactly. I agree we shouldn't dictate what assertions to use—I used chai's expect in the template test, but the user could use other types of chai assertions, or add a different library. But in order to be useful, a test runner should show you the outcomes of the tests; that would require it to understand the structure of what a test is, which is why you have syntax like describe and it. I don't really see how something that just ran arbitrary Javascript code in node or the browser could really be called a testing framework.

@just-boris
Copy link
Contributor

just-boris commented Jul 26, 2016

Usually I have the following line in my tests setup

require.extensions['.css'] = function () {};

This code makes node.js module system to ignore css imports and return empty module instead. As far as css-modules are not allowed (#98), that will be enough to deal with it.

@wdhorton
Copy link
Contributor Author

What are the pros and cons of using Webpack as test builder vs just using Node + Babel?

@gaearon To your original question, a major benefit of using Webpack is that, when the project adopts Webpack 2 and ES modules, the test setup won't have to continue to rely on transpiling to CommonJS.

@ghost ghost added the CLA Signed label Jul 27, 2016
@ForbesLindesay
Copy link
Contributor

I don't really see how something that just ran arbitrary Javascript code in node or the browser could really be called a testing framework.

If you think in terms of continuous integration servers (e.g. travic-ci) the only thing they ever understand from tests is the exit code. They show the stdout/stderr to the user for debugging, but all they use in any structured way is EXIT_CODE === 0 to tell if the test passed. In this way a node.js module that exits with the correct code and loggs debugging info to stdout provides all the information that's needed.

The describe/it structure doesn't need to be provided by the same module that runs the JavaScript file (consider testit for example, which I wrote).

The issue with having tests which are not just files to be run is that you can't choose a different runner. You can stack libraries for assertions, mocking, jsdom, structuring tests etc. as much as you like within a node.js module. Meaning you can pick whichever of those are useful to you. You can only have one test runner though. This makes it really hard to add code coverage to runners like mocha and jest (unless they add support directly). It makes it really hard to do arbitrary transpilation for babel/webpack etc. It means you can't run something like node-inspector to step through your tests.

By making sure that the runner's only job is finding the modules and running them (potentially in parallel) you can swap it out and use a different runner when it's convenient. Not being able to run tests with node-inspector has been a huge extra challenge when testing with mocha. Being defaulted into all the terrible auto-mocking stuff is the reason I don't use jest. We need to use more focused testing tools.

@ForbesLindesay
Copy link
Contributor

@just-boris The require.extensions approach is neat. It will break down completely if we adopt css-modules though, and you already need to make sure that all the image formats return a string.

@ghost ghost added the CLA Signed label Jul 27, 2016
@wdhorton
Copy link
Contributor Author

@ForbesLindesay I see your point, and you're right about being able to separate the running from the structure of the test.

Given that, though, I don't see how running the test with node versus using mocha gives the user more flexibility to swap in a new test runner (without ejecting). Assume we've gone the less-opinionated route—when they run npm test, it does nothing more than run all the test files with node. There's still no simple way they could make it so that npm test runs ava or karma instead. They could try to use a different npm script like npm test:ava, they could eject, or they could rewrite npm test to not use react-scripts at all. But those are all the options they would have had if we included mocha (or any other runner) by default anyway.

@ghost ghost added the CLA Signed label Jul 27, 2016
@lacker lacker mentioned this pull request Jul 27, 2016
@lacker
Copy link
Contributor

lacker commented Jul 27, 2016

Some random questions after implementing something similar for #245 -

Does this webpack up the node_modules? I think it does (I could be missing something) but I don't think you have to do that for testing.

Is the convention that NODE_ENV is "test" for testing? I thought the convention was that NODE_ENV was always either "development" or "production" or unset.

I think it is better to put these intermediate files in the global temporary directory - then it can get automatically cleaned up if your system is administrated that way.

@wdhorton
Copy link
Contributor Author

I believe that it does include node_modules in the bundle, which I think is necessary. They might be imported and used in some way that would affect the component's render output. Would the bundle be able to require them?

I'm really not sure about the NODE_ENV conventions. I'm not using it anywhere, so we can leave it unset.

I had no idea that os.tmpdir was a thing—definitely a better idea here.

@lacker lacker mentioned this pull request Jul 28, 2016
@lacker
Copy link
Contributor

lacker commented Jul 28, 2016

I think it will be pretty slow if you are recompiling all of your node_modules every time you run tests, and normally in a node environment you would expect the node_modules to be shipped in ES5 so they can be run without compiling.

@wdhorton
Copy link
Contributor Author

Still not sure I understand. This shouldn't include all of your node_modules, but it will include all the dependencies of your tests (which might include node_modules) in the bundle. Your node_modules won't get run through Babel (since, like you said, they're ES5 at this point), but if they're not in the bundle, how would the bundled test code know where to find them? Especially if the bundle is in your OS's tmpdir, how would it resolve the require statements to your project's node_modules?

@lacker
Copy link
Contributor

lacker commented Jul 28, 2016

You should be able to instruct it that requires go to node_modules somehow. On a different PR someone suggested https://www.npmjs.com/package/webpack-node-externals

@wdhorton
Copy link
Contributor Author

That looks promising, I'll check it out

@cpojer
Copy link
Contributor

cpojer commented Jul 29, 2016

I don't think it is scalable to bundle all app code for every test run. This kinda kills iteration speed on any slightly bigger project.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

Hey, thank you very much for the PR.
I’m inclined to go with #250 instead for these reasons: #250 (comment).
We might reevaluate this choice in the future (luckily porting Jasmine 2 tests to another runner is no big deal).

@gaearon gaearon closed this Jul 29, 2016
@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