-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add playwright test report on PR #26067
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
base: master
Are you sure you want to change the base?
Conversation
0a6c279 to
ec80899
Compare
Playwright test resultsDetails
Flaky testschromium › battery-settings-co |
ef3f6cb to
e58a39a
Compare
.github/workflows/default.yml
Outdated
| - uses: actions/checkout@v6 | ||
| with: | ||
| persist-credentials: false | ||
| repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }} |
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.
This is a security issue. Why is this needed for this feature?
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.
GH Action doesnt want to comment on the pr for some reason:
Unable to comment on PR
Unable to comment on your PR — this can happen for PR's originating from a fork without write permissions.
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.
Yes, but with this change people can run code from a fork within the permission boundaries of this project and get access to actions secrets.
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.
Well then the action will only work for people with write access to the original repo.
Agreed that this is not a good practice, but I dont know how else to go around this?
e58a39a to
e701bda
Compare
e701bda to
caec6c1
Compare
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.
Hey there - I've reviewed your changes - here's some feedback:
- Marking the Playwright test step with
continue-on-error: truewill let the workflow succeed even when tests fail; if you still want failures to block merges, consider movingcontinue-on-erroronly to the report/comment step or gating the job status based on the test result. - The Playwright config now always emits JSON reports (including for local runs); if the JSON is only needed in CI for the PR summary, consider making the JSON reporter conditional on
process.env.CIto avoid unnecessary artifacts locally. - The new permission comment still refers to "coverage reports" even though it's used for Playwright test summaries; updating the comment will keep the workflow configuration self-explanatory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Marking the Playwright test step with `continue-on-error: true` will let the workflow succeed even when tests fail; if you still want failures to block merges, consider moving `continue-on-error` only to the report/comment step or gating the job status based on the test result.
- The Playwright config now always emits JSON reports (including for local runs); if the JSON is only needed in CI for the PR summary, consider making the JSON reporter conditional on `process.env.CI` to avoid unnecessary artifacts locally.
- The new permission comment still refers to "coverage reports" even though it's used for Playwright test summaries; updating the comment will keep the workflow configuration self-explanatory.
## Individual Comments
### Comment 1
<location> `.github/workflows/default.yml:272-276` </location>
<code_context>
- name: Run tests
- run: npx playwright test
+ run: PLAYWRIGHT_JSON_OUTPUT_NAME=results.json npx playwright test
timeout-minutes: 20
env:
TZ: Europe/Berlin
+ continue-on-error: true
- uses: actions/upload-artifact@v5
</code_context>
<issue_to_address>
**issue (bug_risk):** Marking the Playwright test step as `continue-on-error` can let the workflow succeed even if tests fail.
Because this step (and the summary step) use `continue-on-error: true`, the workflow can pass even when Playwright tests fail, unless a later step explicitly checks the results and fails. If you want reports to always be generated but the job to still fail on test failures, consider keeping this step as a normal failing step and running reporting in a separate step with `if: always()` instead of using `continue-on-error` here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add report for playwright on PR comment