Jest: The cli doesn't exit correctly when stdout is a non-tty

[szeller]

Do you want to request a _feature_ or report a _bug_?

Bug

What is the current behavior?

If you are redirecting stdout to a file (or have stdout not connected to a tty in a similar way), then the jest-cli process will exit with 0 even if there are failing tests. If you execute jest from the cli without any kind of redirect in place, then you get a non-zero exit value for failing tests as expected.

It appears that this was introduced with the exit changes in this PR https://github.com/facebook/jest/pull/5313

What is the expected behavior?

A non-zero exit value in the case of test failures.

Please provide your exact Jest configuration and mention your Jest, node,
yarn/npm version and operating system.

• node v6.11.4
• npm v3.10.10
• jest 22.1.1
• ubuntu 14.04

NOTE I updated the description since the original bug report with my latest findings

[thymikee]

I assume you mean versions 22.1.0 and 22.1.1. cc @mjesun

[mjesun]

I've been able to repro; I'm checking it.

[szeller]

@thymikee you are 100% correct 😉 I updated the original comment

[SimenB]

@szeller can you test with 22.1.2?

[szeller]

@SimenB it's still broken in that version for me

[Sebruck]

I can confirm that I have the same problem

[SimenB]

Can you create a repro?

[mjesun]

I've been able to repro it internally with a parent project that excludes files of a child project. I'm fixing it by re-running the testPattern function on every file, even if the file exists, and even if it is on the root of the Haste instance monitoring the project root folder.

I should have something soon as a PR.

[szeller]

@mjesun release 22.1.3 doesn't fix this one for me

[mjesun]

Then we're going to need a minimal repro test case; I can't see any other issues with the internal configs 😔

[szeller]

Okay. I'll look at trying to create a minimal test case for it.

[szeller]

I made a test case repo for this one https://github.com/szeller/jest_failure_bug

In order to reproduce, you need to use AWS codebuild using the default node 6 build. Since there's a failure in one of the tests, the build should fail (and does fail on jest 22.1.0) but won't fail on jest latest

[mjesun]

Thanks @szeller! I will try looking at it through the day. This should definitely make debugging easier.

What is the test that should fail?

[mjesun]

NVM, i fully checked the project and saw there's only one 😄

[mjesun]

The test fails for me on Node 6, Jest 22.1.4 (which we released few minutes ago, but does not include any specific fix regarding test discovery):

I wonder if it's then something specific to AWS CodeBuild. Is the test failing for you as well when you execute it locally?

[szeller]

@mjesun like I said in the original description, everything works fine on my local machine. On both codebuild and locally, the test output is more or less the same. However, on codebuild, the npm test has an exit code of 0, which causes codebuild to believe that it has succeeded.

My guess is that it's not so much codebuild specific as it is specific to some part of the codebuild env: docker, using sh vs bash, non-tty, returning true from https://github.com/watson/is-ci, etc

[szeller]

@mjesun

I did some more testing and was able to reproduce the issue locally. It looks like the problem is that the exit value is always 0 if stdout is not a tty. e.g. if you do npm test > foo, the exit value from jest is wrong. I updated the package.json in the repo to define test as jest && echo exit is broken. The echo doesn't happen if you just run npm test but the echo will get written to foo if you run npm test > foo.

[szeller]

Hopefully the latest edits to the title and description clarify this one. It feels like this is pretty major regression and I'm surprised that more people haven't reported it. That said, I tried running the tests via jenkins and the build failed as expected so there's definitely some scenarios where it works fine.

[lsjroberts]

I can confirm we are experiencing the same issue, all great locally but on Jenkins a failed test run exits with 0. Pinning both jest and jest-cli to 22.1.0 fixes it.

[casba]

It looks like it's due to the exit dependency attempting to write to stdout/stderr in combination with exit being called in a process.on('exit') handler. The exit module will place the call to process.exit(exitCode) on the event loop which will never run if the exit function was called via the 'exit' event.

This should only happen on asynchronous stdout/stderr streams.

[dcarrot2]

@lsjroberts For us, 22.1.0 didn't fix it and we rolled back to 21.2.0

[lsjroberts]

@dcarrot2 Did you do both jest and jest-cli? As [email protected] requires jest-cli@^22.1.0 so picks up the later version with the new exit dependency, which seems to be the culprit.

[szeller]

@mjesun any ETA on a fix? This one feels fairly critical since it's allowing build systems to ignore test failures.

[tgrant59]

Experiencing this on Jenkins as well. Even with jest and jest-cli pinned to most recent versions.
@mjesun +1 on this being a high-priority bug, it means we can't upgrade to Jest v22

[cpojer]

Unfortunately we already rely on this at FB actually. Would a possible fix be to set process.exitCode in all places?

[lsjroberts]

@cpojer Yeah I believe that would fix it, thanks.

I was under the impression using process.exitCode was best practice (https://stackoverflow.com/a/37592669), and using it would mean not needing the exit() call?

I'm sure there was a reason process.exit() was instead chosen in the first place, so would it introduce any other issues if exit() was swapped out for process.exitCode and perhaps a return here and there?

[markFromMST]

I see the problem even at the command line locally, 21.2.1 exits properly:

Test Suites: 1 failed, 2 passed, 3 total
Tests:       1 failed, 9 passed, 10 total
Snapshots:   0 total
Time:        2.292s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

Process finished with exit code 1

22.0.0 is when it started failing to exit properly (and it doesn't even seem to catch its own errors, I can't show the full error, but here is what I can show):

      at Promise._settlePromiseFromHandler (node_modules/bluebird/js/release/promise.js:512:31)
      at Promise._settlePromise (node_modules/bluebird/js/release/promise.js:569:18)
      at Promise._settlePromise0 (node_modules/bluebird/js/release/promise.js:614:10)
      at Promise._settlePromises (node_modules/bluebird/js/release/promise.js:689:18)
      at Async.Object.<anonymous>.Async._drainQueue (node_modules/bluebird/js/release/async.js:133:16)
      at Async.Object.<anonymous>.Async._drainQueues (node_modules/bluebird/js/release/async.js:143:10)
      at Immediate.Async.drainQueues [as _onImmediate] (node_modules/bluebird/js/release/async.js:17:14)

PASS  test/mc-persistence.test.js

Test Suites: 3 passed, 3 total
Tests:       10 passed, 10 total
Snapshots:   0 total
Time:        3.555s
Ran all test suites.

Process finished with exit code 0

[szeller]

@markFromMST I think that's potentially a different problem. For the one that I'm encountering, the failure count is always correct and the exit value is the only thing that is wrong

[mjesun]

Removing all process.exit is not feasible. It's correct to think that process.exitCode + just returning is the best approach; but dealing with user code can introduce references that are not properly freed (think about tested code using setInterval without a clearInterval that un-references the timer).

The only change done lately to exit codes is in #5313, but the change was pretty straightforward. Plus, the exit module already forces the exit code when calling process.exit. The only possibility that comes to my mind is that, now that the exit is delayed until std streams are flushed, a second call to process.exit happens, overriding the initial exit code.

It's quite interesting that this only happens when the output is not a TTY, essentially because that's the default for integration tests. I will try investigating it tomorrow, sorry for the inconvenience.

[szeller]

@mjesun I agree that PR #5313 was fairly straightforward but that's the commit that caused the problem that I'm referencing 😉 You can test that my test repo works fine with the release previous to that change and fails using the version with that PR in it. It looks like there are some odd issues on some platforms with exit that were never fixed. It sounds like something similar to the bug that we're seeing here. https://github.com/cowboy/node-exit/issues/7

[mjesun]

Thanks @szeller for the repro repository, it definitely helped tracking down the problem 🙂

I manually patched my node_modules folder with the proposed change in #5445 and looks like the message (exit is broken) is not shown anymore when redirecting to a file.

[Pasupathi-Rajamanickam]

What is the package version for jest and jest-cli to fix it. For me, all test pass case not exiting. :/

[SimenB]

The fix is available in 22.2.0 and newer

[johnnykoo84]

I'm using jest 24.9.0 and seeing the same issue, i'm not seeing exit code 1 with failure (i'm using AWS codebuild for CI) Anyone has the same issue anymore?

[Fantaztig]

Same issue with jenkins and jest 24.9.0