Skip to content
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

Merged

Conversation

ericjgagnon
Copy link
Contributor

feat: #1122

@netlify
Copy link

netlify bot commented Apr 10, 2022

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 5f16964
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6266a8a9d8136900087050c0
😎 Deploy Preview https://deploy-preview-1136--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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)
Copy link
Member

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.

Copy link
Contributor Author

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 👍🏻

Copy link
Contributor Author

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?

Copy link
Member

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 👍

Copy link
Contributor Author

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
ericjgagnon and others added 2 commits April 15, 2022 23:08
…r new in customer reporter module definition

Co-authored-by: Anjorin Damilare <damilareanjorin1@gmail.com>
docs/guide/index.md Outdated Show resolved Hide resolved
test/reporters/tests/utils.test.ts Outdated Show resolved Hide resolved
test/reporters/tests/utils.test.ts Outdated Show resolved Hide resolved
return stdout
}

describe('Custom reporters', () => {
Copy link
Member

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'],
  }

Copy link
Contributor Author

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

Copy link
Member

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.

docs/config/index.md Outdated Show resolved Hide resolved
@ericjgagnon
Copy link
Contributor Author

@sheremet-va is this ready to go? Or would you like additional changes?

@sheremet-va
Copy link
Member

LGTM 👍

docs/config/index.md Outdated Show resolved Hide resolved
docs/guide/index.md Outdated Show resolved Hide resolved
@antfu antfu enabled auto-merge (squash) April 25, 2022 14:01
@antfu antfu merged commit f2bceb2 into vitest-dev:main Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants