-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: added support for passing a path to a custom reporter when usin… #1136
feat: added support for passing a path to a custom reporter when usin… #1136
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
async function loadCustomReporterModule<C extends Reporter>(path: string): Promise<new () => C> { | ||
let customReporterModule: { default: new() => C } | ||
try { | ||
customReporterModule = await import(path) |
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.
Maybe we should what globalSetup does? Otherwise users cannot use typescript and other features.
It's better if we load the module with ViteNode, I think.
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.
I'll take a look and see what I can do 👍🏻
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.
@sheremet-va I've revised the code by moving the ViteNodeRunner out of GlobalSetup and into VItest to support loading both TS and JS files in both ESM and CommonJS formats. The ViteNodeRunner is exposed through the runner
property/field on Vitest so that GlobalSetup still has access to it.
Also, should we consider updating the docs to mention support for custom reporters via the CLI? If so, should I reach out to anyone before making those changes?
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.
Docs are located in docs folder. To run it, use pnpm run docs:dev
, I think?
Just add enough information there and we are good 👍
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.
@sheremet-va sorry for the wait. Just pushed up a small change to the docs for command line options. Let me know if more needs to be added or if it's not quite clear. Thanks
…rt loading both TS and JS files in ESM and CommonJS format. Refactored tests
…r new in customer reporter module definition Co-authored-by: Anjorin Damilare <damilareanjorin1@gmail.com>
return stdout | ||
} | ||
|
||
describe('Custom reporters', () => { |
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.
It would also be great to see a test case for "custom reporter defined in configuration using path".
test: {
reporters: ['./src/custom-reporter'],
}
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.
@AriPerkkio the first test case is one that uses a custom reporter defined in a config file
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.
Sure, that one is a reporter instance instead of a string path.
reporters: [new TestReporter()], |
I don't think anything covers "custom reporter defined in configuration using path" yet.
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
@sheremet-va is this ready to go? Or would you like additional changes? |
LGTM 👍 |
feat: #1122