Skip to content

Commit

Permalink
lib: fix assert shows diff messages in ESM and CJS
Browse files Browse the repository at this point in the history
This PR addresses an issue which was caused by the design in
the ESM loader.
The ESM loader was modifying the file path and replacing the 'file'
property with the file proto in the stack trace.
This, in turn, led to unhandled exceptions when the assert module
attempted to open the file to display erroneous code.
The changes in this PR resolve this issue by handling the file path
correctly, ensuring that the remaining message formatting code can
execute as expected.

PR-URL: #50634
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
MrJithil authored and targos committed Nov 11, 2023
1 parent 524911e commit 592b18f
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 1 deletion.
11 changes: 10 additions & 1 deletion lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ const CallTracker = require('internal/assert/calltracker');
const {
validateFunction,
} = require('internal/validators');
const { fileURLToPath } = require('internal/url');

let isDeepEqual;
let isDeepStrictEqual;
Expand Down Expand Up @@ -296,7 +297,7 @@ function getErrMessage(message, fn) {
overrideStackTrace.set(err, (_, stack) => stack);
const call = err.stack[0];

const filename = call.getFileName();
let filename = call.getFileName();
const line = call.getLineNumber() - 1;
let column = call.getColumnNumber() - 1;
let identifier;
Expand Down Expand Up @@ -330,6 +331,14 @@ function getErrMessage(message, fn) {
const { StringDecoder } = require('string_decoder');
decoder = new StringDecoder('utf8');
}

// ESM file prop is a file proto. Convert that to path.
// This ensure opensync will not throw ENOENT for ESM files.
const fileProtoPrefix = 'file://';
if (StringPrototypeStartsWith(filename, fileProtoPrefix)) {
filename = fileURLToPath(filename);
}

fd = openSync(filename, 'r', 0o666);
// Reset column and message.
({ 0: column, 1: message } = getCode(fd, line, column));
Expand Down
51 changes: 51 additions & 0 deletions test/parallel/test-assert-esm-cjs-message-verify.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

const { spawnPromisified } = require('../common');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const { writeFileSync, unlink } = require('fs');
const { describe, after, it } = require('node:test');

tmpdir.refresh();

const fileImports = {
cjs: 'const assert = require("assert");',
mjs: 'import assert from "assert";',
};

const fileNames = [];

for (const [ext, header] of Object.entries(fileImports)) {
const fileName = `test-file.${ext}`;
// Store the generated filesnames in an array
fileNames.push(`${tmpdir.path}/${fileName}`);

writeFileSync(tmpdir.resolve(fileName), `${header}\nassert.ok(0 === 2);`);
}

describe('ensure the assert.ok throwing similar error messages for esm and cjs files', () => {
const nodejsPath = `${process.execPath}`;
const errorsMessages = [];

it('should return code 1 for each command', async () => {
for (const fileName of fileNames) {
const { stderr, code } = await spawnPromisified(nodejsPath, [fileName]);
assert.strictEqual(code, 1);
// For each error message, filter the lines which will starts with AssertionError
errorsMessages.push(
stderr.split('\n').find((s) => s.startsWith('AssertionError'))
);
}
});

after(() => {
assert.strictEqual(errorsMessages.length, 2);
assert.deepStrictEqual(errorsMessages[0], errorsMessages[1]);

for (const fileName of fileNames) {
unlink(fileName, () => {});
}

tmpdir.refresh();
});
});

0 comments on commit 592b18f

Please sign in to comment.