Force redirecting stdout to stderr if useStderr is passed

[rubennorte]

Summary

Right now anything written to process.stdout in a test (or in a worker) goes to stdout even if useStderr is passed. This breaks JSON output if a test explicitly writes to stdout or if a worker runs out of memory (v8 prints directly to stdout :S), which we handle correctly making the test fail.

useStderr	method	real output
false	console.log	stdout
false	process.stdout	stdout
false	process.stderr	stderr
true	console.log	stderr
true	process.stdout	stdout => stderr
true	process.stderr	stderr

Test plan

Updated tests.

[SimenB]

Hoho, a bajillion failures 😬 Super excited about this though

Sorta related: #6583

[mjesun]

All errors look however related to coloring :/

[rubennorte]

Yeah, as @mjesun said, all errors but one are caused by colors. I have a version with that fixed but I want to try to get a closest fake copy of stdout and stderr.

[SimenB]

Cool, that should make #6524 simpler as well

[rubennorte]

Oh @SimenB, sorry I didn't see that one.

I can fix the errors in tests by simply making the pass-through stream that I define as stdout and stderr extending tty.WriteStream, but I think we need to be conscious here about the implications of doing that. Should we proxy the actual values of the actual stdout/stderr? Should we provide real values for the number of rows and columns in the terminal? What about the events? What about the methods with side-effects like clearScreenDown?

I think we should "copy" (actually using the real value via a getter) the information about the terminal (so we can use that to determine if we can use colors, for example) and define noops for the methods with side-effects.

Thoughts?

[mjesun]

IMO we should provide tty.ReadStream and tty.WriteStream instances, which are not piped anywhere, but to a single pass through socket. The output of this socket would then be sent to the parent process. This would simplify the overly complex setup of the console too, by just letting it rely on these globals.

For now, I think the suggestion @rubennorte made is good enough; so happy to go ahead with it. Copying information is not required since instantiating the streams over the fds will create it.

[SimenB]

should we import from readable-stream instead to avoid inconsistencies between node versions?

[rubennorte]

Yeah, we can do that, given that we're already depending on that module transitively.

[SimenB]

For now, I think the suggestion @rubennorte made is good enough; so happy to go ahead with it.

Yeah, agreed!

[rubennorte]

@mjesun how would your solution behave regarding the mutating methods? AFAIK if you use the same fd you'd be sending those commands to the real terminal (unless you're also suggesting we should make those noops).

[rubennorte]

Even though this could be considered a fix, we can wait until the next major to ship it. It's pretty breaking if someone is depending on the current observable behaviour, which isn't unlikely.

[SimenB]

Definitely agree with waiting for 25 to ship this. Potentially add a new flag allowing opt-in, then make the behavior the default and remove the flag in 25?

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