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 suggestion of using --passWithNoTests on no tests found #5127

Merged

Conversation

@emilgoldsmith
Copy link
Contributor

commented Dec 19, 2017

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:

screenshot from 2017-12-18 20-55-25

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

@emilgoldsmith emilgoldsmith force-pushed the emilgoldsmith:enhancement/passWithNoTests-suggestion branch from 3dcf491 to 5f31553 Dec 19, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Dec 19, 2017

Codecov Report

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

Impacted file tree graph

@@          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
Copy link
Collaborator

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2017

`${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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

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

@emilgoldsmith

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2017

Yes

@emilgoldsmith

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2017

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 emilgoldsmith force-pushed the emilgoldsmith:enhancement/passWithNoTests-suggestion branch from 5f31553 to ab94aa4 Jan 2, 2018

@emilgoldsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2018

@emilgoldsmith Sorry about the slow response!

I'd update the assertion in this test:

test('fails the test suite if no files are found', () => {
const result = runJest(DIR, ['--testPathPattern', '/non/existing/path/']);
const status = result.status;
const stdout = result.stdout.toString();
expect(stdout).toMatch('No tests found');
expect(status).toBe(1);
});

@emilgoldsmith emilgoldsmith force-pushed the emilgoldsmith:enhancement/passWithNoTests-suggestion branch from ab94aa4 to 1be1adf Jan 12, 2018

@emilgoldsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2018

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 :)

@@ -51,6 +51,9 @@

* `[jest-config]` Add `forceCoverageMatch` to allow collecting coverage from
ignored files. ([#5081](https://github.com/facebook/jest/pull/5081))
* `[jest-cli]` Make Jest exit without an error when no tests are found in

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 12, 2018

Collaborator

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

You probably need a rebase

const path = require('path');
const runJest = require('../runJest');

const DIR = path.resolve(__dirname, '../pass_with_no_tests-test');

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 12, 2018

Collaborator

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

@emilgoldsmith emilgoldsmith force-pushed the emilgoldsmith:enhancement/passWithNoTests-suggestion branch from 1be1adf to 6ce7982 Jan 12, 2018

Make [jest-cli] not error when no tests are found with --findRelatedT…
…ests, --lastCommit or --onlyChanged options

@emilgoldsmith emilgoldsmith force-pushed the emilgoldsmith:enhancement/passWithNoTests-suggestion branch from 6ce7982 to dfb0e33 Jan 12, 2018

@emilgoldsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2018

Rebased and comments addressed

@SimenB

SimenB approved these changes Jan 12, 2018

Copy link
Collaborator

left a comment

Not sure what's up with ci...

@cpojer cpojer merged commit c799a02 into facebook:master Jan 12, 2018

2 of 6 checks passed

ci/circleci: test-and-deploy-website Your tests failed on CircleCI
Details
ci/circleci: test-node-6 Your tests failed on CircleCI
Details
ci/circleci: test-node-8 Your tests failed on CircleCI
Details
ci/circleci: test-node-9 Your tests failed on CircleCI
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@emilgoldsmith emilgoldsmith deleted the emilgoldsmith:enhancement/passWithNoTests-suggestion branch Jan 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.