test_runner: allow requiring reporter from node_modules #45916
Conversation
|
Review requested:
|
nodejs-github-bot
added
dont-land-on-v14.x
needs-ci
| const kDefaultReporter = 'tap'; | ||
| const kDefaultDestination = 'stdout'; | ||
|
|
||
| let paerentModule = null; |
There was a problem hiding this comment.
| let paerentModule = null; | |
| let parentModule = null; |
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Should we make it "anything that's accepted byimport" so we can support loading from e.g. https: URLs in the future?
|
I'm confused; why wouldn't this just work exactly like If node lacks an |
| * @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?
| }); | ||
| }); | ||
|
|
||
| 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 |
5 participants
No description provided.