No error diff color on forks pool

[hi-ogawa]

Describe the bug

Not sure why I just noticed this now, but it looks like we don't have diff color on forks.

https://stackblitz.com/edit/vitest-dev-vitest-cnfjynoc?file=test%2Fbasic.test.ts

npm run test

npm run test --pool threads

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-cnfjynoc?file=test%2Fbasic.test.ts

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    vitest: 2.1.8 => 2.1.8

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[hi-ogawa]

It turned out this is due to tinypool v1.0.2 tinylibs/tinypool#104.

• forks: tinypool v1.0.1 https://stackblitz.com/edit/vitest-dev-vitest-hhruvape?file=test%2Fsuite.test.js

• forks: tinypool v1.0.2 https://stackblitz.com/edit/vitest-dev-vitest-gxjfg4h7?file=package.json

• threads: : tinypool v1.0.2

@AriPerkkio Do you have some idea for fix? tty becoming false for users is not probably crucial per-se, but our own color logic not working on default setup seems bad.

Btw, tinyrainbow is checking isatty(1) and a few more conditions to decide enabling color https://github.com/tinylibs/tinyrainbow/blob/c8202ffc00d9f7f039ebdc2af52d08c6376fb6c0/src/node.ts#L6
https://github.com/tinylibs/tinyrainbow/blob/c8202ffc00d9f7f039ebdc2af52d08c6376fb6c0/src/index.ts#L87-L93

[AriPerkkio]

Good catch @hi-ogawa, I wasn't expecting this.

Here's minimal reproduction without Tinypool demonstrating the fix from tinylibs/tinypool#104. Happy to change the implementation if another approach is found:

Intercept stdout of node:child_process in main process

import { writeFileSync } from "node:fs";
import { fork } from "node:child_process";
import { setTimeout } from "node:timers/promises";

writeFileSync(
  "./worker.mjs",
  `
process.stdout.write("Message\\n");
process.stderr.write("Error message\\n");
  `.trim(),
  "utf8"
);

const write = process.stdout.write.bind(process.stdout);

process.stdout.write = (data) => {
  write(`Captured stdout: ${data}`);
};

process.stderr.write = (data) => {
  write(`Captured stderr: ${data}`);
};

{
  // Unable to intercept stdout
  const subprocess = fork("./worker.mjs");

  await setTimeout(3_000);
  subprocess.kill();
}

{
  // Intercepts stdout
  const subprocess = fork("./worker.mjs", { stdio: "pipe" });
  subprocess.stdout.pipe(process.stdout);
  subprocess.stderr.pipe(process.stderr);

  await setTimeout(3_000);
  subprocess.kill();
}

Intercept stdout of node:worker_threads, works automatically

import { writeFileSync } from "node:fs";
import { Worker } from "node:worker_threads";
import { setTimeout } from "node:timers/promises";

writeFileSync(
  "./worker.mjs",
  `
process.stdout.write("Message\\n");
process.stderr.write("Error message\\n");
  `.trim(),
  "utf8"
);

const write = process.stdout.write.bind(process.stdout);

process.stdout.write = (data) => {
  write(`Captured stdout: ${data}`);
};

process.stderr.write = (data) => {
  write(`Captured stderr: ${data}`);
};

const worker = new Worker("./worker.mjs");

await setTimeout(3_000);
await worker.terminate();

[hi-ogawa]

I guess we could pass down some flag env: { __VITEST_ISATTY__: isatty(1)} from main thread to test runtime and create tinyrainbow instance manually by createColors(process.env.__VITEST_ISATTY__ === 'true') to at least save our own color.

[AriPerkkio]

Yup that should work! It makes sense that TTY checks start to fail after changing the streams. Not sure why colors should be disabled when TTY is disabled though.

[AriPerkkio]

@sheremet-va what do you think about having process.env.FORCE_TTY here? https://github.com/tinylibs/tinyrainbow/blob/c8202ffc00d9f7f039ebdc2af52d08c6376fb6c0/src/index.ts#L83-L97

It could be used in cases like this, where main thread/process is TTY but spawned processes' streams aren't.