safe --json && --listTests

[aaronabramov]

we've had way too many bugs caused by accidentally console.logging something together with JSON output, which, of course, would make this outputted JSON invalid.

This PR solves the problem by completely replacing process.stdout with process.stderr if we're in one of the "no random stdout" modes (--json, --listTests, --useStderr).

I had to hijack process.stdout, before we require a single module, because otherwise any console.log in inside node_modules, third party or plugin/reporter code can mess things up.

cc @mjesun

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@f7b25b6). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master    #4212   +/-   ##
=========================================
  Coverage          ?   60.19%           
=========================================
  Files             ?      191           
  Lines             ?     6768           
  Branches          ?        6           
=========================================
  Hits              ?     4074           
  Misses            ?     2691           
  Partials          ?        3

Impacted Files	Coverage Δ
packages/jest-config/src/defaults.js	85.71% <ø> (ø)
...ackages/jest-cli/src/reporters/default_reporter.js	91.04% <ø> (ø)
packages/jest-cli/src/run_jest.js	0% <0%> (ø)
packages/jest-cli/src/watch.js	73.59% <66.66%> (ø)

---

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 f7b25b6...5e1bdeb. Read the comment docs.

[cpojer]

I don't feel like this is the right way to go and it is using a huge bandaid to solve a real problem. I'm willing to bet the reason why it was impossible to locate where the stdout data came from is because that was inside a test file or other user code writing to stdout directly. How about in the environment where we clone the process object, we swap out stdout with stderr when the json flags are given, so that test code can only print to stderr but Jest itself can retain the ability to print to either?

[mjesun]

I have mixed feelings with this PR: on one side, I agree with @cpojer that this does not look the best way to do it, and that we should really investigate the root cause of why stdout gets polluted.

On the other side, it's also true that jest package is not a standalone package, and it requires from many other packages, which in turn are not tied to an exact version. If a non-breaking change is added to any of them (i.e. outputting to process.stdout.write) it can ruin the whole thing.

While Jest takes care of buffering console.log and process.stdout within a test to avoid polluting the terminal, we do nothing outside them, so in the end I don't think this is a bad idea; it just needs a different approach.

On the other (unrelated) side, I didn't know that --listTests without --json was also printing JSON. I would strongly suggest --listTests to return separated lines when not using the --json flag so it follows a "console" approach (e.g. I can grep, awk, for .. in with the output). I opened #4214 for that.

[cpojer]

@mjesun note that Jest doesn't wrap process.stdout within a test, which is what I suggest we should do. The wrapping you linked to wraps output during a test run, but will print the data at the end.

[aaronabramov]

I don't feel like this is the right way

why do you think it's a bad approach?
my main concern is actually about Jest code itself and not third-party/user code (the problem i'm trying to track right now is using --listTests, which does not run any tests or setup code). I feel that the approach we have today is the wrong way to solve this problem because:

1. We pass outputStream or pipe all over the code
2. The only way to enforce the right usage of console/process.stdout is code reviews (and it doesn't scale)
3. It didn't work in the past and breaks all the time

So even though in this PR we modify the global object, i strongly feel that it is a much better way to solve this problem than playing a whack-a-mole with with console/process.stdout calls throughout the codebase without the way to automate it. (and that already cost us a major breakage)

[cpojer]

I would like to understand first the reasons why the current approach is breaking down concretely. As said, I think the biggest problem is test code and we can patch process.stdout for sandboxes.

1. I agree third-party code printing to stdout is bad, and we should avoid using such libraries. Which libraries that are currently part of Jest do this?
2. We can lint against use of process.stdout to protect against introducing it.
3. I don't think it's true that it breaks all the time.

All I'm saying is that this is the nuclear option that imho goes too far in one direction when we don't fully understand the source of the current problem and there are simpler fixes that we could try out first and changes to our process (hah!) that can mitigate this problem in the future.

[mjesun]

@aaronabramov Based on @cpojer's comment I'd strongly agree into looking at what is corrupting stdout. If all the test run output is stored in a buffer, then released to either stdout or stderr depending on the flag, there is clearly something we are doing incorrectly, since we already use --useStderr yet we get a polluted stdout.

[aaronabramov]

closing for now. we fixed it locally and will come up with a better strategy to fix it in the future

[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.