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: add new config prop populateGitInfo #34329

Merged
merged 11 commits into from
Jan 28, 2025

Conversation

vitalets
Copy link
Contributor

Fixes #34094

This comment has been minimized.

@vitalets
Copy link
Contributor Author

vitalets commented Jan 16, 2025

@yury-s thanks for the review.

  • described populateGitInfo property in doc
  • inverted dependency to GitCommitInfoPlugin

This comment has been minimized.

@vitalets vitalets requested a review from yury-s January 16, 2025 07:59
docs/src/test-api/class-fullconfig.md Outdated Show resolved Hide resolved
docs/src/test-api/class-testconfig.md Outdated Show resolved Hide resolved
* since: v1.50
- type: ?<[boolean]>

Whether to populate [`property: TestConfig.metadata`] with Git info.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also mention that the metadata will automatically appear in the html report, otherwise it's not clear what the use of the option?

packages/playwright/src/runner/runner.ts Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

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

Let's either update the new ui-mode* test or revert it completely, otherwise looks good.

packages/playwright/src/runner/runner.ts Show resolved Hide resolved

This comment has been minimized.

@vitalets
Copy link
Contributor Author

Hi @yury-s !

I've tried to update ui mode test to check git info in metadata, and I'm a bit stuck.
I've decided to check metadata via custom reporter, to not pull showReport() fixture for html reporter.
But there are two problems:

  1. console.log from custom reporter.ts does not go to testProcess.output without PWTEST_DEBUG=1, so can't assert it. I've used reporter.spec.ts for reference.
  2. Even with PWTEST_DEBUG=1 metadata does not contain git info. I've debugged as much as I can, gitCommitInfoPlugin definitely populates metadata, but it gets lost somewhere later.

PR is updated. Could you please assist on the issues?

This comment has been minimized.

@yury-s
Copy link
Member

yury-s commented Jan 24, 2025

  1. console.log from custom reporter.ts does not go to testProcess.output without PWTEST_DEBUG=1, so can't assert it. I've used reporter.spec.ts for reference.
  2. Even with PWTEST_DEBUG=1 metadata does not contain git info. I've debugged as much as I can, gitCommitInfoPlugin definitely populates metadata, but it gets lost somewhere later.

PR is updated. Could you please assist on the issues?

Thanks for adding the test! The output goes to the output panel in case of UI mode:
image
You can either open the panel in the UI mode runner and assert the content or just have the reporter write its output to a file and then assert that file's content.

@vitalets
Copy link
Contributor Author

@yury-s I've fixed the assertion, used output panel of the ui mode.

But point 2. is still relevant - metadata is not populated in ui mode for some reason:
image

I've logged GitCommitInfoPlugin output, it definitely populates config.metadata:

      Object.assign(config.metadata, info);
+     console.log('Git commit info - config.metadata:', config.metadata);

Output during the test:
image

Could you point the direction, how to fix it?

This comment has been minimized.

@yury-s
Copy link
Member

yury-s commented Jan 27, 2025

Could you point the direction, how to fix it?

c00e781 should fix it. In UI mode we run the plugins once in runGlobalSetup, but every call to _loadConfigOrReportError returns a new instance of FullConfigInternal, which resulted in new metadata instance being created every time. User config is cached between the calls as we use Node's require() to load it. Now that we always read the metadata object from userConfig.metadata, it should also be reused between _loadConfigOrReportError invocations.

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:206:5 › mobile viewport › view scale should reset after navigation @webkit-ubuntu-22.04-node18
❌ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
❌ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18

14 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @ubuntu-20.04-chromium-tip-of-tree
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:92:3 › should remove cookies by domain @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-device.spec.ts:45:5 › device › should scroll to click @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:97:5 › mobile viewport › should fire orientationchange event @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:116:5 › mobile viewport › default mobile viewports to 980 width @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/popup.spec.ts💯3 › should inherit touch support from browser context @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:44:14 › page screenshot › should work with a mobile viewport @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:55:14 › page screenshot › should work with a mobile viewport and clip @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:278:14 › element screenshot › should restore viewport after page screenshot and exception @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/video.spec.ts:441:5 › screencast › should work for popups @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37718 passed, 649 skipped
✔️✔️✔️

Merge workflow run.

@vitalets
Copy link
Contributor Author

c00e781 should fix it.

Yes, now it works! Thank you!
I've removed unnecessary comments, the PR is ready to get merged from my side.
There is one failing check [webkit - Node.js 18], but looks like it's unrelated to the PR changes.

@dgozman dgozman changed the title Add new config prop populateGitInfo feat: add new config prop populateGitInfo Jan 28, 2025
@dgozman dgozman merged commit 61d595a into microsoft:main Jan 28, 2025
28 of 29 checks passed
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.

[Docs]: Improve documentation for metadata
3 participants