npm test fails if the project doesn't have git

[gaearon]

Apparently Jest did this. Oops!

Determining test suites to run...
--watch is not supported without git/hg, please use --watchAll 
npm ERR! Test failed.  See above for more details.

[gaearon]

@SimenB @rogeliog Is this intentional? We provide --watch by default — and we're fine to fall back to --watchAll. Do we really need to check for .git/.hg presence ourselves just to prevent Jest from failing later?

[gaearon]

To reproduce:

npx create-react-app@next --scripts-version=@next test-next
cd test-next
rm -rf .git
yarn
yarn test

[SimenB]

This is by design: jestjs/jest#4737 (comment)

Maybe it makes sense to fall back to --watchAll automatically? Imo, we shouldn't have watchAll, we should have --watch and --watch -onlyChanged. The latter is currently implied. I get that we want to encourage people to use onlyChanged as that's a better mode, but it leads to counter-intuitive behavior like this.

So the short of it is that, yes, you should probably detect missing VCS and in that case use watchAll in CRA.

Although I'd personally be open to printing something like Warning: No VCS detected, falling back to running all tests instead of exiting (unless the user explicitly passes --changedSince, --lastCommit or --onlyChanged), thus making --watch without VCS be equivalent to --watchAll automatically. You could pitch it to Chris and see what he thinks? 😁

[gaearon]

What I really want is something like --watchChangedIfPossibleOtherwiseWatchAll :-) With a better name for sure.

There's no need to shame users into adding a VCS in tiny projects. At this point "watch changed" would bring them zero benefit because they're basically just learning programming. It's just confusing to scare them with this message.

IMO there should be a seamless mode where it "tries its best" at watching changed, but if it can't, falls back to "watching all". We can implement it ourselves for sure but I'm worrying about diverging logic, and also about unnecessary bloat in ejected scripts, duplicating what Jest is already doing.

[SimenB]

Yeah, that was the pitch in jestjs/jest#4737, but it was changed to throw as it was deemed (I think) to be more confusing to change a user's config from under their feet than asking them to be explicit.

[SimenB]

/cc @aaronabramov

[gaearon]

Well, that's true when user specifies the config. But in our case we specify the config, and for us Jest config is an implementation detail that now leaks to the user.

[rogeliog]

I agree. From what I've been looking at, it seems to be that there was a regression introduced between jest@20.x which is what create-react-app@1.15 uses and jest@22.0.0. This was fixed by providing a better message for the exception, but still exiting with non-zero.

I'm sure @cpojer might provide better insights, but here are my initial thoughts.

1. Change the behavior in Jest to gracefully handle --watch in non-VCS environments. (Which is how it used to behave.) Probably by just falling back to --watchAll.
2. Patch CRA to allow --watch to behave as expected.
3. Release this change in the next Jest release.
4. Remove the patch in CRA and upgrade Jest

[gaearon]

This sounds great! For now I'm adding a detection workaround.

[SimenB]

Note that, unless somebody (probably meaning @mjesun) wants to do some cherry-picking, the next release of Jest will be a major, so any change we make to Jest will most likely not be something CRA can pick up before its next major. So a workaround in CRA makes sense regardless