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.
  • Loading branch information
MrJithil committed Nov 10, 2023
1 parent 33704c4 commit d7d41e3
Show file tree
Hide file tree
Showing 2 changed files with 65 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 { URL, fileURLToPath } = require('internal/url');

Check failure on line 76 in lib/assert.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

'URL' is assigned a value but never used

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
55 changes: 55 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,55 @@
'use strict';

require('../common');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const { writeFileSync, unlink } = require('fs');
const { execSync } = require('child_process');

tmpdir.refresh();

// To ensure the assert ok throwing similar error messages for esm and cjs

const nodejs = `${process.execPath}`;

// Importing package for each extensions.
const fileImports = {
mjs: 'import assert from "assert";',
js: 'const assert = require("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);`);
}

const errorsMessages = [];

for (const fileName of fileNames) {
const command = `${nodejs} ${fileName}`;
assert.throws(
() => {
execSync(command, { stdio: 'pipe' });
},
(e) => {
// For each error message, filter the lines which will starts with AssertionError
errorsMessages.push(
e.message.split('\n').find((s) => s.startsWith('AssertionError'))
);
return true;
}
);
}

assert.deepStrictEqual(errorsMessages[0], errorsMessages[1]);

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

tmpdir.refresh();

0 comments on commit d7d41e3

Please sign in to comment.