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

Don't force "--watch" into Jest #784

Closed
alexewerlof opened this issue Sep 27, 2016 · 13 comments
Closed

Don't force "--watch" into Jest #784

alexewerlof opened this issue Sep 27, 2016 · 13 comments

Comments

@alexewerlof
Copy link

If you are reporting a bug, please fill in below. Otherwise feel to remove this template entirely.

Description

I want the tests to terminate after running once. But there doesn't seem to be any way to disable this behavior except setting the CI environment variable like: "test": "CD=true react-scripts test --env=jsdom", in package.json. I traced the source of this behavior to these lines in test.js

// Watch unless on CI
if (!process.env.CI) {
  argv.push('--watch');
}

I understand that watching can be good when in the development cycle but when I want to run the tests in a pipeline or some script or git push hooks, I want to escape this mechanism.

Expected behavior

One of these

  1. Make the npm test run only once and then make another one called npm run test:watch that watches.
  2. Maybe add another package.json script called npm run test:once

Actual behavior

Tell us what actually happens.

Environment

react-scripts@0.4.1
node@6.5.0
npm@3.10.3

I can make a PR if this behavior is confirmed.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2016

when I want to run the tests in a pipeline or some script or git push hooks, I want to escape this mechanism.

Is there a reason you don’t want to set CI=true when running them in a script or git hooks? This is the intended usage. I understand it’s slightly inconvenient, but we need to teach people to use the watcher (and if it’s not good, nudge them to report problems so it gets better). So it seems to me that asking you to specify CI=true in scripts and hooks is a fair tradeoff.

@alexewerlof
Copy link
Author

alexewerlof commented Sep 27, 2016

Is there a reason you don’t want to set CI=true when running them in a script or git hooks?

Yes, it feels like a hack to set CI=true for a git hook that has nothing to do with CI.

This is the intended usage.

IF we have to use environment variables for this, maybe we can go for a more intuitive name like "WATCH" or "TEST_WATCH" or "WATCH_TESTS".

we need to teach people to use the watcher (and if it’s not good, nudge them to report problems so it gets better)

I have watched your talk about how Redux and hot reloading can improve developer productivity by shortening the code-run-debug loop and I totally understand why it's a good idea to keep the test runner watch the files. But the test command has a bit of an odd behaviour if you've worked with anything like Mocha or even Jest. The test runners by default run the test once and if someone wants the watch behavior, s/he opts-in. I feel that CRA is targeted at newbies and tries to help them by using best practices, but the CI=true solution seems a bit hacky and hard to grok.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2016

I understand your concerns and CI is indeed not the most obvious way to do it. It is documented, however, and I think it’s a reasonable compromise between having things work automatically, a good default setup, and a possibility of extensions. For now, Create React App prioritizes interactivity and heuristics over explicit configuration. This means that some advanced scenarios get a little bit more awkward but since you’re using them in code, you might as well put a comment there. In the future we might reconsider this (and even add some configuration). But today is not that day yet.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2016

Going to close this for reasons above. Happy to revisit in a few months if you’d like.
Sorry for the inconvenience!

@alexewerlof
Copy link
Author

Sorry for being so persistent.

I made a PR to demonstrate one possible solution.

Another (better IMHO) solution is not to do anything based on the environment variables and let the "test" script in package.json decide that. Travis runs npm test by default. So we can have something like test:dev or test:watch that can be used in the development cycle.

Another solution is to keep the --watch in the "test" script. This way if someone doesn't like it, they can easily remove --watch from their package.json rather than having to set an env variable. Everybody wins: your code will be simpler, newbie users won't notice a change and pro devs will have an option to opt out.

I'll be happy to help with another PR whatever makes more sense (and update the docs).

@gaearon
Copy link
Contributor

gaearon commented Sep 28, 2016

From your PR:

  • CI is already being handled
  • COVERAGE doesn’t really seem like a good fit for me because you can already run npm test -- --coverage as explained in User Guide, and it works in watch mode just as fine.
  • I explained why I don’t want to introduce NO_TEST_WATCH or similar in the comment above in this thread. As I also noted, I’m happy to revisit this at some point, but not now.

I understand your arguments well. I have considered each of these options (including leaving --watch in package.json) when I was integrating the test watcher. I made a choice to default to --watch unless CI is present, and I intend to stick to that choice for a while now. I appreciate your feedback and I’m happy to discuss this again in several months, but I don’t intend to change it now.

@heracek
Copy link

heracek commented May 28, 2018

Simple hack is to prevent --watch is this:

react-scripts test --watchAll=false

This works because the code detects presence of --watchAll:

if (
  !process.env.CI &&
  argv.indexOf('--coverage') === -1 &&
  argv.indexOf('--watchAll') === -1
) {
  argv.push('--watch');
}

@tshinnic
Copy link

tshinnic commented Jun 5, 2018

Having a problem making your wonderful workaround work nicely as CRA 1.1.4 is insisting on using a previous copy 20.0.4 of Jest, which doesn't (?) support --watchAll . I nastily installed jest@23 underneath the existing node_modules\react-scripts and now your workaround makes me happy.

@rudasn
Copy link

rudasn commented Jun 27, 2018

Just a heads up, you can also use --coverage to disable watch mode.

npm run test -- --coverage 

@slorber
Copy link

slorber commented Aug 22, 2018

Hi @gaearon

To update snapshots without the --watch, I can do `"test:updateSnapshots": "CI=true npm run test -- --updateSnapshot",``

The CI=true feels a bit weird here as I run this command locally, totally unrelated to CI.

I'll be fine with it, just reporting the issue

@mrmckeb
Copy link
Contributor

mrmckeb commented Nov 6, 2018

@gaearon A problem here is that CI=true disables color in tests, which is not ideal. I'd like to see another solution here, but for now I'm using @heracek's solution.

AndreMiras added a commit to AndreMiras/etheroll that referenced this issue Nov 19, 2018
Another option was to use `CI=true`, refs:
facebook/create-react-app#784
@ScottMcCormack
Copy link

Just a heads up, you can also use --coverage to disable watch mode.

npm run test -- --coverage 

I'm a bit confused by the syntax -- --coverage

Why doesn't this operate the same as --coverage?

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2018

@ScottMcCormack, that tells npm to pass the flag through, otherwise it would be considered an npm flag. Note that this is not required in yarn.

@lock lock bot locked and limited conversation to collaborators Jan 8, 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