test_runner: allow requiring reporter from node_modules #45916
Conversation
|
Review requested:
|
nodejs-github-bot
added
dont-land-on-v14.x
needs-ci
| @@ -100,10 +100,21 @@ const kBuiltinReporters = new SafeMap([ | |||
| const kDefaultReporter = 'tap'; | |||
| const kDefaultDestination = 'stdout'; | |||
|
|
|||
| let paerentModule = null; | |||
There was a problem hiding this comment.
| let paerentModule = null; | |
| let parentModule = null; |
|
I'm confused; why wouldn't this just work exactly like If node lacks an |
| @@ -35,7 +35,7 @@ function shouldUseESMLoader(filePath) { | |||
| * @returns {any} | |||
| * requireOrImport imports a module if the file is an ES module, otherwise it requires it. | |||
| */ | |||
| function requireOrImport(filePath) { | |||
There was a problem hiding this comment.
esmLoader.import‘s second parameter is parentURL, but you haven’t updated that call (on line 45 here) like you have for Module._load. If there were a test where the reporter were ESM (rather than the single new test in this PR, with reporter r as CommonJS) this would have been caught.
Why do we need this utility? Node.js has --import, which accepts either CommonJS or ESM. Can’t --test-reporter be an alias for that, or map to it?
| @@ -92,4 +92,13 @@ describe('node:test reporters', { concurrency: true }, () => { | |||
| assert.strictEqual(child.stdout.toString(), `${filename} {"test:start":5,"test:pass":2,"test:fail":3,"test:plan":3,"test:diagnostic":7}`); | |||
| }); | |||
| }); | |||
|
|
|||
| it('should support a custom reporter from node_modules', async () => { | |||
There was a problem hiding this comment.
Please add a test for a similar reporter that’s written in ESM.
|
See #45923; under that PR, it should already be possible to load a test reporter from inside |
|
closing in favor of #45923 |
No description provided.