test_runner: allow requiring reporter from node_modules - #45916
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
| const kDefaultReporter = 'tap'; | ||
| const kDefaultDestination = 'stdout'; | ||
|
|
||
| let paerentModule = null; |
There was a problem hiding this comment.
| let paerentModule = null; | |
| let parentModule = null; |
Sorry, something went wrong.
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?
Sorry, something went wrong.
|
I'm confused; why wouldn't this just work exactly like --require? If node lacks an --import then please add it, but why would it be appropriate for the test runner to add such a critical feature in a non-holistic way? |
Sorry, something went wrong.
| * @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?
Sorry, something went wrong.
| }); | ||
| }); | ||
|
|
||
| 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.
Sorry, something went wrong.
|
See #45923; under that PR, it should already be possible to load a test reporter from inside node_modules, since ESMLoader.import applies the module resolution algorithm which will look there. Is there another problem that this PR was intending to solve—something about preserving the parent module when loading from within node_modules? If there’s an issue with that, that would be good to fix in general for all module loading, rather than as a fix specific to test reporters. |
Sorry, something went wrong.
No description provided.