Skip to content

Commit

Permalink
fix(node): map stdio [0, 1, 2] to "inherit" (denoland#19352)
Browse files Browse the repository at this point in the history
<!--
Before submitting a PR, please read https://deno.com/manual/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix denoland#7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->
Internally, `node-tap` spawns a child process with `stdio: [0, 1, 2]`.
Whilst we don't support passing fd numbers as an argument so far, it
turns out that `[0, 1, 2]` is equivalent to `"inherit"` which we already
support. See: https://nodejs.org/api/child_process.html#optionsstdio

Mapping it to `"inherit"` is fine for us and gets us one step closer in
getting `node-tap` working. I'm now at the stage where already the
coverage table is shown 🎉
  • Loading branch information
marvinhagemeister authored Jun 2, 2023
1 parent 25fdc7b commit f5c1ff0
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 0 deletions.
22 changes: 22 additions & 0 deletions cli/tests/unit_node/child_process_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,28 @@ Deno.test(
},
);

Deno.test(
"[node/child_process spawn] supports stdio [0, 1, 2] option",
async () => {
const cmdFinished = deferred();
let output = "";
const script = path.join(
path.dirname(path.fromFileUrl(import.meta.url)),
"testdata",
"child_process_stdio_012.js",
);
const cp = spawn(Deno.execPath(), ["run", "-A", script]);
cp.stdout?.on("data", (data) => {
output += data;
});
cp.on("close", () => cmdFinished.resolve());
await cmdFinished;

assertStringIncludes(output, "foo");
assertStringIncludes(output, "close");
},
);

Deno.test({
name: "[node/child_process spawn] supports SIGIOT signal",
ignore: Deno.build.os === "windows",
Expand Down
15 changes: 15 additions & 0 deletions cli/tests/unit_node/testdata/child_process_stdio_012.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import childProcess from "node:child_process";
import process from "node:process";
import * as path from "node:path";

const script = path.join(
path.dirname(path.fromFileUrl(import.meta.url)),
"node_modules",
"foo",
"index.js",
);

const child = childProcess.spawn(process.execPath, [script], {
stdio: [0, 1, 2],
});
child.on("close", () => console.log("close"));
7 changes: 7 additions & 0 deletions ext/node/polyfills/internal/child_process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,13 @@ function normalizeStdioOption(
...Array<Stream | NodeStdio | number>,
] {
if (Array.isArray(stdio)) {
// `[0, 1, 2]` is equivalent to `"inherit"`
if (
stdio.length === 3 && stdio[0] === 0 && stdio[1] === 1 && stdio[2] === 2
) {
return ["inherit", "inherit", "inherit"];
}

// At least 3 stdio must be created to match node
while (stdio.length < 3) {
ArrayPrototypePush(stdio, undefined);
Expand Down

0 comments on commit f5c1ff0

Please sign in to comment.