Skip to content

chore(e2e-test): add regression test for dynamic import() MONGOSH-1062 #2458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 4, 2025
Merged

chore(e2e-test): add regression test for dynamic import() MONGOSH-1062 #2458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64e90fc
chore(e2e-test): add regression test for dynamic import() MONGOSH-1062
addaleax May 22, 2025
f9d2960
fixup: `git add -f` so that node_modules changes are applied as well
addaleax May 28, 2025
eb369f0
Merge branch 'main' into 1062-dev
addaleax Jun 10, 2025
908e8d5
Merge branch 'main' into 1062-dev
addaleax Jun 16, 2025
4f546c5
fixup: review comment
addaleax Aug 4, 2025
41edd42
Merge branch 'main' into 1062-dev
addaleax Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions packages/e2e-tests/test/e2e.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1170,22 +1170,40 @@ describe('e2e', function () {
env: {
...process.env,
NODE_PATH: path.resolve(__dirname, 'fixtures', 'node-path'),
NODE_OPTIONS: '--expose-gc',
},
});
await shell.waitForPrompt();
shell.assertNoErrors();
});

it('require() searches the current working directory according to Node.js rules', async function () {
let result;
it('require() and import() search the current working directory according to Node.js rules', async function () {
let result: string;
result = await shell.executeLine('require("a")');
expect(result).to.match(/Error: Cannot find module 'a'/);
result = await shell.executeLine('require("./a")');
expect(result).to.match(/^A$/m);
result = await shell.executeLine('require("b")');
expect(result).to.match(/^B$/m);
result = await shell.executeLine('require("b-esm").value');
expect(result).to.match(/^B-ESM$/m);
result = await shell.executeLine('require("c")');
expect(result).to.match(/^C$/m);
result = await shell.executeLine('import("b").then(m => m.default)');
expect(result).to.match(/^B$/m);
result = await shell.executeLine('import("b-esm").then(m => m.value)');
expect(result).to.match(/^B-ESM$/m);
});

// Regression test for https://github.com/nodejs/node/issues/38695
it('import() works when interleaved with GC', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the relationship between the garbage collector and import that would cause issues?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gagik Sorry, completely missed this comment! The original ticket is nodejs/node#38695, I'll link it here in the comments as well – basically, Node.js internals had a memory management bug where a C++ object describing modules could get deleted during garbage collection while the "public" JS module instance was still alive, and then accessing that C++ object from JS would crash

Suggested change
it('import() works when interleaved with GC', async function () {
// Regression test for https://github.com/nodejs/node/issues/38695
it('import() works when interleaved with GC', async function () {

Obviously this is really more of a Node.js issue and one could argue that we don't need a specific regression test here in mongosh, but I'd feel more comfortable having it in place for when we start having a "proper" ESM story in mongosh (i.e. mongosh scripts with import/export).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! the link in comments is helpful.

await shell.executeLine('importESM = () => import("b-esm")');
expect(await shell.executeLine('globalThis.gc(); "ran gc"')).to.include(
'ran gc'
);
const result = await shell.executeLine('importESM().then(m => m.value)');
expect(result).to.match(/^B-ESM$/m);
shell.assertNoErrors();
});

it('Can use Node.js APIs without any extra effort', async function () {
Expand All @@ -1194,6 +1212,7 @@ describe('e2e', function () {
`fs.readFileSync(${JSON.stringify(__filename)}, 'utf8')`
);
expect(result).to.include('Too lazy to write a fixture');
shell.assertNoErrors();
});
});

Expand Down
1 change: 1 addition & 0 deletions packages/e2e-tests/test/fixtures/require-base/node_modules/b-esm/index.mjs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/e2e-tests/test/fixtures/require-base/node_modules/b-esm/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading