From df6723415b2e2177f7bfc53d90b8adbb0fcd4837 Mon Sep 17 00:00:00 2001 From: bymyself Date: Sun, 12 Oct 2025 16:00:38 -0700 Subject: [PATCH] Address review comments and improve workflow - Add workflow documentation explaining selective update strategy - Improve logging with clear output formatting (no emojis) - Add GitHub Actions workflow summary with file change details - Fix command injection vulnerability by validating test paths with regex - Add error handling for JSON.parse with descriptive messages - Replace non-null assertion with safer null checking pattern - Add explicit error handling for TypeScript script execution --- .github/workflows/tests-ci.yaml | 6 +- .../update-playwright-expectations.yaml | 107 ++++++++++++++++-- .../cicd/build-failed-screenshot-manifest.ts | 17 ++- 3 files changed, 115 insertions(+), 15 deletions(-) diff --git a/.github/workflows/tests-ci.yaml b/.github/workflows/tests-ci.yaml index ac49e551e..47b7ee712 100644 --- a/.github/workflows/tests-ci.yaml +++ b/.github/workflows/tests-ci.yaml @@ -256,7 +256,11 @@ jobs: if: ${{ needs.playwright-tests-chromium-sharded.result == 'failure' }} run: | set -euo pipefail - pnpm tsx scripts/cicd/build-failed-screenshot-manifest.ts + if ! pnpm tsx scripts/cicd/build-failed-screenshot-manifest.ts; then + echo "ERROR: Failed to generate screenshot manifest" + echo "This may indicate an issue with the Playwright JSON report or the manifest script" + exit 1 + fi working-directory: ComfyUI_frontend - name: Upload failed screenshot manifest diff --git a/.github/workflows/update-playwright-expectations.yaml b/.github/workflows/update-playwright-expectations.yaml index 5d08a3be0..ece83e390 100644 --- a/.github/workflows/update-playwright-expectations.yaml +++ b/.github/workflows/update-playwright-expectations.yaml @@ -1,4 +1,12 @@ # Setting test expectation screenshots for Playwright +# +# This workflow uses a selective snapshot update strategy: +# 1. When tests fail in CI, they generate a manifest of failed test locations (file:line) +# 2. This workflow downloads that manifest from the failed test run artifacts +# 3. Only the failed tests are re-run with --update-snapshots (much faster than running all tests) +# 4. Updated snapshots are committed back to the PR branch +# +# Trigger: Add label "New Browser Test Expectations" OR comment "/update-playwright" on PR name: Update Playwright Expectations on: @@ -97,39 +105,80 @@ jobs: continue-on-error: true run: | set -euo pipefail + + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + echo "Selective Snapshot Update" + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + echo "" + + # Check if manifest exists if [ ! -d ci-rerun ]; then - echo "No manifest found; running full suite as fallback" - PLAYWRIGHT_JSON_OUTPUT_NAME=playwright-report/report.json \ - pnpm exec playwright test --update-snapshots \ - --reporter=line --reporter=html - exit 0 + echo "ERROR: No manifest found in ci-rerun/ directory" + echo " This means no failed screenshot tests were detected in the latest CI run." + echo " Please ensure tests have been run and failures were recorded." + exit 1 fi + shopt -s nullglob files=(ci-rerun/*.txt) + if [ ${#files[@]} -eq 0 ]; then - echo "Manifest is empty; running full suite as fallback" - PLAYWRIGHT_JSON_OUTPUT_NAME=playwright-report/report.json \ - pnpm exec playwright test --update-snapshots \ - --reporter=line --reporter=html - exit 0 + echo "ERROR: No manifest files found in ci-rerun/" + echo " Expected files like: chromium.txt, chromium-2x.txt, mobile-chrome.txt" + exit 1 fi + + echo "Found ${#files[@]} project manifest(s):" + for f in "${files[@]}"; do + project="$(basename "$f" .txt)" + count=$(grep -c . "$f" 2>/dev/null || echo "0") + echo " - $project: $count failed test(s)" + done + echo "" + + # Re-run tests per project + total_tests=0 for f in "${files[@]}"; do project="$(basename "$f" .txt)" mapfile -t lines < "$f" filtered=( ) + + # Validate and sanitize test paths to prevent command injection for l in "${lines[@]}"; do - [ -n "$l" ] && filtered+=("$l") + # Skip empty lines + [ -z "$l" ] && continue + + # Validate format: must be browser_tests/...spec.ts:number + if [[ "$l" =~ ^browser_tests/.+\.spec\.ts:[0-9]+$ ]]; then + filtered+=("$l") + else + echo "WARNING: Skipping invalid test path: $l" + fi done + if [ ${#filtered[@]} -eq 0 ]; then + echo "WARNING: Skipping $project (no valid tests in manifest)" continue fi - echo "Re-running ${#filtered[@]} tests for project $project" + + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + echo "Updating snapshots for project: $project" + echo " Re-running ${#filtered[@]} failed test(s)..." + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + PLAYWRIGHT_JSON_OUTPUT_NAME=playwright-report/report.json \ pnpm exec playwright test --project="$project" --update-snapshots \ --reporter=line --reporter=html \ "${filtered[@]}" + + total_tests=$((total_tests + ${#filtered[@]})) + echo "" done + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + echo "Completed snapshot updates for $total_tests test(s)" + echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + - uses: actions/upload-artifact@v4 if: always() with: @@ -144,6 +193,7 @@ jobs: git status - name: Commit updated expectations + id: commit working-directory: ComfyUI_frontend run: | git config --global user.name 'github-actions' @@ -157,7 +207,13 @@ jobs: git add browser_tests if git diff --cached --quiet; then echo "No expectation updates detected; skipping commit." + echo "changed=false" >> $GITHUB_OUTPUT else + # Count changed snapshots + changed_count=$(git diff --cached --name-only browser_tests | wc -l) + echo "changed=true" >> $GITHUB_OUTPUT + echo "count=$changed_count" >> $GITHUB_OUTPUT + git commit -m "[automated] Update test expectations" if [ "${{ github.event_name }}" = "issue_comment" ]; then git push @@ -165,3 +221,30 @@ jobs: git push origin HEAD:${{ github.head_ref }} fi fi + + - name: Generate workflow summary + if: always() + working-directory: ComfyUI_frontend + run: | + echo "## Snapshot Update Summary" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + + if [ "${{ steps.commit.outputs.changed }}" = "true" ]; then + echo "**${{ steps.commit.outputs.count }} snapshot(s) updated**" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "
" >> $GITHUB_STEP_SUMMARY + echo "View updated files" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + git diff HEAD~1 --name-only browser_tests 2>/dev/null || echo "No git history available" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + echo "
" >> $GITHUB_STEP_SUMMARY + elif [ "${{ steps.commit.outputs.changed }}" = "false" ]; then + echo "No snapshot changes detected" >> $GITHUB_STEP_SUMMARY + else + echo "WARNING: Snapshot update may have failed - check logs above" >> $GITHUB_STEP_SUMMARY + fi + echo "" >> $GITHUB_STEP_SUMMARY + echo "---" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "**Strategy:** Selective snapshot update (only failed tests re-run)" >> $GITHUB_STEP_SUMMARY diff --git a/scripts/cicd/build-failed-screenshot-manifest.ts b/scripts/cicd/build-failed-screenshot-manifest.ts index 9ad7c9f83..38f42df64 100644 --- a/scripts/cicd/build-failed-screenshot-manifest.ts +++ b/scripts/cicd/build-failed-screenshot-manifest.ts @@ -28,7 +28,17 @@ async function main() { } const raw = await fsp.readFile(reportPath, 'utf8') - const data = JSON.parse(raw) + + let data: JSONReport + try { + data = JSON.parse(raw) + } catch (error) { + throw new Error( + `Failed to parse Playwright JSON report at ${reportPath}. ` + + `The report file may be corrupted or incomplete. ` + + `Error: ${error instanceof Error ? error.message : String(error)}` + ) + } const hasScreenshotSignal = (r: JSONReportTestResult) => { return r.attachments.some((att) => att?.contentType?.startsWith('image/')) @@ -52,7 +62,10 @@ async function main() { last && last.status === 'failed' && hasScreenshotSignal(last) if (!failedScreenshot) continue if (!out.has(project)) out.set(project, new Set()) - out.get(project)!.add(loc) + const projectSet = out.get(project) + if (projectSet) { + projectSet.add(loc) + } } } }