Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No error diff color on forks pool #7042

Open
6 tasks done
hi-ogawa opened this issue Dec 7, 2024 · 5 comments · May be fixed by #7090
Open
6 tasks done

No error diff color on forks pool #7042

hi-ogawa opened this issue Dec 7, 2024 · 5 comments · May be fixed by #7090
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) upstream

Comments

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Dec 7, 2024

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

image

npm run test --pool threads

image

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

@hi-ogawa hi-ogawa added pending triage p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Dec 7, 2024
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Dec 13, 2024

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

image

image

  • threads: : tinypool v1.0.2

image

@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

@hi-ogawa hi-ogawa moved this to P2 - 3 in Team Board Dec 13, 2024
@AriPerkkio
Copy link
Member

AriPerkkio commented Dec 13, 2024

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
Copy link
Contributor Author

hi-ogawa commented Dec 13, 2024

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) upstream
Projects
Status: Approved
Development

Successfully merging a pull request may close this issue.

2 participants