From ec29b77819ba99199b11f3a5ac80076655038e71 Mon Sep 17 00:00:00 2001 From: snomiao Date: Thu, 4 Sep 2025 05:51:52 +0000 Subject: [PATCH] Fix Playwright PR commenter false-positives when tests are skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/pr-playwright-deploy.yaml | 113 ++++++++++++-------- .github/workflows/test-ui.yaml | 47 ++++++++ 2 files changed, 114 insertions(+), 46 deletions(-) diff --git a/.github/workflows/pr-playwright-deploy.yaml b/.github/workflows/pr-playwright-deploy.yaml index 12051fa990..a144619a13 100644 --- a/.github/workflows/pr-playwright-deploy.yaml +++ b/.github/workflows/pr-playwright-deploy.yaml @@ -212,61 +212,82 @@ jobs: echo "## 🎭 Playwright Test Results" >> comment.md echo "" >> comment.md - # Check if all tests passed - ALL_PASSED=true + # Check if we have any test results + FILES_FOUND=false for file in deployment-info/*.txt; do if [ -f "$file" ]; then - browser=$(basename "$file" .txt) - info=$(cat "$file") - exit_code=$(echo "$info" | cut -d'|' -f2) - if [ "$exit_code" != "0" ]; then - ALL_PASSED=false - break - fi + FILES_FOUND=true + break fi done - if [ "$ALL_PASSED" = "true" ]; then - echo "✅ **All tests passed across all browsers!**" >> comment.md + if [ "$FILES_FOUND" = "false" ]; then + # No test results found - tests were skipped or not run + echo "⏭️ **Tests were skipped or not run**" >> comment.md + echo "" >> comment.md + echo "⏰ Completed at: ${{ steps.completion-time.outputs.time }} UTC" >> comment.md + echo "" >> comment.md + echo "No test results available for this PR. This can happen when:" >> comment.md + echo "- Tests were skipped due to branch filters" >> comment.md + echo "- The test workflow didn't complete successfully" >> comment.md + echo "- Tests are still running" >> comment.md else - echo "❌ **Some tests failed!**" >> comment.md - fi - - echo "" >> comment.md - echo "⏰ Completed at: ${{ steps.completion-time.outputs.time }} UTC" >> comment.md - echo "" >> comment.md - echo "### 📊 Test Reports by Browser" >> comment.md - - for file in deployment-info/*.txt; do - if [ -f "$file" ]; then - browser=$(basename "$file" .txt) - info=$(cat "$file") - exit_code=$(echo "$info" | cut -d'|' -f2) - url=$(echo "$info" | cut -d'|' -f3) - - # Validate URLs before using them in comments - sanitized_url=$(echo "$url" | grep -E '^https://[a-z0-9.-]+\.pages\.dev(/.*)?$' || echo "INVALID_URL") - if [ "$sanitized_url" = "INVALID_URL" ]; then - echo "Invalid deployment URL detected: $url" - url="#" # Use safe fallback + # Check if all tests passed + ALL_PASSED=true + for file in deployment-info/*.txt; do + if [ -f "$file" ]; then + browser=$(basename "$file" .txt) + info=$(cat "$file") + exit_code=$(echo "$info" | cut -d'|' -f2) + if [ "$exit_code" != "0" ]; then + ALL_PASSED=false + break + fi fi - - if [ "$exit_code" = "0" ]; then - status="✅" - else - status="❌" - fi - - echo "- $status **$browser**: [View Report]($url)" >> comment.md + done + + if [ "$ALL_PASSED" = "true" ]; then + echo "✅ **All tests passed across all browsers!**" >> comment.md + else + echo "❌ **Some tests failed!**" >> comment.md fi - done - echo "" >> comment.md - echo "---" >> comment.md - if [ "$ALL_PASSED" = "true" ]; then - echo "🎉 Your tests are passing across all browsers!" >> comment.md - else - echo "⚠️ Please check the test reports for details on failures." >> comment.md + echo "" >> comment.md + echo "⏰ Completed at: ${{ steps.completion-time.outputs.time }} UTC" >> comment.md + echo "" >> comment.md + echo "### 📊 Test Reports by Browser" >> comment.md + + for file in deployment-info/*.txt; do + if [ -f "$file" ]; then + browser=$(basename "$file" .txt) + info=$(cat "$file") + exit_code=$(echo "$info" | cut -d'|' -f2) + url=$(echo "$info" | cut -d'|' -f3) + + # Validate URLs before using them in comments + sanitized_url=$(echo "$url" | grep -E '^https://[a-z0-9.-]+\.pages\.dev(/.*)?$' || echo "INVALID_URL") + if [ "$sanitized_url" = "INVALID_URL" ]; then + echo "Invalid deployment URL detected: $url" + url="#" # Use safe fallback + fi + + if [ "$exit_code" = "0" ]; then + status="✅" + else + status="❌" + fi + + echo "- $status **$browser**: [View Report]($url)" >> comment.md + fi + done + + echo "" >> comment.md + echo "---" >> comment.md + if [ "$ALL_PASSED" = "true" ]; then + echo "🎉 Your tests are passing across all browsers!" >> comment.md + else + echo "⚠️ Please check the test reports for details on failures." >> comment.md + fi fi - name: Comment PR - Tests Complete diff --git a/.github/workflows/test-ui.yaml b/.github/workflows/test-ui.yaml index be5c0cfc2b..fec0c7a0ea 100644 --- a/.github/workflows/test-ui.yaml +++ b/.github/workflows/test-ui.yaml @@ -140,6 +140,27 @@ jobs: path: blob-report/ retention-days: 1 + # Save test result for deployment info (only on shard 1 to avoid duplicates) + - name: Save test result for chromium + if: always() && matrix.shardIndex == 1 + run: | + # Use outcome to determine exit code (0 for success, 1 for failure/skipped) + if [ "${{ steps.playwright.outcome }}" = "success" ]; then + EXIT_CODE=0 + else + EXIT_CODE=1 + fi + # For sharded tests, we use the main chromium project name + echo "chromium|${EXIT_CODE}|https://comfyui-playwright-chromium.pages.dev" > deployment-info-chromium.txt + working-directory: ComfyUI_frontend + + - uses: actions/upload-artifact@v4 + if: always() && matrix.shardIndex == 1 + with: + name: deployment-info-chromium + path: ComfyUI_frontend/deployment-info-chromium.txt + retention-days: 1 + playwright-tests: # Ideally, each shard runs test in 6 minutes, but allow up to 15 minutes timeout-minutes: 15 @@ -204,6 +225,32 @@ jobs: path: ComfyUI_frontend/playwright-report/ retention-days: 30 + # Save test result for deployment info + - name: Save test result for ${{ matrix.browser }} + if: always() + run: | + # Use outcome to determine exit code (0 for success, 1 for failure/skipped) + if [ "${{ steps.playwright.outcome }}" = "success" ]; then + EXIT_CODE=0 + else + EXIT_CODE=1 + fi + # Generate project name based on browser + if [ "${{ matrix.browser }}" = "chromium-0.5x" ]; then + PROJECT_NAME="comfyui-playwright-chromium-0-5x" + else + PROJECT_NAME="comfyui-playwright-${{ matrix.browser }}" + fi + echo "${{ matrix.browser }}|${EXIT_CODE}|https://${PROJECT_NAME}.pages.dev" > deployment-info-${{ matrix.browser }}.txt + working-directory: ComfyUI_frontend + + - uses: actions/upload-artifact@v4 + if: always() + with: + name: deployment-info-${{ matrix.browser }} + path: ComfyUI_frontend/deployment-info-${{ matrix.browser }}.txt + retention-days: 1 + # Merge sharded test reports merge-reports: needs: [playwright-tests-chromium-sharded]