Add suggestion of using --passWithNoTests on no tests found

[emilgoldsmith]

Summary

I just upgraded my version of Jest and encountered the breaking change regarding No tests found returning a non-zero error code, and after researching and figuring out the cause I thought that the message I added now would've been helpful, so I thought I'd suggest adding it.
Test plan

Should'nt be much testing necessary here, but this is how it looks now:

Also a quick question regarding the CHANGELOG: Would this be a fix, feature or a chore?

[codecov-io]

Codecov Report

Merging #5127 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5127   +/-   ##
======================================
  Coverage    60.8%   60.8%           
======================================
  Files         201     201           
  Lines        6707    6707           
  Branches        3       3           
======================================
  Hits         4078    4078           
  Misses       2628    2628           
  Partials        1       1

Impacted Files	Coverage Δ
packages/jest-cli/src/run_jest.js	52.54% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8549900...ab94aa4. Read the comment docs.

[SimenB]

Thanks for the PR!

I think this warning only makes sense if findRelatedTests is passed. A full test run not finding any tests should just be considered an error.

It's also a bit verbose, even though I don't have any better suggestion off the top of my head.

A test (integration test is fine) for this would also be needed.

And an update to the changelog

[emilgoldsmith]

It's also a bit verbose, even though I don't have any better suggestion off the top of my head.

I guess we could maybe do something more along the lines of NOTE: To make no tests found not constitute a failed test (non-zero exit code) use the '--passWithNoTests' option? Any other suggestions for wording also very welcome :)

And an update to the changelog

Yep, as I asked in the original PR message I was just a bit in doubt about whether this counted as a fix, feature, or a chore. But now that we're adding logic regarding other options etc. I guess I'm starting to lean towards feature?

I think this warning only makes sense if findRelatedTests is passed. A full test run not finding any tests should just be considered an error.

That makes sense, as I guess any "constant" (non-dynamic) command should always find at least one tests or it should be removed from whatever script is being run. I was just looking through CLI options to see if there were any other dynamic CLI options, and I found --lastCommit and --onlyChanged. I could produce same behaviour as with --findRelatedTests with --onlyChanged but with --lastCommit it always ran the test in a dummy repo I setup even though my last commit was just creating a new file (is that a bug by the way? Seems like it). So should we also include these options? And are there any other dynamic options I missed?
I guess an argument could also be made if we make the message a bit less verbose like above to just always put it there and people can just always have the choice, but I see the argument for why in a constant setting it really doesn't make sense. Up to you!

[SimenB]

`${noTestsFoundMessage}\n\nIf you consider no tests matching your filters correct behavior, use the '--passWithNoTests' option`

Something like that?

I guess I'm starting to lean towards feature?

Agreed

(is that a bug by the way? Seems like it)

Sounds like it. --onlyChanged for working tree should have the same behavior as --lastCommit for HEAD. A test can be that --lastCommit should behave the exact same as --onlyChanged after doing git reset HEAD~1

@cpojer thoughts here?

[emilgoldsmith]

One could of course also consider just making it not error when using one of the dynamic options such as --findRelatedTests or does that seem like too confusing behaviour?

[SimenB]

That might make sense. We've turned it off for watch mode already, extending that to onlyChanged, lastCommit and findRelatedTests might make sense - finding no tests in those cases is perfectly valid.

[emilgoldsmith]

Yeah I'd personally find that behavior nice and expected, especially for my use case of precommit hooks.

So shall I implement it like that? Just making it exit with exit code 0 in the above mentioned "dynamic" cases?

[SimenB]

@cpojer (and @EnoahNetzach) what do you think? I'm leaning towards what I said in my last post, but I'd love to hear your thoughts on it

[EnoahNetzach]

Off the top of my head I could find usages where you want to use --lastCommit, --findRelatedTests or --onlyChanged and still fail if no tests are found.
Thinking about a CI where you want tests to be added if certain conditions are met, in which case you wouldn't have a way to enforce the "zero tests error" rule.

The --passWithNoTests is always a viable option in any other cases, so I'm leaning towards a message like #5127 (comment), but no automatic switching that option on with those dynamic options.

[cpojer]

I agree, let’s just not make it exit with 1 when one of those options are passed.

[emilgoldsmith]

Okay so that's a 2 - 1 vote for changing exit code as opposed to just a warning message from the contributors. Is that final?

[SimenB]

Yes

[emilgoldsmith]

Okay, I'll get around to implementing it in the near future then. Will enjoy celebrating Christmas with the family first though! Happy holidays to all that celebrate something :)

[emilgoldsmith]

Okay, back to work after holidays :).

I pushed the changes and update to the CHANGELOG. The only thing I could think that one could do different is add a message in our special case that lets the user know that we aren't erroring, but I think in my opinion it's probably okay to leave that out as it is still obviously stated that there are no tests found, and as we discussed above it seems to be the intuitive behavior for it not to error.

This should definitely have some simple tests though, but I was looking at it a bit and it's just a big folder structure of tests and I got a bit lost so I thought I might ask for some suggestions where to put the tests? If there is a file / directory that I could just integrate it into or whether I should make a new one (and if so where)? I'm assuming there are already tests that check that it errors correctly in usual circumstances (though I just did a vimgrep /passWithNoTests/ ./**/__tests__/** to check and didn't find anything related to that though?), so I just want to do some (probably integration) tests where we run with these options and no tests and check that it doesn't error. I'm guessing I will need to either use some fixtures or mocks somehow and also wondered if there was something I could take inspiration from instead of trying to reinvent the wheel.

Thanks! And happy New Year!

[SimenB]

@emilgoldsmith Sorry about the slow response!

I'd update the assertion in this test: https://github.com/facebook/jest/blob/e5b9339e23ae5d3e9713c8210d365a005de823e9/integration_tests/__tests__/pass_with_no_tests.test.js#L18-L25

[emilgoldsmith]

Thanks for the heads up @SimenB, just what I needed!

I'm not sure what went wrong with the CI tbh, it looks like the problems happen in the restoring of cache? Everything runs fine on my computer.

As far as I'm concerned I think I'm done now, so it should be ready for a final review :)

[SimenB]

can you move this to be under master at the top?

You probably need a rebase

[SimenB]

you should rename this directory as well since you're renaming this file

[emilgoldsmith]

Rebased and comments addressed

[SimenB]

Not sure what's up with ci...

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.