Skip to content

Conversation

@snomiao
Copy link
Member

@snomiao snomiao commented Sep 4, 2025

Summary

This PR fixes the issue where the PR comment bot incorrectly reports "All tests passed" when Playwright tests are skipped or don't run.

Fixes #5338

Problem

The PR comment workflow (pr-playwright-deploy.yaml) was checking for test failures by looking for deployment-info-* artifacts. However:

  1. The test-ui.yaml workflow wasn't creating these artifacts
  2. When no artifacts existed, the script defaulted to ALL_PASSED=true
  3. This resulted in false-positive "All tests passed" messages when tests were actually skipped

Solution

1. Modified test-ui.yaml

  • Added steps to capture Playwright test exit codes using steps.playwright.outcome
  • Creates deployment-info-* artifacts for each browser with format: browser|exit_code|url
  • Uploads these artifacts for the deploy workflow to consume

2. Modified pr-playwright-deploy.yaml

  • Added check to detect when no test results are available
  • Shows clear message "Tests were skipped or not run" instead of false success
  • Preserves existing logic for when test results are available

Test Plan

  • Verify deployment-info artifacts are created when tests run
  • Verify correct status (0 or 1) is captured based on test outcome
  • Verify PR comment shows "skipped" message when no test results exist
  • Verify PR comment shows correct pass/fail status when tests do run

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

This PR fixes the issue where the PR comment bot incorrectly reports "All tests passed" when Playwright tests are skipped or don't run.

Changes:
1. Modified test-ui.yaml to capture and upload test status as deployment-info artifacts
2. Modified pr-playwright-deploy.yaml to properly handle missing test results

The fix ensures that:
- Test exit codes are properly captured and passed between workflows
- When tests are skipped, the comment clearly states "Tests were skipped or not run"
- Each browser test result is tracked individually with proper status reporting

Fixes #5338

🤖 Generated with Claude Code
Co-Authored-By: Claude <[email protected]>
@github-actions
Copy link

github-actions bot commented Sep 4, 2025

🎭 Playwright Test Results

All tests passed across all browsers!

⏰ Completed at: 09/07/2025, 01:59:01 AM UTC

📊 Test Reports by Browser


🎉 Your tests are passing across all browsers!

@snomiao snomiao marked this pull request as ready for review September 5, 2025 17:38
@snomiao snomiao requested a review from a team as a code owner September 5, 2025 17:38
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 5, 2025
@snomiao
Copy link
Member Author

snomiao commented Sep 5, 2025

Just reviewed and LGTM found a problem, sharded tests may have to deploy to under multiple project, or merge the reports before deploy

@christian-byrne

The sharded chromium tests were generating deployment info only from shard 1, which could miss failures from other shards. This change moves the deployment info generation to the merge-reports job where we have visibility into the overall result of all shards.

Fixes #5338
@snomiao
Copy link
Member Author

snomiao commented Sep 10, 2025

@snomiao snomiao closed this Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Playwright PR commenter - false-positives

2 participants