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(test-runner): use require to resolve reporters #7749

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

JoelEinbinder
Copy link
Contributor

We import four different kinds of files:

config - has some very custom logic around folders and playwright.config.js
globalSetup/globalTeardown - Surprisingly allows relative paths without ./
tests - custom logic with matches
reporters - updated in this patch to use require.resolve

I can update globalSetup/globalTeardown to use require.resolve. I think
it is a bug that they accept relative paths without ./. But it would be
a breaking change, so I'll do it in another patch.

types/test.d.ts Outdated
@@ -213,7 +213,7 @@ interface ConfigBase {
* It is possible to pass multiple reporters. A common pattern is using one terminal reporter
* like `'line'` or `'list'`, and one file reporter like `'json'` or `'junit'`.
*/
reporter?: 'dot' | 'line' | 'list' | 'junit' | 'json' | 'null' | ReporterDescription[];
reporter?: string | ReporterDescription[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was explicit before so that autocomplete works. Now it will not suggest builtin reporters, unfortunately.

What if we force reporter: ['my-reporter-module'] for third-party reporters instead?

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 brought back the autocomplete.

tests/playwright-test/reporter.spec.ts Outdated Show resolved Hide resolved
tests/playwright-test/reporter.spec.ts Outdated Show resolved Hide resolved
src/test/util.ts Outdated Show resolved Hide resolved
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.

2 participants