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

[Docs]: Improve documentation for metadata #34094

Closed
vitalets opened this issue Dec 19, 2024 · 9 comments · Fixed by #34329
Closed

[Docs]: Improve documentation for metadata #34094

vitalets opened this issue Dec 19, 2024 · 9 comments · Fixed by #34329
Assignees
Labels

Comments

@vitalets
Copy link
Contributor

Page(s)

https://playwright.dev/docs/next/api/class-testconfig#test-config-metadata

Description

1. Current metadata example does not pass TypeScript check:

import { defineConfig } from '@playwright/test';

export default defineConfig({
  metadata: 'acceptance tests',
});

Result:

Type 'string' is not assignable to type 'Metadata'.

2. Documentation should mention the certain fields that are allowed in metadata, e.g.

export type Metainfo = {
  'revision.id'?: string;
  'revision.author'?: string;
  'revision.email'?: string;
  'revision.subject'?: string;
  'revision.timestamp'?: number | Date;
  'revision.link'?: string;
  'ci.link'?: string;
  'timestamp'?: number
};
@dgozman
Copy link
Contributor

dgozman commented Jan 9, 2025

We discussed this issue, and are not sure how to improve it.

@dgozman dgozman closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2025
@dgozman
Copy link
Contributor

dgozman commented Jan 9, 2025

@vitalets We are open to bringing the commit info into the metadata under an option in the config. If that goes public, then we'll definitely document the fields and the html reporter will show them nicely. Would you like to contribute?

@vitalets
Copy link
Contributor Author

@vitalets We are open to bringing the commit info into the metadata under an option in the config. If that goes public, then we'll definitely document the fields and the html reporter will show them nicely. Would you like to contribute?

Yes, I'm ready to contribute. Could you guide me, how to name / where to place the config option?

@dgozman
Copy link
Contributor

dgozman commented Jan 10, 2025

Yes, I'm ready to contribute. Could you guide me, how to name / where to place the config option?

Something like TestConfig.populateGitInfo, and we'll finalize/rename before the release after the API review.

@vitalets
Copy link
Contributor Author

Something like TestConfig.populateGitInfo, and we'll finalize/rename before the release after the API review.

Ok! When TestConfig.populateGitInfo is enabled, Playwright internally activates GitCommitInfo plugin, that populates metadata?

@dgozman
Copy link
Contributor

dgozman commented Jan 10, 2025

Something like TestConfig.populateGitInfo, and we'll finalize/rename before the release after the API review.

Ok! When TestConfig.populateGitInfo is enabled, Playwright internally activates GitCommitInfo plugin, that populates metadata?

Exactly.

@vitalets
Copy link
Contributor Author

I thought more about it and got some concerns.
We are going to introduce one more test runner option (TestConfig.populateGitInfo) that actually populates another existing option (metadata), with kind of third-party data needed only for specific reporter (html).
Playwright configuration is already rather complex and has many options. Maybe move this option to html reporter itself? Or to GitInfo plugin somehow?
Otherwise, how will the following code behave from the performance perspective - if I enable gitinfo, but don't use html reporter:

export default defineConfig({
  populateGitInfo: true,
  reporter: 'list'
});

@dgozman What do you think?

@dgozman
Copy link
Contributor

dgozman commented Jan 14, 2025

We discussed this being an option of the html reporter, but that is not compatible with merge-reports workflow. There is no harm in populating git info without html - any custom reporter can use that data. In fact, json reporter already includes all of the metadata today. Since we do not have plugins in the public api, config option seems like the best solution.

I think we'd recommend to only enable it on CI anyway:

export default defineConfig({
  populateGitInfo: !!process.env.CI,
});

vitalets added a commit to vitalets/playwright that referenced this issue Jan 15, 2025
vitalets added a commit to vitalets/playwright that referenced this issue Jan 15, 2025
@vitalets
Copy link
Contributor Author

Created PR: #34329

@Skn0tt Skn0tt assigned vitalets and unassigned Skn0tt Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants