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

Running global jest-cli should warn instead of error #1293

Closed
mmcgahan opened this issue Jul 14, 2016 · 4 comments · Fixed by #1294
Closed

Running global jest-cli should warn instead of error #1293

mmcgahan opened this issue Jul 14, 2016 · 4 comments · Fixed by #1294

Comments

@mmcgahan
Copy link
Contributor

mmcgahan commented Jul 14, 2016

The process.exit(1) call triggered when running a globally-installed version of Jest results in odd test results - even when all tests pass, the process returns a non-zero exit code, and it's not immediately clear why.

When jest is assigned to npm test (which is a pretty common convention), you get:

Please run `npm install` to use the version of Jest intended for this project.
...
163 tests passed (163 total in 30 test suites, run time 3.292s)
npm ERR! Test failed.  See above for more details.

There are no obvious details (the "Please run..." line doesn't look like an error message") and no failing tests, so debugging is awkward.

Recommendations

  1. I would prefer to remove the test for a package-local version of Jest from getJest entirely. If people want to use a non-local version of Jest without running npm install, I'm not sure why the package should care?
  2. If the test is useful enough to remain, it should use console.warn without affecting the exit code because the tests may still run without trouble, and the warning is sufficient indication that the output might not reflect exactly what the user intended.
  3. If returning a non-zero exit code is preferred, the test should throw a real Error to make debugging easier - Node's built-in error handling is much more explicit than console.error followed by process.exit(1).
  4. If you really don't want to throw an error, call process.exit(1) immediately rather than waiting for all the tests to run - if it's not okay to run Jest globally when there's a missing locally-defined dependency, then it shouldn't run at all.
@cpojer
Copy link
Member

cpojer commented Jul 14, 2016

Thank you for bringing this up and for the detailed description of the issue. To be honest, I have been upset about this stuff myself and it definitely makes Jest's test suite a little harder to deal with.

I also agree the way this is working currently does not make any sense if Jest still ends up running tests anyway. I'm sorry you wasted time on this.

Would you be willing to send a pull request?

My suggestion is as follows:

  • Remove process.exit(1).
  • Do process.on('exit', () => console.log(chalk.red('The error message '))) so that we get a nice, red message at the end of a test run telling you that something is not quite as it should.

It will allow people to use the wrong version of Jest (which isn't recommended, because there might be breaking changes) but it will still finish the test run properly and will print a message at the bottom. What do you think?

@mmcgahan
Copy link
Contributor Author

@cpojer That works for me, thanks for the quick response! Will submit the PR shortly

@jedrichards
Copy link

@mmcgahan @cpojer I'm seeing this non-fatal error when running Jest inside a child package within a yarn workspaces monorepo. Feels like maybe it's being shown in error because the way you look for jest isn't dealing with packages being hoisted up to the monorepo root?

@github-actions
Copy link

This issue 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants