Repository: denoland/deno
Pull request: fix(ext/node): improve assert.ifError

This PR improves `assert.ifError` implementation and enables `parallel/test-assert-if-error.js` test case.

towards #29801 

Changed files:
filename: ext/node/polyfills/assert.ts
patch: @@ -21,7 +21,15 @@ import { primordials } from "ext:core/mod.js";
 import { CallTracker } from "ext:deno_node/internal/assert/calltracker.js";
 import { deprecate } from "node:util";
 
-const { ObjectPrototypeIsPrototypeOf } = primordials;
+const {
+  ArrayPrototypeIndexOf,
+  ArrayPrototypeJoin,
+  ArrayPrototypeSlice,
+  ObjectPrototypeIsPrototypeOf,
+  StringPrototypeIndexOf,
+  StringPrototypeSlice,
+  StringPrototypeSplit,
+} = primordials;
 
 function innerFail(obj: {
   actual?: unknown;
@@ -181,6 +189,7 @@ function throws(
       operator: "throws",
       actual: undefined,
       expected: error,
+      stackStartFn: throws,
     });
   } else if (typeof error === "string") {
     // Use case of throws(fn, message)
@@ -189,20 +198,23 @@ function throws(
       operator: "throws",
       actual: undefined,
       expected: undefined,
+      stackStartFn: throws,
     });
   } else if (typeof error === "function" && error?.prototype !== undefined) {
     throw new AssertionError({
       message: `Missing expected exception (${error.name}).`,
       operator: "throws",
       actual: undefined,
       expected: error,
+      stackStartFn: throws,
     });
   } else {
     throw new AssertionError({
       message: "Missing expected exception.",
       operator: "throws",
       actual: undefined,
       expected: error,
+      stackStartFn: throws,
     });
   }
 }
@@ -698,25 +710,27 @@ function ifError(err: any) {
       // This will remove any duplicated frames from the error frames taken
       // from within `ifError` and add the original error frames to the newly
       // created ones.
-      const tmp2 = origStack.split("\n");
-      tmp2.shift();
-
-      // Filter all frames existing in err.stack.
-      let tmp1 = newErr!.stack?.split("\n");
-
-      for (const errFrame of tmp2) {
-        // Find the first occurrence of the frame.
-        const pos = tmp1?.indexOf(errFrame);
-
-        if (pos !== -1) {
-          // Only keep new frames.
-          tmp1 = tmp1?.slice(0, pos);
-
-          break;
+      const origStackStart = StringPrototypeIndexOf(origStack, "\n    at");
+      if (origStackStart !== -1) {
+        const originalFrames = StringPrototypeSplit(
+          StringPrototypeSlice(origStack, origStackStart + 1),
+          "\n",
+        );
+        // Filter all frames existing in err.stack.
+        let newFrames = StringPrototypeSplit(newErr.stack, "\n");
+        for (const errFrame of originalFrames) {
+          // Find the first occurrence of the frame.
+          const pos = ArrayPrototypeIndexOf(newFrames, errFrame);
+          if (pos !== -1) {
+            // Only keep new frames.
+            newFrames = ArrayPrototypeSlice(newFrames, 0, pos);
+            break;
+          }
         }
+        const stackStart = ArrayPrototypeJoin(newFrames, "\n");
+        const stackEnd = ArrayPrototypeJoin(originalFrames, "\n");
+        newErr.stack = `${stackStart}\n${stackEnd}`;
       }
-
-      newErr.stack = `${tmp1?.join("\n")}\n${tmp2.join("\n")}`;
     }
 
     throw newErr;

filename: tests/node_compat/config.jsonc
patch: @@ -250,6 +250,7 @@
       "test-assert-builtins-not-read-from-filesystem.js",
       "test-assert-checktag.js",
       "test-assert-fail.js",
+      "test-assert-if-error.js",
       "test-assert.js",
       "test-async-hooks-run-in-async-scope-caught-exception.js",
       "test-async-hooks-run-in-async-scope-this-arg.js",

filename: tests/node_compat/runner/TODO.md
patch: @@ -1,7 +1,7 @@
 <!-- deno-fmt-ignore-file -->
 # Remaining Node Tests
 
-1211 tests out of 3993 have been ported from Node 23.9.0 (30.33% ported, 70.30% remaining).
+1212 tests out of 3993 have been ported from Node 23.9.0 (30.35% ported, 70.27% remaining).
 
 NOTE: This file should not be manually edited. Please edit `tests/node_compat/config.json` and run `deno task setup` in `tests/node_compat/runner` dir instead.
 
@@ -229,7 +229,6 @@ NOTE: This file should not be manually edited. Please edit `tests/node_compat/co
 - [parallel/test-assert-esm-cjs-message-verify.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-esm-cjs-message-verify.js)
 - [parallel/test-assert-fail-deprecation.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-fail-deprecation.js)
 - [parallel/test-assert-first-line.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-first-line.js)
-- [parallel/test-assert-if-error.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-if-error.js)
 - [parallel/test-assert-objects.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-objects.js)
 - [parallel/test-assert-typedarray-deepequal.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-assert-typedarray-deepequal.js)
 - [parallel/test-async-hooks-async-await.js](https://github.com/nodejs/node/tree/v23.9.0/test/parallel/test-async-hooks-async-await.js)

filename: tests/node_compat/test/parallel/test-assert-if-error.js
patch: @@ -0,0 +1,111 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 23.9.0
+// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually.
+
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const { test } = require('node:test');
+
+test('Test that assert.ifError has the correct stack trace of both stacks', () => {
+  let err;
+  // Create some random error frames.
+  (function a() {
+    (function b() {
+      (function c() {
+        err = new Error('test error');
+      })();
+    })();
+  })();
+
+  const msg = err.message;
+  const stack = err.stack;
+
+  (function x() {
+    (function y() {
+      (function z() {
+        let threw = false;
+        try {
+          assert.ifError(err);
+        } catch (e) {
+          assert.strictEqual(e.message,
+                             'ifError got unwanted exception: test error');
+          assert.strictEqual(err.message, msg);
+          assert.strictEqual(e.actual, err);
+          assert.strictEqual(e.actual.stack, stack);
+          assert.strictEqual(e.expected, null);
+          assert.strictEqual(e.operator, 'ifError');
+          threw = true;
+        }
+        assert(threw);
+      })();
+    })();
+  })();
+});
+
+test('General ifError tests', () => {
+  assert.throws(
+    () => {
+      const error = new Error();
+      error.stack = 'Error: containing weird stack\nYes!\nI am part of a stack.';
+      assert.ifError(error);
+    },
+    (error) => {
+      assert(!error.stack.includes('Yes!'));
+      return true;
+    }
+  );
+
+  assert.throws(
+    () => assert.ifError(new TypeError()),
+    {
+      message: 'ifError got unwanted exception: TypeError'
+    }
+  );
+
+  assert.throws(
+    () => assert.ifError({ stack: false }),
+    {
+      message: 'ifError got unwanted exception: { stack: false }'
+    }
+  );
+
+  assert.throws(
+    () => assert.ifError({ constructor: null, message: '' }),
+    {
+      message: 'ifError got unwanted exception: '
+    }
+  );
+
+  assert.throws(
+    () => { assert.ifError(false); },
+    {
+      message: 'ifError got unwanted exception: false'
+    }
+  );
+});
+
+test('Should not throw', () => {
+  assert.ifError(null);
+  assert.ifError();
+  assert.ifError(undefined);
+});
+
+test('https://github.com/nodejs/node-v0.x-archive/issues/2893', () => {
+  let threw = false;
+  try {
+    // eslint-disable-next-line no-restricted-syntax
+    assert.throws(() => {
+      assert.ifError(null);
+    });
+  } catch (e) {
+    threw = true;
+    assert.strictEqual(e.message, 'Missing expected exception.');
+    assert(!e.stack.includes('throws'), e);
+  }
+  assert(threw);
+});