Conversation
test/tools/runner/vm_runner.js
Outdated
|
|
||
| function createSandboxContext(filename) { | ||
| const exportsContainer = {}; | ||
| return { |
There was a problem hiding this comment.
| return { | |
| return { | |
| __proto__: null, |
Just so none of the property lookups for built-ins fall back to the global one unintentionally
test/tools/runner/vm_runner.js
Outdated
| exports: exportsContainer, | ||
| module: { exports: exportsContainer }, | ||
| __filename: filename, | ||
| __dirname: path.dirname(filename), |
There was a problem hiding this comment.
I'd try to emulate the behavior of the built-in CommonJS evaluator as closely as possible.
None of these are typically globals, and modules do share a single global – all of require, module, exports, __filenameand__dirname` are injected as parameters to an implicit function that is used to wrap around user code
(you should generally be able to use that file as a reference for a lot of this)
There was a problem hiding this comment.
That is awesome, thanks! I haven't seen this file before, this is huge help indeed.
There was a problem hiding this comment.
Added the same style wrapper around the script and define "globals" (context) just once so it's shared.
test/tools/runner/vm_runner.js
Outdated
| return function sandboxRequire(moduleIdentifier) { | ||
| if (!moduleIdentifier.startsWith('.')) { | ||
| return require(moduleIdentifier); | ||
| } |
There was a problem hiding this comment.
Is this intentional? I figured we'd want to require almost everything inside the sandbox except for Node.js built-ins and maybe a list of modules that are explicit exceptions to this rule
There was a problem hiding this comment.
This is temporary solution to be able to import all dev dependencies and to highlight the more general idea (this ticket and PR is spike), so I was focusing mostly on "running existing integration tests inside vm.Context sandbox". But you are absolutely right, ideally all prod dependencies should be required from the inside, otherwise it's pointless (if driver is browser-compatible but, say bson not - we can't run application).
|
Re: testing - there's a command for running tests against 3 local servers, but what is the command we should use to start those 3 servers? I tried starting a replica set ( |
tadjik1
changed the title
I actually don't know the answer yet. I mean you did everything right, I personally use |
tadjik1
changed the title
| // force instanceof to work across contexts by defining custom `instanceof` function | ||
| Object.defineProperty(value, Symbol.hasInstance, { | ||
| value: (instance: any) => { | ||
| return ( | ||
| instance && (instance.constructor.name === value.name || instance instanceof value) | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I don't really like this part, but I found this problem a little bit tricky. The problem is that host environment (and consequently chai) has its' own set of Error classes, so instanceof from inside the sandbox is broken. @addaleax, maybe there is more convenient way to make it work rather than re-defining instanceof?
| const sandboxedDependencies = ['bson']; | ||
| const isSandboxedDep = sandboxedDependencies.some( | ||
| dep => moduleIdentifier === dep || moduleIdentifier.startsWith(`${dep}/`) | ||
| ); | ||
| if (!moduleIdentifier.startsWith('.') && !isSandboxedDep) { | ||
| return require(moduleIdentifier); | ||
| } |
There was a problem hiding this comment.
@addaleax this seems to work kinda of fine, we can list dependencies that should be loaded from within the sandbox. i decided to go with this approach because list of dev and prod dependencies we need to import in host is large. but maybe i'm missing something and it should be the other way around (everything is inside except certain builtin modules)?
test/tools/runner/vm_runner.ts
Outdated
| } | ||
|
|
||
| // clientmetadata requires package.json to fetch driver's version | ||
| if (realPath.endsWith('.json')) { |
There was a problem hiding this comment.
Do we want to do this for any *.json file, or only for package.json?
There was a problem hiding this comment.
changed to package.json only
test/tools/runner/vm_runner.ts
Outdated
| return result; | ||
| } catch (err: any) { | ||
| moduleCache.delete(realPath); | ||
| console.error(`Error running ${realPath} in sandbox:`, err.stack); |
There was a problem hiding this comment.
err.stack doesn't include code, message, and a bunch of other important info. Suggest we log the entire err object.
| const isTestFile = realPath.includes('.test.ts') || realPath.includes('.test.js'); | ||
|
|
||
| let localBuffer = Buffer; | ||
| if (isSourceFile && !isTestFile) { |
There was a problem hiding this comment.
I guess this or similar way we can differentiate between source and test files, and allow 3rd party libraries (like bson) to use globals like Buffer. cc @PavelSafronov
| const isBSON = realPath.includes('node_modules/bson'); | ||
| const isError = realPath.includes('src/error.ts'); | ||
|
|
||
| if (isBSON || isError) { |
There was a problem hiding this comment.
In order instanceof to work we have to propagate certain classes between contexts, most notable all Mongo*Errors as well as bson.Long.
3 participants
Description
Summary of Changes
This POC implements a custom test runner using Node.js's
vmmodule to execute driver intergation tests in an isolated environment. The goal is to enforce the architectural deprecation ofBufferand other globals in source files while permitting it in tests.Notes for Reviewers
To run the tests:
Key features implemented / tested:
moduleCachefor the sandbox.Forbiddenerror ifBuffermethods (like.from,.alloc) or constructors are accessed.Symbol.hasInstanceon exportedBSONandErrorclasses.Object.prototype.isPrototypeOfto bridge the gap between Host-context assertions and Sandbox-context instances, ensuringinstanceofchecks pass without infinite recursion.Known problems, future tasks:
vm.createContextto keep basic prototype chains aligned with the host.Uncaught RangeError: Maximum call stack size exceededsometimes).What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript