feat(NODE-7335): Create dedicated mocha runner with isolated vm context#4876
feat(NODE-7335): Create dedicated mocha runner with isolated vm context#4876PavelSafronov wants to merge 38 commits intomongodb:mainfrom
Conversation
…o the bundled export file
…thub.com/PavelSafronov/node-mongodb-native into NODE-7335-bundle-and-barrel-approach-poc
…nd export a few more relevant types
There was a problem hiding this comment.
Pull request overview
This PR implements a Node-less runtime testing system that allows running unit and integration tests against a bundled version of the MongoDB Node.js driver where access to specific Node.js core modules (like 'os') is blocked. This ensures the driver can work in non-Node runtimes without accidentally introducing dependencies on Node.js-specific APIs.
Changes:
- Created a VM context with restricted
require()function that blocks access to specific core modules while allowing them through an allowlist - Added esbuild-based bundling system to create a CommonJS bundle of the driver for testing
- Modified test imports across ~120+ test files to use a runtime-conditional barrel file (
mongodb_runtime-testing.ts) that switches between regular and bundled driver based onMONGODB_BUNDLEDenvironment variable - Added Evergreen CI variants for Windows and RHEL to run the full test suite in "nodeless" mode
- Added npm scripts for running bundled tests and switching between bundled/unbundled modes
Reviewed changes
Copilot reviewed 161 out of 164 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| test/tools/runner/vm_context_helper.ts | Creates VM context with restricted require() that blocks core modules |
| etc/bundle-driver.mjs | Bundles driver using esbuild, externalizing Node builtins and dependencies |
| etc/build-runtime-barrel.mjs | Generates conditional barrel file based on MONGODB_BUNDLED env var |
| test/mongodb_runtime-testing.ts | Auto-generated barrel file that exports from either regular or bundled driver |
| test/mongodb_bundled.ts | Manually maintained exports from the contextified VM module |
| test/mongodb.ts | Test entrypoint that exports driver and internal types (has duplicate exports) |
| package.json | Adds esbuild dependency and scripts for bundled testing |
| test/unit/nodeless.test.ts | Test to verify runtime-testing barrel matches environment variable |
| test/integration/crud/misc_cursors.test.ts | Uses ensureTypeByName instead of instanceof for VM compatibility |
| test/integration/crud/crud_api.test.ts | Uses ensureTypeByName instead of instanceof for VM compatibility |
| .evergreen/config.yml | Adds rhel80-large-nodeless and windows-2022-latest-large-nodeless variants |
| .evergreen/generate_evergreen_tasks.js | Generates nodeless build variant for each OS |
| test/readme.md | Documents the nodeless runtime testing system |
| Various test files (120+) | Changes imports from '../mongodb' to '../mongodb_runtime-testing' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "check:test": "npm run build:bundle && nyc mocha --config test/mocha_mongodb.js test/integration", | ||
| "check:test-bundled": "MONGODB_BUNDLED=true npm run check:test", | ||
| "check:unit": "npm run build:bundle && nyc mocha test/unit", | ||
| "check:unit-bundled": "MONGODB_BUNDLED=true npm run check:unit", |
There was a problem hiding this comment.
Similar to check:test-bundled, the check:unit-bundled script has the same issue. The MONGODB_BUNDLED environment variable is set for the check:unit script invocation, but build:bundle is called within check:unit without the environment variable being propagated. This means the barrel file will be generated incorrectly. Consider changing to: "MONGODB_BUNDLED=true npm run build:bundle && MONGODB_BUNDLED=true nyc mocha test/unit" or restructuring the scripts for proper environment variable propagation.
| "check:unit-bundled": "MONGODB_BUNDLED=true npm run check:unit", | |
| "check:unit-bundled": "MONGODB_BUNDLED=true npm run build:bundle && MONGODB_BUNDLED=true nyc mocha test/unit", |
There was a problem hiding this comment.
Copilot might have smoked something funny, environment variables are definitely propagated to child processes, because, like, that's the whole point of why they exist?
There was a problem hiding this comment.
Yeah, that's a surprising comment, that's definitely not how env vars work. :/
test/mongodb.ts
Outdated
| export * from '../src/cursor/list_collections_cursor'; | ||
| export * from '../src/cursor/list_collections_cursor'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'list_collections_cursor' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
test/mongodb.ts
Outdated
| export * from '../src/cursor/list_indexes_cursor'; | ||
| export * from '../src/cursor/list_indexes_cursor'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'list_indexes_cursor' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
test/mongodb.ts
Outdated
| export * from '../src/mongo_types'; | ||
| export * from '../src/mongo_types'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'mongo_types' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
test/mongodb.ts
Outdated
| export * from '../src/operations/aggregate'; | ||
| export * from '../src/operations/aggregate'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'aggregate' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
| it('runNodelessTests variable should match env vars', function () { | ||
| const nodelessEnv = env.NODELESS; | ||
| const expectedNodeless = nodelessEnv === 'true'; | ||
| const actualNodeless = runNodelessTests; | ||
| expect(actualNodeless).to.equal( | ||
| expectedNodeless, | ||
| "runNodelessTests variable does not match NODELESS env var, run 'npm run build:runtime-barrel' to update the barrel file" | ||
| ); |
There was a problem hiding this comment.
The test expects the environment variable NODELESS to match MONGODB_BUNDLED, but according to the build scripts and README, the environment variable that controls bundling is MONGODB_BUNDLED, not NODELESS. This test will always fail unless both environment variables are set consistently. Either update this test to check MONGODB_BUNDLED instead of NODELESS, or update the documentation and build scripts to use NODELESS consistently.
There was a problem hiding this comment.
Good catch, fixed the inconsistency between names.
| const callStack = new Error().stack; | ||
| const correctPath = platform() === 'win32' ? path.win32 : path.posix; | ||
| const methodAndFile = callStack.split('\n')[2]; | ||
| const match = methodAndFile.match(/at (.*) \((.*)\)/); | ||
| const method = match ? match[1] : null; | ||
| const sourceFileAndLineNumbers = match ? match[2] : null; | ||
| const sourceFile = | ||
| sourceFileAndLineNumbers.indexOf('.ts:') !== -1 | ||
| ? sourceFileAndLineNumbers.substring(0, sourceFileAndLineNumbers.lastIndexOf('.ts:') + 3) | ||
| : sourceFileAndLineNumbers; |
There was a problem hiding this comment.
Stack trace parsing may fail if the call stack format is unexpected. The code assumes the third line of the stack trace (index 2) contains the caller information and uses a regex to extract method and file. If the stack trace format changes or if the regex doesn't match, match will be null, but the code proceeds to use match[1] and match[2] without checking. Consider adding null checks or a fallback to handle cases where the regex doesn't match. For example, if the stack trace format is different in the bundled context, this could silently fail to block restricted modules.
There was a problem hiding this comment.
Are you okay with using a V8-specific API here? If so: https://v8.dev/docs/stack-trace-api#customizing-stack-traces
There was a problem hiding this comment.
Tried the prepareStackTrace approach, and while it's nice to be able to use NodeJS.CallSite, the data that came back is different from the stack traces produced by Error:
- the V8 approach reports that the
resolveRuntimeAdapterscall is coming from the bundle (/Users/pavel.safronov/code/node-mongodb-native/test/tools/runner/bundle/driver-bundle.js:18291:40) - JS approach reports that
resolveRuntimeAdaptersis coming from source (/Users/pavel.safronov/code/node-mongodb-native/src/runtime_adapters.ts:47:40)
Here's the code for just getting this info, from a local unit test run:
return function restrictedRequire(moduleName: string) {
// Block core modules
if (isBuiltin(moduleName) && blockedModules.has(moduleName)) {
console.log(`pavel >>> simple stack trace: ${new Error().stack}`);
Error.prepareStackTrace = (_, stack) => stack;
const callStack = new Error().stack as any as NodeJS.CallSite[];
const debugInfo = callStack
.map(call => {
const fileName = call.getFileName();
const lineNumber = call.getLineNumber();
const columnNumber = call.getColumnNumber();
const functionName = call.getFunctionName();
return `at ${functionName} (${fileName}:${lineNumber}:${columnNumber})`;
})
.join('\n');
console.log(`pavel >>> Blocked require of core module '${moduleName}' from:\n${debugInfo}`);
There was a problem hiding this comment.
Ah yeah, makes sense that the V8 stack wouldn't yet have source maps applied to it. I think that's fine then :)
test/mongodb.ts
Outdated
| export * from '../src/cursor/find_cursor'; | ||
| export * from '../src/cursor/find_cursor'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'find_cursor' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
test/mongodb.ts
Outdated
| export * from '../src/mongo_logger'; | ||
| export * from '../src/mongo_logger'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'mongo_logger' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
test/mongodb.ts
Outdated
| export * from '../src/operations/client_bulk_write/common'; | ||
| export * from '../src/operations/client_bulk_write/common'; |
There was a problem hiding this comment.
Duplicate export statement detected. The file 'common' is exported twice on consecutive lines. This creates redundant exports that could cause confusion. Remove one of the duplicate export lines.
| context('when 100s of operations are executed and complete', () => { | ||
| beforeEach(function () { | ||
| if (this.currentTest && typeof v8.queryObjects !== 'function') { | ||
| if ((this.currentTest && typeof v8.queryObjects !== 'function') || runNodelessTests) { |
There was a problem hiding this comment.
Fyi – I've recently added a test to mongosh that tracks cross-context resource cleanup: mongodb-js/mongosh@73a6905
It's probably quite a bit more expensive to scan full snapshots than to use v8.queryObjects(), so I'm not recommending that approach, but I wanted to mention it to say "it's possible, if you want to"
| "check:test": "npm run build:bundle && nyc mocha --config test/mocha_mongodb.js test/integration", | ||
| "check:test-bundled": "MONGODB_BUNDLED=true npm run check:test", | ||
| "check:unit": "npm run build:bundle && nyc mocha test/unit", | ||
| "check:unit-bundled": "MONGODB_BUNDLED=true npm run check:unit", |
There was a problem hiding this comment.
Copilot might have smoked something funny, environment variables are definitely propagated to child processes, because, like, that's the whole point of why they exist?
| const exportsContainer = {}; | ||
| const moduleContainer = { exports: exportsContainer }; |
There was a problem hiding this comment.
| const exportsContainer = {}; | |
| const moduleContainer = { exports: exportsContainer }; | |
| const exportsContainer = { __proto__: null }; | |
| const moduleContainer = { __proto__: null, exports: exportsContainer }; |
not strictly necessary, but probably also no reason not to add this extra bit of strictness
| Date: global.Date, | ||
| Error: global.Error, |
There was a problem hiding this comment.
| Date: global.Date, | |
| Error: global.Error, |
I'd recommend avoiding re-defining JS spec variables
There was a problem hiding this comment.
I think we need this. If we don't re-define Error like this, then we start to see failures in checks like expect(...).to.throw(MongoParserError):
AssertionError: expected [Function] to throw 'MongoParseError' but MongoParseError: timeoutMS can only be a positive int value, got: -1 { errorLabelSet: Set{} } was thrown
There was a problem hiding this comment.
I think that's mainly because we don't have the fix in chaijs/check-error@d3622a0 yet
There was a problem hiding this comment.
Will add a TODO here so we can remove this line once we are able to upgrade to chai 5 and a more recent version of check-error.
There was a problem hiding this comment.
Adding TODO and link to https://jira.mongodb.org/browse/NODE-7460
| Promise: Promise, | ||
| Map: Map, | ||
| Set: Set, | ||
| WeakMap: WeakMap, | ||
| WeakSet: WeakSet, | ||
| ArrayBuffer: ArrayBuffer, | ||
| SharedArrayBuffer: SharedArrayBuffer, | ||
| Atomics: Atomics, | ||
| DataView: DataView, | ||
| Int8Array: Int8Array, | ||
| Uint8Array: Uint8Array, | ||
| Uint8ClampedArray: Uint8ClampedArray, | ||
| Int16Array: Int16Array, | ||
| Uint16Array: Uint16Array, | ||
| Int32Array: Int32Array, | ||
| Uint32Array: Uint32Array, | ||
| Float32Array: Float32Array, | ||
| Float64Array: Float64Array, | ||
| BigInt64Array: BigInt64Array, | ||
| BigUint64Array: BigUint64Array, |
There was a problem hiding this comment.
| Promise: Promise, | |
| Map: Map, | |
| Set: Set, | |
| WeakMap: WeakMap, | |
| WeakSet: WeakSet, | |
| ArrayBuffer: ArrayBuffer, | |
| SharedArrayBuffer: SharedArrayBuffer, | |
| Atomics: Atomics, | |
| DataView: DataView, | |
| Int8Array: Int8Array, | |
| Uint8Array: Uint8Array, | |
| Uint8ClampedArray: Uint8ClampedArray, | |
| Int16Array: Int16Array, | |
| Uint16Array: Uint16Array, | |
| Int32Array: Int32Array, | |
| Uint32Array: Uint32Array, | |
| Float32Array: Float32Array, | |
| Float64Array: Float64Array, | |
| BigInt64Array: BigInt64Array, | |
| BigUint64Array: BigUint64Array, |
| Symbol: Symbol, | ||
| Proxy: Proxy, | ||
| Reflect: Reflect, | ||
| Object: Object, | ||
| Array: Array, | ||
| Function: Function, | ||
| String: String, | ||
| Number: Number, | ||
| Boolean: Boolean, | ||
| RegExp: RegExp, | ||
| Math: Math, | ||
| JSON: JSON, | ||
| Intl: global.Intl, |
There was a problem hiding this comment.
| Symbol: Symbol, | |
| Proxy: Proxy, | |
| Reflect: Reflect, | |
| Object: Object, | |
| Array: Array, | |
| Function: Function, | |
| String: String, | |
| Number: Number, | |
| Boolean: Boolean, | |
| RegExp: RegExp, | |
| Math: Math, | |
| JSON: JSON, | |
| Intl: global.Intl, |
| }); | ||
|
|
||
| // Make global and globalThis point to the sandbox | ||
| sandbox.global = sandbox; |
There was a problem hiding this comment.
Do we need global? I think reducing reliance on that would be a reasonable (although maybe not high-priority) goal of this project, since it's a legacy Node.js-specific feature with a clear replacement path
| * Creates a require function that blocks access to specified core modules | ||
| */ | ||
| function createRestrictedRequire() { | ||
| const blockedModules = new Set(['os']); |
There was a problem hiding this comment.
Should this be an allowlist rather than a blocklist? In particular, shouldn't it also disallow requires to non-core modules by default?
| const callStack = new Error().stack; | ||
| const correctPath = platform() === 'win32' ? path.win32 : path.posix; | ||
| const methodAndFile = callStack.split('\n')[2]; | ||
| const match = methodAndFile.match(/at (.*) \((.*)\)/); | ||
| const method = match ? match[1] : null; | ||
| const sourceFileAndLineNumbers = match ? match[2] : null; | ||
| const sourceFile = | ||
| sourceFileAndLineNumbers.indexOf('.ts:') !== -1 | ||
| ? sourceFileAndLineNumbers.substring(0, sourceFileAndLineNumbers.lastIndexOf('.ts:') + 3) | ||
| : sourceFileAndLineNumbers; |
There was a problem hiding this comment.
Are you okay with using a V8-specific API here? If so: https://v8.dev/docs/stack-trace-api#customizing-stack-traces
| method: 'resolveRuntimeAdapters', | ||
| module: 'os' | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Not sure if this is the ideal approach ... maybe our custom require() could take a second argument that can be used to say "Yeah, I know what I'm doing, please allow requiring this module even if it's not otherwise allowed"? I don't know if bundlers can handle that, though.
There was a problem hiding this comment.
Another approach is to modify the tests to always pass a test-created runtime (so the tests are doing require('os'), which won't be caught by this logic), but we would have to update every instance where we create clients.
Keep this "less than optimal" approach for now and add a TODO NODE task for coming up with a better approach?
There was a problem hiding this comment.
Another approach is to modify the tests to always pass a test-created runtime (so the tests are doing
require('os'), which won't be caught by this logic), but we would have to update every instance where we create clients.
Yeah, I don't think we'd want to do this, that would be both very verbose and would mean that we don't really cover the require('os') path anymore.
Keep this "less than optimal" approach for now and add a TODO NODE task for coming up with a better approach?
I don't think it's blocking but it's a quite a bit of extra complexity that we're likely to remove again later, so I wouldn't mind addressing it in this PR here
There was a problem hiding this comment.
What about exposing a global __driver_require method, which is an unrestricted require? If that global is available then Driver uses it; otherwise, fall back to using require.
Removes all complexity from this file, and on the Driver side we just do this:
export function resolveRuntimeAdapters(options: MongoClientOptions): Runtime {
const correctRequire = (globalThis as any).__driver_require || require;
return {
os: options.runtimeAdapters?.os ?? correctRequire('os')
};
}
Description
Summary of Changes
Adds a way to run unit and integration tests against a special version of Node Driver where we block specific
requirecalls. This is so we can be confident that our updates don't accidentally re-introduce a dependence on Node.This PR also adds a number of Evergreen tasks for testing various combinations of the server with the "Node-less" VM. In these tasks we run all integ tests and most unit tests against the bundled and contextified version of the driver.
Notes for Reviewers
Take a look at the README updates, that should explain a lot about how this bundled testing works.
Below are steps for testing this out locally with timeout unit tests and CRUD API integ tests.
Steps to test unit tests
src/timeout.tsaround line 62, in the private constructornpm run check:unitnpm run check:unit-bundledSteps to test unit tests
src/mongo_client.ts, around line 430, in the constructornpm run check:testnpm run check:test-bundledWhat is the motivation for this change?
To fail our tests if our code starts to use a prohibited import, so we can guarantee that the driver will work in a non-Node runtime.
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript