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

Allow npm test to not run in watch mode for better cross-platform / tooling CI. #1137

Closed
cchamberlain opened this issue Dec 2, 2016 · 12 comments
Closed

Comments

@cchamberlain
Copy link

@cchamberlain cchamberlain commented Dec 2, 2016

I've been using create-react-app in conjunction with lerna repos and for the most part its been an awesome experience. The one issue I've been seeing is when trying to issue lerna run test commands to test all packages that have a "test" script. Since create-react-app defaults to entering watch mode on test, it hangs.

After some digging it looks like I might be able to set a CI environment variable to disable the watch mode, however this does not work well due to the nuances of setting environment variables cross platform.

Something as simple as changing the default from

    "test": "react-scripts test --env=jsdom",

to

    "test": "react-scripts test --env=jsdom --watch",

would allow me to simply turn it off. Conversely adding an optional --no-watch or --single-run flag would solve the problem.

I'm happy to submit a PR, but wanted to get some feedback first on the internal direction regarding whether this has been considered in the past, and the reasoning around the current implementation. I get that its a nice experience for the casual user, but at the cost of CI and incompatibility with lerna there should be some sort of escape hatch (short of eject).

@fson
Copy link
Contributor

@fson fson commented Dec 3, 2016

@cchamberlain Could you elaborate on why setting the CI environment variable is not an option for you? Would cross-env help here?

Loading

@cchamberlain
Copy link
Author

@cchamberlain cchamberlain commented Dec 6, 2016

@fson - Thanks, I'll test it out soon but think it should work.

Still feels against the norm to me for npm test to run tests in watch mode. Given how common it is for CI to run npm test, wouldn't it be simpler to have that just execute the tests and have users run npm test -- --watch to run jest in watch mode?

Loading

@troygoode
Copy link

@troygoode troygoode commented Jan 2, 2017

@cchamberlain I've just changed my package.json to:

"test": "CI=true react-scripts test --env=jsdom",

which works great

Loading

@cchamberlain
Copy link
Author

@cchamberlain cchamberlain commented Jan 3, 2017

@troygoode - Have you tried that on Windows?

Loading

@troygoode
Copy link

@troygoode troygoode commented Jan 3, 2017

@cchamberlain no, I haven't. works great in bash though

Loading

@cchamberlain
Copy link
Author

@cchamberlain cchamberlain commented Jan 3, 2017

@troygoode - The main problem here is that setting environment variables via scripts section is not cross-platform friendly and is not on a roadmap to ever be (last I read).

On Windows, you would need to use:
"test": "set CI=true &&react-scripts test --env=jsdom",

Further, npm test is commonly used in CI processes in the context of it being a single run without needing to set an environment variable. This is the only thing I've come across in create-react-app that rubs me the wrong way. As @fson pointed out, I could use cross-env to work around this but no workaround would be necessary if npm test ran tests once, and npm test -- --watch did the same thing I'd expect jest --watch does. This approach would also allow create-react-app to continue with its no dependencies approach since it's baked into react-scripts.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Feb 11, 2017

You can set them in cross-platform way:

npm i --save-dev cross-env

then use

  "test": "cross-env CI=true react-scripts test --env=jsdom"

Further, npm test is commonly used in CI processes in the context of it being a single run without needing to set an environment variable.

I think you might have misunderstood. In most CIs you don't need to set this variable because it is already set by CI environments. Travis, Circle, and probably others already do this.

I hope this helps!

Loading

@gaearon gaearon closed this Feb 11, 2017
@ryansully
Copy link
Contributor

@ryansully ryansully commented Feb 12, 2017

From https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/scripts/test.js#L29-L32

// Watch unless on CI or in coverage mode
if (!process.env.CI && argv.indexOf('--coverage') < 0) {
  argv.push('--watch');
}

From this, cross-env is one solution, but running npm test -- --coverage also only runs tests once, and avoids the need for another dependency (unless you already have need for it for other reasons).

For my projects I've created an additional NPM script:

"test:coverage": "npm test -- --coverage"

and just include that in other script definitions as needed when I want to run tests only once.

Loading

@cchamberlain
Copy link
Author

@cchamberlain cchamberlain commented Feb 13, 2017

Thanks @gaearon and @ryansully. Both of those should cover my needs.

Loading

@quantuminformation
Copy link

@quantuminformation quantuminformation commented Apr 22, 2017

Did anyone get this to work with pre-commit? I'm getting passing tests but the exit code stops the commit.
I get:

We've failed to pass the specified git pre-commit hooks as the `npm`
			pre-commit: hook returned an exit code (1)

image

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Apr 22, 2017

If you can create a standalone reproducing case somebody might be able to take a look!
(Please file a new issue for this.)

Loading

@antonovicha
Copy link

@antonovicha antonovicha commented Jul 20, 2018

and probably others already do this.

Jenkins does not set CI env variable.

Also setting up debug launch configuration in VS Code for tests is extra effort. Actually I am not yet figure it out.

Loading

jeffmcaffer added a commit to clearlydefined/website that referenced this issue Nov 21, 2018
Seems like react-scripts test by default goes into a continuous mode watching the file system. It also appears that this causes problems both on Windows and the Linux CI build such that the test infrastructure does not find any tests until a file is changed. Using --coverage is a hack ([based on a comment in CRA](facebook/create-react-app#1137 (comment))) that react-scripts looks for and forces immediate test run and no watching.
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants