-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_runner: allow requiring reporter from node_modules #45916
Conversation
Review requested:
|
@@ -100,10 +100,21 @@ const kBuiltinReporters = new SafeMap([ | |||
const kDefaultReporter = 'tap'; | |||
const kDefaultDestination = 'stdout'; | |||
|
|||
let paerentModule = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let paerentModule = null; | |
let parentModule = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.