Test concurrency causes the Test Runner to report a wrong isTTY value

[nicholaswmin]

another one for the test runner:

Description

Using child_process for test parallisation also sets process.stdout.isTTY to undefined.

The following returns undefined when it's run with node --test:

# node --test example.test.js
console.log(process.stdout.isTTY)
# undefined

when it's run with plain node, it's true:

# node example.test.js
console.log(process.stdout.isTTY)
# true

I had a very faint memory of a similar issue described by someone else,
very similar to this and IIRC about the child_process.

So, I run the same with --experimental-test-isolation=none and there it is: it returns true which confirms my suspicions.

# node --test --experimental-test-isolation=none example.test.js
console.log(process.stdout.isTTY)
# true

Although you can work around it, it's very problematic for the following reasons:

• It's inconsistent with POLA
• It' inconsistent with it's own API because it reports one way with one flag, and another without.
• It leaks implementation details

How the tests are run is an implementation detail that shouldn't be leaking like that.
I'm sure there are some valid reasons for it but this feels plain wrong.

Thoughts?

Note: I only suspect these 2 issues are related.

[preveen-stack]

@nodejs/child_process Can you advise

[gireeshpunathil]

with --test option, the process is running as a child of the test harness I guess, which effectively resembles child in the below code.

const { fork } = require('node:child_process');
const process = require('node:process');

if (process.argv[2] === 'child') {
	console.log(process.stdout.isTTY)
} else {
  const child = fork(__filename, ['child'], {silent: true})
  child.stdout.on('data', (d) => {
    console.log(d.toString())
  });
}

in that, the standard streams (including stdout) is piped into the parent - so it is a pipe, not a tty.

apparently the isTTY field of the stdout object is not set (as false) in that scenario looks like a flaw to me

[nicholaswmin]

Hi,

That's bang on, exactly what I remember. There's was a discussion about this somewhere, IIRC.

with --test option, the process is running as a child of the test harness I guess

yes, hence why the test-isolation switch affects it. A value of 'none' means there's no child processes.

so it is a pipe, not a tty.

Well, it is a tty. The fact that it's a child process is an implementation detail. Is isTTY should report true in this case.

Is that what you're saying as well?

Also, can we move this to NodeJS issues?

[gireeshpunathil]

Well, it is a tty. The fact that it's a child process is an implementation detail. Is isTTY should report true in this case.

let me explain. when a child process is spawned, there are many ways you can configure it's standard streams. when it is piped into the parent, the default fd is closed and fds 0, 1, 2 are attached to one of a pipe, whose other end is given to the parent - that way the parent can manage the child in / out / err data.

below is the logic to check the device type - which isTTY depends on. so in the pipe case, if will not be a tty, instead a socket.

https://github.com/libuv/libuv/blob/467859c2ba0beadbb536ebde919e3883c7b4bc11/src/unix/tty.c#L387-L472

makes sense?

[nicholaswmin]

Hi Gireesh, well aware. I've written threadpools on top of the child_process module so I know exactly how that mechanism works.

What I'm saying here is that this is a classic case of a leaky abstraction.

A leaky abstraction in software development refers to a design flaw where an abstraction, intended to simplify and hide the underlying complexity of a system, fails to completely do so.

This is akin to traffic lights that turn green and orange but never red, and this is normal because the manager of the red-light-bulbs factory in China has had a fight with his wife and is lagging lightbulb production. No sarcasm intended here, just an analogy.

[gireeshpunathil]

@nicholaswmin - I think the disconnect is from my misunderstanding of your statement Well, it is a tty.

you might have meant it as it should be a tty, the process is running as a child (with --test flag) should not affect the baseline scenario (without --test) . is that right?

I took it as it is still a tty (even if the child is piped into the parent) . I then modified the code above to see what it is actually and I got pipe.

I have no disagreement with you on the expected behaviour; only on the interpretation of the current behaviour.

are we good?

[nicholaswmin]

I have no disagreement with you on the expected behaviour; are we good?

I think so but not 100%; but It's my bad since I wasn't entirely clear the 1st time around.

I'll rephrase it now, get rid of any ambiguity and provide context into my assumptions. Here goes:

isTTY: means is TeletypeWriter, an archaic notion which nowadays basically means = "do I (possibly) have a human looking at a terminal screen? Or am I being sent to machine logs?"

This is the flag I use to decide whether errors should be logged with formatting, for example. Otherwise I'll mess up my logs with unnecessary newlines that only matter if a human is reading that error.

contd;

based on this assumption, yes you do have a human (me) looking at the test results on my terminal. Therefore, isTTY === true is what I expect to see, regardless if I'm running node foo.js, oor node --test foo.js

I know isTTY detection isn't an exact process, and detection is made on a best-effort basis. It could return isTTY = false.
That's almost OK, but the results should be consistent throughout.

Having node --test foo.js and node foo-js.js return different values is really confusing. Either all should be false or all should be true.

Again, I'm running these tests in my terminal so the isTTY === true, everywhere, is the only correct value. How that's done or whatever goes on internally, is none of my business (as an API consumer), as those are abstracted from me.

Now, as a software engineer, I can understand that what is correct and what's doable
often diverge. Fine, but it's still a bug; plus this case is confusingly inconsistent, in addition to being wrong. I'd rather know something is wrong to avoid it altogether rather than being tricked in one mode vs another. node foo.js vs node --test foo.js

points:

1. isTTY should be true, everywhere, when there's a terminal and a person.
   2. The above case I presented has a a terminal yet it returns isTTY = undefined.
   Therefore it is wrong. Additionally it's confusing because it's not always wrong.

2. if not possible, it should be treated as a bug

3. if it can't be fixed for ABC reasons, it should also be documented as a warning in the docs..

Thoughts?

This begs the question: How did the test runner team implement ANSI stripping in the built-in reporters if isTTY is incorrect?