From 51f20263ac3720135ee802032668fc2d73f40b97 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 11:45:01 -0700 Subject: [PATCH 1/3] fix: enforce trustworthy quality snapshots (Fixes #503) Pin comparisons to the exact PR base, fail invalid or regressed performance and coverage snapshots, and harden the E2E JSONRPC client against buffered-output loss and stderr backpressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/coverage-baseline.yml | 18 ++ .github/workflows/coverage.yml | 195 +++--------- .github/workflows/perf-baseline.yml | 39 ++- .github/workflows/perf-tests.yml | 395 ++++-------------------- crates/pet/tests/e2e_performance.rs | 178 ++++++++--- docs/QUALITY_SNAPSHOTS.md | 50 +++ scripts/quality_snapshot.py | 345 +++++++++++++++++++++ scripts/tests/test_quality_snapshot.py | 212 +++++++++++++ 8 files changed, 880 insertions(+), 552 deletions(-) create mode 100644 docs/QUALITY_SNAPSHOTS.md create mode 100644 scripts/quality_snapshot.py create mode 100644 scripts/tests/test_quality_snapshot.py diff --git a/.github/workflows/coverage-baseline.yml b/.github/workflows/coverage-baseline.yml index 2ca3b031..df758b71 100644 --- a/.github/workflows/coverage-baseline.yml +++ b/.github/workflows/coverage-baseline.yml @@ -4,6 +4,10 @@ on: push: branches: - main + - release* + - release/* + - release-* + workflow_dispatch: permissions: contents: read @@ -29,6 +33,10 @@ jobs: with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -175,6 +183,16 @@ jobs: RUST_LOG: trace shell: bash + - name: Validate Coverage Baseline + run: >- + python scripts/quality_snapshot.py coverage + --current lcov.info + --baseline lcov.info + --platform "${{ matrix.os }} baseline" + --report coverage-baseline-report.md + --summary "$GITHUB_STEP_SUMMARY" + shell: bash + - name: Upload Coverage Artifact uses: actions/upload-artifact@v4 with: diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index f8019a6d..d20c7dc8 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -23,41 +23,36 @@ jobs: include: - os: ubuntu-latest target: x86_64-unknown-linux-musl + platform: Linux + comment_header: coverage-linux - os: windows-latest target: x86_64-pc-windows-msvc + platform: Windows + comment_header: coverage-windows steps: - name: Checkout uses: actions/checkout@v4 - - name: Post Coverage Started Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') + - name: Post Coverage Started Comment uses: marocchino/sticky-pull-request-comment@v2 with: - header: coverage-linux + header: ${{ matrix.comment_header }} message: | - ## Test Coverage Report (Linux) + ## Test Coverage Report (${{ matrix.platform }}) :hourglass_flowing_sand: **Coverage analysis in progress...** - This comment will be updated with results when the analysis completes. - - - name: Post Coverage Started Comment (Windows) - if: startsWith(matrix.os, 'windows') - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: coverage-windows - message: | - ## Test Coverage Report (Windows) - - :hourglass_flowing_sand: **Coverage analysis in progress...** - - This comment will be updated with results when the analysis completes. + Comparing against exact base `${{ github.event.pull_request.base.sha }}`. - name: Set Python to PATH uses: actions/setup-python@v5 with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -198,6 +193,7 @@ jobs: shell: bash - name: Run Tests with Coverage + id: coverage run: cargo llvm-cov --features ci --lcov --output-path lcov.info -- --nocapture --test-threads=1 env: RUST_BACKTRACE: 1 @@ -205,160 +201,39 @@ jobs: shell: bash - name: Upload PR Coverage Artifact + if: always() uses: actions/upload-artifact@v4 with: name: coverage-pr-${{ matrix.os }} path: lcov.info + if-no-files-found: ignore - - name: Download Baseline Coverage + - name: Download Exact PR Base Coverage + if: always() uses: dawidd6/action-download-artifact@v6 - id: download-baseline - continue-on-error: true with: workflow: coverage-baseline.yml - branch: main + commit: ${{ github.event.pull_request.base.sha }} + workflow_conclusion: success name: coverage-baseline-${{ matrix.os }} path: baseline-coverage - - - name: Install lcov (Linux) - if: startsWith(matrix.os, 'ubuntu') - run: sudo apt-get update && sudo apt-get install -y lcov - - - name: Install lcov (Windows) - if: startsWith(matrix.os, 'windows') - run: choco install lcov -y + check_artifacts: true + search_artifacts: true + + - name: Compare Coverage Snapshot + if: always() + run: >- + python scripts/quality_snapshot.py coverage + --current lcov.info + --baseline baseline-coverage/lcov.info + --platform "${{ matrix.platform }}" + --report coverage-report.md + --summary "$GITHUB_STEP_SUMMARY" shell: bash - - name: Generate Coverage Report (Linux) - if: startsWith(matrix.os, 'ubuntu') - id: coverage-linux - run: | - # Extract PR coverage - PR_LINES=$(lcov --summary lcov.info 2>&1 | grep "lines" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - PR_FUNCTIONS=$(lcov --summary lcov.info 2>&1 | grep "functions" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - - # Extract baseline coverage (default to 0 if not available) - if [ -f baseline-coverage/lcov.info ]; then - BASELINE_LINES=$(lcov --summary baseline-coverage/lcov.info 2>&1 | grep "lines" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - BASELINE_FUNCTIONS=$(lcov --summary baseline-coverage/lcov.info 2>&1 | grep "functions" | sed 's/.*: //' | sed 's/%.*//' | tr -d ' ') - else - BASELINE_LINES="0" - BASELINE_FUNCTIONS="0" - fi - - # Calculate diff - LINE_DIFF=$(echo "$PR_LINES - $BASELINE_LINES" | bc) - FUNC_DIFF=$(echo "$PR_FUNCTIONS - $BASELINE_FUNCTIONS" | bc) - - # Determine delta indicator - if (( $(echo "$LINE_DIFF > 0" | bc -l) )); then - DELTA_INDICATOR=":white_check_mark:" - elif (( $(echo "$LINE_DIFF < 0" | bc -l) )); then - DELTA_INDICATOR=":x:" - else - DELTA_INDICATOR=":heavy_minus_sign:" - fi - - # Set outputs - echo "pr_lines=$PR_LINES" >> $GITHUB_OUTPUT - echo "baseline_lines=$BASELINE_LINES" >> $GITHUB_OUTPUT - echo "line_diff=$LINE_DIFF" >> $GITHUB_OUTPUT - echo "delta_indicator=$DELTA_INDICATOR" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Test Coverage Report (${{ matrix.os }})" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | Value |" >> $GITHUB_STEP_SUMMARY - echo "|--------|-------|" >> $GITHUB_STEP_SUMMARY - echo "| Current Coverage | ${PR_LINES}% |" >> $GITHUB_STEP_SUMMARY - echo "| Base Branch Coverage | ${BASELINE_LINES}% |" >> $GITHUB_STEP_SUMMARY - echo "| Delta | ${LINE_DIFF}% ${DELTA_INDICATOR} |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Generate Coverage Report (Windows) - if: startsWith(matrix.os, 'windows') - id: coverage-windows - run: | - # Extract PR coverage - $prContent = Get-Content -Path "lcov.info" -Raw - $prLinesFound = ($prContent | Select-String -Pattern "LF:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - $prLinesHit = ($prContent | Select-String -Pattern "LH:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - if ($prLinesFound -gt 0) { - $prPct = [math]::Round(($prLinesHit / $prLinesFound) * 100, 2) - } else { - $prPct = 0 - } - - # Extract baseline coverage (default to 0 if not available) - if (Test-Path "baseline-coverage/lcov.info") { - $baselineContent = Get-Content -Path "baseline-coverage/lcov.info" -Raw - $baselineLinesFound = ($baselineContent | Select-String -Pattern "LF:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - $baselineLinesHit = ($baselineContent | Select-String -Pattern "LH:(\d+)" -AllMatches).Matches | ForEach-Object { [int]$_.Groups[1].Value } | Measure-Object -Sum | Select-Object -ExpandProperty Sum - if ($baselineLinesFound -gt 0) { - $baselinePct = [math]::Round(($baselineLinesHit / $baselineLinesFound) * 100, 2) - } else { - $baselinePct = 0 - } - } else { - $baselinePct = 0 - } - - $diff = [math]::Round($prPct - $baselinePct, 2) - - if ($diff -gt 0) { - $deltaIndicator = ":white_check_mark:" - } elseif ($diff -lt 0) { - $deltaIndicator = ":x:" - } else { - $deltaIndicator = ":heavy_minus_sign:" - } - - # Set outputs - echo "pr_lines=$prPct" >> $env:GITHUB_OUTPUT - echo "baseline_lines=$baselinePct" >> $env:GITHUB_OUTPUT - echo "line_diff=$diff" >> $env:GITHUB_OUTPUT - echo "delta_indicator=$deltaIndicator" >> $env:GITHUB_OUTPUT - - # Write step summary - echo "## Test Coverage Report (${{ matrix.os }})" >> $env:GITHUB_STEP_SUMMARY - echo "" >> $env:GITHUB_STEP_SUMMARY - echo "| Metric | Value |" >> $env:GITHUB_STEP_SUMMARY - echo "|--------|-------|" >> $env:GITHUB_STEP_SUMMARY - echo "| Current Coverage | ${prPct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Base Branch Coverage | ${baselinePct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Delta | ${diff}% ${deltaIndicator} |" >> $env:GITHUB_STEP_SUMMARY - shell: pwsh - - - name: Post Coverage Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') + - name: Post Coverage Comment + if: always() uses: marocchino/sticky-pull-request-comment@v2 with: - header: coverage-linux - message: | - ## Test Coverage Report (Linux) - - | Metric | Value | - |--------|-------| - | Current Coverage | ${{ steps.coverage-linux.outputs.pr_lines }}% | - | Base Branch Coverage | ${{ steps.coverage-linux.outputs.baseline_lines }}% | - | Delta | ${{ steps.coverage-linux.outputs.line_diff }}% ${{ steps.coverage-linux.outputs.delta_indicator }} | - - --- - ${{ steps.coverage-linux.outputs.line_diff > 0 && 'Coverage increased! Great work!' || (steps.coverage-linux.outputs.line_diff < 0 && 'Coverage decreased. Please add tests for new code.' || 'Coverage unchanged.') }} - - - name: Post Coverage Comment (Windows) - if: startsWith(matrix.os, 'windows') - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: coverage-windows - message: | - ## Test Coverage Report (Windows) - - | Metric | Value | - |--------|-------| - | Current Coverage | ${{ steps.coverage-windows.outputs.pr_lines }}% | - | Base Branch Coverage | ${{ steps.coverage-windows.outputs.baseline_lines }}% | - | Delta | ${{ steps.coverage-windows.outputs.line_diff }}% ${{ steps.coverage-windows.outputs.delta_indicator }} | - - --- - ${{ steps.coverage-windows.outputs.line_diff > 0 && 'Coverage increased! Great work!' || (steps.coverage-windows.outputs.line_diff < 0 && 'Coverage decreased. Please add tests for new code.' || 'Coverage unchanged.') }} + header: ${{ matrix.comment_header }} + path: coverage-report.md diff --git a/.github/workflows/perf-baseline.yml b/.github/workflows/perf-baseline.yml index efe5b405..ed5dfcbe 100644 --- a/.github/workflows/perf-baseline.yml +++ b/.github/workflows/perf-baseline.yml @@ -4,6 +4,10 @@ on: push: branches: - main + - release* + - release/* + - release-* + workflow_dispatch: permissions: contents: read @@ -31,6 +35,10 @@ jobs: with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -73,26 +81,33 @@ jobs: shell: bash - name: Run Performance Tests - continue-on-error: true - run: cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt + run: | + set -o pipefail + cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt env: RUST_BACKTRACE: 1 RUST_LOG: warn shell: bash - name: Extract Performance Metrics - id: metrics run: | - # Extract JSON metrics from test output - if grep -q "JSON metrics:" perf-output.txt; then - # Extract lines after "JSON metrics:" until the closing brace - sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json - echo "Metrics extracted:" - cat metrics.json - else - echo '{"server_startup_ms": 0, "full_refresh_ms": 0, "environments_count": 0}' > metrics.json - echo "No metrics found, created empty metrics" + if ! grep -q "JSON metrics:" perf-output.txt; then + echo "Performance baseline produced no JSON metrics" >&2 + exit 1 fi + sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json + python -m json.tool metrics.json > /dev/null + cat metrics.json + shell: bash + + - name: Validate Performance Baseline + run: >- + python scripts/quality_snapshot.py performance + --current metrics.json + --baseline metrics.json + --platform "${{ matrix.os }} baseline" + --report performance-baseline-report.md + --summary "$GITHUB_STEP_SUMMARY" shell: bash - name: Upload Performance Baseline Artifact diff --git a/.github/workflows/perf-tests.yml b/.github/workflows/perf-tests.yml index 89ea789e..9f5dda5a 100644 --- a/.github/workflows/perf-tests.yml +++ b/.github/workflows/perf-tests.yml @@ -24,49 +24,39 @@ jobs: include: - os: windows-latest target: x86_64-pc-windows-msvc + platform: Windows + comment_header: perf-windows - os: ubuntu-latest target: x86_64-unknown-linux-musl + platform: Linux + comment_header: perf-linux - os: macos-latest target: x86_64-apple-darwin + platform: macOS + comment_header: perf-macos steps: - name: Checkout uses: actions/checkout@v4 - - name: Post In-Progress Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') && github.event_name == 'pull_request' + - name: Post In-Progress Comment + if: github.event_name == 'pull_request' uses: marocchino/sticky-pull-request-comment@v2 with: - header: perf-linux + header: ${{ matrix.comment_header }} message: | - ## Performance Report (Linux) :hourglass_flowing_sand: + ## Performance Report (${{ matrix.platform }}) :hourglass_flowing_sand: - Running performance tests... Results will appear here when complete. - - - name: Post In-Progress Comment (Windows) - if: startsWith(matrix.os, 'windows') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-windows - message: | - ## Performance Report (Windows) :hourglass_flowing_sand: - - Running performance tests... Results will appear here when complete. - - - name: Post In-Progress Comment (macOS) - if: startsWith(matrix.os, 'macos') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-macos - message: | - ## Performance Report (macOS) :hourglass_flowing_sand: - - Running performance tests... Results will appear here when complete. + Running performance tests against baseline `${{ github.event.pull_request.base.sha }}`. - name: Set Python to PATH uses: actions/setup-python@v5 with: python-version: "3.12" + - name: Validate Snapshot Comparator + run: python -m unittest discover -s scripts/tests -p 'test_*.py' -v + shell: bash + - name: Add Conda to PATH (Windows) if: startsWith(matrix.os, 'windows') run: | @@ -109,340 +99,73 @@ jobs: shell: bash - name: Run Performance Tests - continue-on-error: true - run: cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt + id: benchmark + run: | + set -o pipefail + cargo test --release --features ci-perf --target ${{ matrix.target }} --test e2e_performance test_performance_summary -- --nocapture 2>&1 | tee perf-output.txt env: RUST_BACKTRACE: 1 RUST_LOG: warn shell: bash - name: Extract Performance Metrics - id: metrics + if: steps.benchmark.outcome == 'success' run: | - # Extract JSON metrics from test output - if grep -q "JSON metrics:" perf-output.txt; then - # Extract lines after "JSON metrics:" until the closing brace - sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json - echo "Metrics extracted:" - cat metrics.json - else - echo '{"server_startup_ms": 0, "full_refresh_ms": 0, "environments_count": 0}' > metrics.json - echo "No metrics found, created empty metrics" + if ! grep -q "JSON metrics:" perf-output.txt; then + echo "Performance test produced no JSON metrics" >&2 + exit 1 fi + sed -n '/JSON metrics:/,/^}/p' perf-output.txt | tail -n +2 > metrics.json + python -m json.tool metrics.json > /dev/null + cat metrics.json shell: bash - name: Upload PR Performance Results + if: always() uses: actions/upload-artifact@v4 with: name: perf-pr-${{ matrix.os }} path: metrics.json + if-no-files-found: ignore - - name: Download Baseline Performance + - name: Download Exact PR Base Performance + if: always() && github.event_name == 'pull_request' uses: dawidd6/action-download-artifact@v6 - id: download-baseline - continue-on-error: true with: workflow: perf-baseline.yml - branch: main + commit: ${{ github.event.pull_request.base.sha }} + workflow_conclusion: success name: perf-baseline-${{ matrix.os }} path: baseline-perf + check_artifacts: true + search_artifacts: true - - name: Generate Performance Report (Linux) - if: startsWith(matrix.os, 'ubuntu') - id: perf-linux - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - PR_STARTUP=$(jq -r '.server_startup_ms // 0' metrics.json) - PR_REFRESH=$(jq -r '.full_refresh_ms // 0' metrics.json) - PR_ENVS=$(jq -r '.environments_count // 0' metrics.json) - - # Extract P95 values from stats object (if available) - PR_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' metrics.json) - PR_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' metrics.json) - - # Extract baseline metrics (default to 0 if not available) - if [ -f baseline-perf/metrics.json ]; then - BASELINE_STARTUP=$(jq -r '.server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH=$(jq -r '.full_refresh_ms // 0' baseline-perf/metrics.json) - BASELINE_ENVS=$(jq -r '.environments_count // 0' baseline-perf/metrics.json) - BASELINE_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' baseline-perf/metrics.json) - else - BASELINE_STARTUP=0 - BASELINE_REFRESH=0 - BASELINE_ENVS=0 - BASELINE_STARTUP_P95=0 - BASELINE_REFRESH_P95=0 - fi - - # Calculate diff (positive means slowdown, negative means speedup) - STARTUP_DIFF=$(echo "$PR_STARTUP - $BASELINE_STARTUP" | bc) - REFRESH_DIFF=$(echo "$PR_REFRESH - $BASELINE_REFRESH" | bc) - - # Calculate percentage change - if [ "$BASELINE_STARTUP" != "0" ]; then - STARTUP_PCT=$(echo "scale=1; ($STARTUP_DIFF / $BASELINE_STARTUP) * 100" | bc) - else - STARTUP_PCT="N/A" - fi - - if [ "$BASELINE_REFRESH" != "0" ]; then - REFRESH_PCT=$(echo "scale=1; ($REFRESH_DIFF / $BASELINE_REFRESH) * 100" | bc) - else - REFRESH_PCT="N/A" - fi - - # Determine delta indicators (for perf, negative is good = faster) - if (( $(echo "$REFRESH_DIFF < -100" | bc -l) )); then - DELTA_INDICATOR=":rocket:" - elif (( $(echo "$REFRESH_DIFF < 0" | bc -l) )); then - DELTA_INDICATOR=":white_check_mark:" - elif (( $(echo "$REFRESH_DIFF > 500" | bc -l) )); then - DELTA_INDICATOR=":warning:" - elif (( $(echo "$REFRESH_DIFF > 100" | bc -l) )); then - DELTA_INDICATOR=":small_red_triangle:" - else - DELTA_INDICATOR=":heavy_minus_sign:" - fi - - # Set outputs - echo "pr_startup=$PR_STARTUP" >> $GITHUB_OUTPUT - echo "pr_refresh=$PR_REFRESH" >> $GITHUB_OUTPUT - echo "pr_startup_p95=$PR_STARTUP_P95" >> $GITHUB_OUTPUT - echo "pr_refresh_p95=$PR_REFRESH_P95" >> $GITHUB_OUTPUT - echo "baseline_startup=$BASELINE_STARTUP" >> $GITHUB_OUTPUT - echo "baseline_refresh=$BASELINE_REFRESH" >> $GITHUB_OUTPUT - echo "baseline_startup_p95=$BASELINE_STARTUP_P95" >> $GITHUB_OUTPUT - echo "baseline_refresh_p95=$BASELINE_REFRESH_P95" >> $GITHUB_OUTPUT - echo "startup_diff=$STARTUP_DIFF" >> $GITHUB_OUTPUT - echo "refresh_diff=$REFRESH_DIFF" >> $GITHUB_OUTPUT - echo "startup_pct=$STARTUP_PCT" >> $GITHUB_OUTPUT - echo "refresh_pct=$REFRESH_PCT" >> $GITHUB_OUTPUT - echo "delta_indicator=$DELTA_INDICATOR" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (Linux)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change |" >> $GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|--------|" >> $GITHUB_STEP_SUMMARY - echo "| Server Startup | ${PR_STARTUP}ms | ${PR_STARTUP_P95}ms | ${BASELINE_STARTUP}ms | ${STARTUP_DIFF}ms | ${STARTUP_PCT}% |" >> $GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${PR_REFRESH}ms | ${PR_REFRESH_P95}ms | ${BASELINE_REFRESH}ms | ${REFRESH_DIFF}ms | ${REFRESH_PCT}% ${DELTA_INDICATOR} |" >> $GITHUB_STEP_SUMMARY - echo "| Environments | ${PR_ENVS} | - | ${BASELINE_ENVS} | - | - |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Generate Performance Report (Windows) - if: startsWith(matrix.os, 'windows') - id: perf-windows - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - $prMetrics = Get-Content -Path "metrics.json" -Raw | ConvertFrom-Json - $prStartup = $prMetrics.server_startup_ms - $prRefresh = $prMetrics.full_refresh_ms - $prEnvs = $prMetrics.environments_count - - # Extract P95 values from stats object (if available) - $prStartupP95 = if ($prMetrics.stats.server_startup.p95) { $prMetrics.stats.server_startup.p95 } else { $prStartup } - $prRefreshP95 = if ($prMetrics.stats.full_refresh.p95) { $prMetrics.stats.full_refresh.p95 } else { $prRefresh } - - # Extract baseline metrics (default to 0 if not available) - if (Test-Path "baseline-perf/metrics.json") { - $baselineMetrics = Get-Content -Path "baseline-perf/metrics.json" -Raw | ConvertFrom-Json - $baselineStartup = $baselineMetrics.server_startup_ms - $baselineRefresh = $baselineMetrics.full_refresh_ms - $baselineEnvs = $baselineMetrics.environments_count - $baselineStartupP95 = if ($baselineMetrics.stats.server_startup.p95) { $baselineMetrics.stats.server_startup.p95 } else { $baselineStartup } - $baselineRefreshP95 = if ($baselineMetrics.stats.full_refresh.p95) { $baselineMetrics.stats.full_refresh.p95 } else { $baselineRefresh } - } else { - $baselineStartup = 0 - $baselineRefresh = 0 - $baselineEnvs = 0 - $baselineStartupP95 = 0 - $baselineRefreshP95 = 0 - } - - # Calculate diff - $startupDiff = $prStartup - $baselineStartup - $refreshDiff = $prRefresh - $baselineRefresh - - # Calculate percentage change - if ($baselineStartup -gt 0) { - $startupPct = [math]::Round(($startupDiff / $baselineStartup) * 100, 1) - } else { - $startupPct = "N/A" - } - - if ($baselineRefresh -gt 0) { - $refreshPct = [math]::Round(($refreshDiff / $baselineRefresh) * 100, 1) - } else { - $refreshPct = "N/A" - } - - # Determine delta indicator - if ($refreshDiff -lt -100) { - $deltaIndicator = ":rocket:" - } elseif ($refreshDiff -lt 0) { - $deltaIndicator = ":white_check_mark:" - } elseif ($refreshDiff -gt 500) { - $deltaIndicator = ":warning:" - } elseif ($refreshDiff -gt 100) { - $deltaIndicator = ":small_red_triangle:" - } else { - $deltaIndicator = ":heavy_minus_sign:" - } - - # Set outputs - echo "pr_startup=$prStartup" >> $env:GITHUB_OUTPUT - echo "pr_refresh=$prRefresh" >> $env:GITHUB_OUTPUT - echo "pr_startup_p95=$prStartupP95" >> $env:GITHUB_OUTPUT - echo "pr_refresh_p95=$prRefreshP95" >> $env:GITHUB_OUTPUT - echo "baseline_startup=$baselineStartup" >> $env:GITHUB_OUTPUT - echo "baseline_refresh=$baselineRefresh" >> $env:GITHUB_OUTPUT - echo "baseline_startup_p95=$baselineStartupP95" >> $env:GITHUB_OUTPUT - echo "baseline_refresh_p95=$baselineRefreshP95" >> $env:GITHUB_OUTPUT - echo "startup_diff=$startupDiff" >> $env:GITHUB_OUTPUT - echo "refresh_diff=$refreshDiff" >> $env:GITHUB_OUTPUT - echo "startup_pct=$startupPct" >> $env:GITHUB_OUTPUT - echo "refresh_pct=$refreshPct" >> $env:GITHUB_OUTPUT - echo "delta_indicator=$deltaIndicator" >> $env:GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (Windows)" >> $env:GITHUB_STEP_SUMMARY - echo "" >> $env:GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change |" >> $env:GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|--------|" >> $env:GITHUB_STEP_SUMMARY - echo "| Server Startup | ${prStartup}ms | ${prStartupP95}ms | ${baselineStartup}ms | ${startupDiff}ms | ${startupPct}% |" >> $env:GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${prRefresh}ms | ${prRefreshP95}ms | ${baselineRefresh}ms | ${refreshDiff}ms | ${refreshPct}% ${deltaIndicator} |" >> $env:GITHUB_STEP_SUMMARY - echo "| Environments | ${prEnvs} | - | ${baselineEnvs} | - | - |" >> $env:GITHUB_STEP_SUMMARY - shell: pwsh - - - name: Generate Performance Report (macOS) - if: startsWith(matrix.os, 'macos') - id: perf-macos - run: | - # Extract PR metrics (P50 values at top level for backwards compatibility) - PR_STARTUP=$(jq -r '.server_startup_ms // 0' metrics.json) - PR_REFRESH=$(jq -r '.full_refresh_ms // 0' metrics.json) - PR_ENVS=$(jq -r '.environments_count // 0' metrics.json) - - # Extract P95 values from stats object (if available) - PR_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' metrics.json) - PR_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' metrics.json) - - # Extract baseline metrics (default to 0 if not available) - if [ -f baseline-perf/metrics.json ]; then - BASELINE_STARTUP=$(jq -r '.server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH=$(jq -r '.full_refresh_ms // 0' baseline-perf/metrics.json) - BASELINE_ENVS=$(jq -r '.environments_count // 0' baseline-perf/metrics.json) - BASELINE_STARTUP_P95=$(jq -r '.stats.server_startup.p95 // .server_startup_ms // 0' baseline-perf/metrics.json) - BASELINE_REFRESH_P95=$(jq -r '.stats.full_refresh.p95 // .full_refresh_ms // 0' baseline-perf/metrics.json) - else - BASELINE_STARTUP=0 - BASELINE_REFRESH=0 - BASELINE_ENVS=0 - BASELINE_STARTUP_P95=0 - BASELINE_REFRESH_P95=0 - fi - - # Calculate diff - STARTUP_DIFF=$((PR_STARTUP - BASELINE_STARTUP)) - REFRESH_DIFF=$((PR_REFRESH - BASELINE_REFRESH)) - - # Set outputs - echo "pr_startup=$PR_STARTUP" >> $GITHUB_OUTPUT - echo "pr_refresh=$PR_REFRESH" >> $GITHUB_OUTPUT - echo "pr_startup_p95=$PR_STARTUP_P95" >> $GITHUB_OUTPUT - echo "pr_refresh_p95=$PR_REFRESH_P95" >> $GITHUB_OUTPUT - echo "baseline_startup=$BASELINE_STARTUP" >> $GITHUB_OUTPUT - echo "baseline_refresh=$BASELINE_REFRESH" >> $GITHUB_OUTPUT - echo "baseline_startup_p95=$BASELINE_STARTUP_P95" >> $GITHUB_OUTPUT - echo "baseline_refresh_p95=$BASELINE_REFRESH_P95" >> $GITHUB_OUTPUT - echo "startup_diff=$STARTUP_DIFF" >> $GITHUB_OUTPUT - echo "refresh_diff=$REFRESH_DIFF" >> $GITHUB_OUTPUT - - # Write step summary - echo "## Performance Report (macOS)" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "| Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta |" >> $GITHUB_STEP_SUMMARY - echo "|--------|----------|----------|----------------|-------|" >> $GITHUB_STEP_SUMMARY - echo "| Server Startup | ${PR_STARTUP}ms | ${PR_STARTUP_P95}ms | ${BASELINE_STARTUP}ms | ${STARTUP_DIFF}ms |" >> $GITHUB_STEP_SUMMARY - echo "| Full Refresh | ${PR_REFRESH}ms | ${PR_REFRESH_P95}ms | ${BASELINE_REFRESH}ms | ${REFRESH_DIFF}ms |" >> $GITHUB_STEP_SUMMARY - echo "| Environments | ${PR_ENVS} | - | ${BASELINE_ENVS} | - |" >> $GITHUB_STEP_SUMMARY - shell: bash - - - name: Post Performance Comment (Linux) - if: startsWith(matrix.os, 'ubuntu') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 - with: - header: perf-linux - message: | - ## Performance Report (Linux) ${{ steps.perf-linux.outputs.delta_indicator }} - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change | - |--------|----------|----------|----------------|-------|--------| - | Server Startup | ${{ steps.perf-linux.outputs.pr_startup }}ms | ${{ steps.perf-linux.outputs.pr_startup_p95 }}ms | ${{ steps.perf-linux.outputs.baseline_startup }}ms | ${{ steps.perf-linux.outputs.startup_diff }}ms | ${{ steps.perf-linux.outputs.startup_pct }}% | - | Full Refresh | ${{ steps.perf-linux.outputs.pr_refresh }}ms | ${{ steps.perf-linux.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-linux.outputs.baseline_refresh }}ms | ${{ steps.perf-linux.outputs.refresh_diff }}ms | ${{ steps.perf-linux.outputs.refresh_pct }}% | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
- - - name: Post Performance Comment (Windows) - if: startsWith(matrix.os, 'windows') && github.event_name == 'pull_request' - uses: marocchino/sticky-pull-request-comment@v2 + - name: Download Main Performance for Manual Run + if: always() && github.event_name == 'workflow_dispatch' + uses: dawidd6/action-download-artifact@v6 with: - header: perf-windows - message: | - ## Performance Report (Windows) ${{ steps.perf-windows.outputs.delta_indicator }} - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | Change | - |--------|----------|----------|----------------|-------|--------| - | Server Startup | ${{ steps.perf-windows.outputs.pr_startup }}ms | ${{ steps.perf-windows.outputs.pr_startup_p95 }}ms | ${{ steps.perf-windows.outputs.baseline_startup }}ms | ${{ steps.perf-windows.outputs.startup_diff }}ms | ${{ steps.perf-windows.outputs.startup_pct }}% | - | Full Refresh | ${{ steps.perf-windows.outputs.pr_refresh }}ms | ${{ steps.perf-windows.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-windows.outputs.baseline_refresh }}ms | ${{ steps.perf-windows.outputs.refresh_diff }}ms | ${{ steps.perf-windows.outputs.refresh_pct }}% | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
+ workflow: perf-baseline.yml + branch: main + workflow_conclusion: success + name: perf-baseline-${{ matrix.os }} + path: baseline-perf + check_artifacts: true + search_artifacts: true + + - name: Compare Performance Snapshot + if: always() + run: >- + python scripts/quality_snapshot.py performance + --current metrics.json + --baseline baseline-perf/metrics.json + --platform "${{ matrix.platform }}" + --report performance-report.md + --summary "$GITHUB_STEP_SUMMARY" + shell: bash - - name: Post Performance Comment (macOS) - if: startsWith(matrix.os, 'macos') && github.event_name == 'pull_request' + - name: Post Performance Comment + if: always() && github.event_name == 'pull_request' uses: marocchino/sticky-pull-request-comment@v2 with: - header: perf-macos - message: | - ## Performance Report (macOS) - - | Metric | PR (P50) | PR (P95) | Baseline (P50) | Delta | - |--------|----------|----------|----------------|-------| - | Server Startup | ${{ steps.perf-macos.outputs.pr_startup }}ms | ${{ steps.perf-macos.outputs.pr_startup_p95 }}ms | ${{ steps.perf-macos.outputs.baseline_startup }}ms | ${{ steps.perf-macos.outputs.startup_diff }}ms | - | Full Refresh | ${{ steps.perf-macos.outputs.pr_refresh }}ms | ${{ steps.perf-macos.outputs.pr_refresh_p95 }}ms | ${{ steps.perf-macos.outputs.baseline_refresh }}ms | ${{ steps.perf-macos.outputs.refresh_diff }}ms | - - > Results based on 10 iterations. P50 = median, P95 = 95th percentile. - - --- -
- Legend - - - :rocket: Significant speedup (>100ms faster) - - :white_check_mark: Faster than baseline - - :heavy_minus_sign: No significant change - - :small_red_triangle: Slower than baseline (>100ms) - - :warning: Significant slowdown (>500ms) -
+ header: ${{ matrix.comment_header }} + path: performance-report.md diff --git a/crates/pet/tests/e2e_performance.rs b/crates/pet/tests/e2e_performance.rs index 4340c95f..38620078 100644 --- a/crates/pet/tests/e2e_performance.rs +++ b/crates/pet/tests/e2e_performance.rs @@ -8,13 +8,14 @@ use serde::Deserialize; use serde_json::{json, Value}; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::env; use std::io::{BufRead, BufReader, Read, Write}; use std::path::PathBuf; -use std::process::{Child, Command, Stdio}; +use std::process::{Child, ChildStdin, ChildStdout, Command, Stdio}; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; +use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; mod common; @@ -24,6 +25,7 @@ static REQUEST_ID: AtomicU32 = AtomicU32::new(1); /// Number of iterations for statistical tests const STAT_ITERATIONS: usize = 10; +const STDERR_TAIL_LINES: usize = 100; /// Statistical metrics with percentile calculations #[derive(Debug, Clone, Default)] @@ -248,6 +250,10 @@ impl SharedState { /// JSONRPC client for communicating with the pet server pub struct PetClient { process: Child, + stdin: ChildStdin, + stdout: BufReader, + stderr_tail: Arc>>, + stderr_handle: Option>, state: Arc, start_time: Instant, } @@ -266,16 +272,34 @@ impl PetClient { let start_time = Instant::now(); - let process = Command::new(&pet_exe) + let mut process = Command::new(&pet_exe) .arg("server") .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() .map_err(|e| format!("Failed to spawn pet server: {}", e))?; + let stdin = process + .stdin + .take() + .expect("PET stdin must be piped by the command above"); + let stdout = process + .stdout + .take() + .expect("PET stdout must be piped by the command above"); + let stderr = process + .stderr + .take() + .expect("PET stderr must be piped by the command above"); + let stderr_tail = Arc::new(Mutex::new(VecDeque::with_capacity(STDERR_TAIL_LINES))); + let stderr_handle = spawn_stderr_reader(stderr, stderr_tail.clone()); Ok(Self { process, + stdin, + stdout: BufReader::new(stdout), + stderr_tail, + stderr_handle: Some(stderr_handle), state: Arc::new(SharedState::new()), start_time, }) @@ -299,11 +323,10 @@ impl PetClient { // Write request { - let stdin = self.process.stdin.as_mut().ok_or("Failed to get stdin")?; - stdin + self.stdin .write_all(message.as_bytes()) .map_err(|e| format!("Failed to write request: {}", e))?; - stdin + self.stdin .flush() .map_err(|e| format!("Failed to flush stdin: {}", e))?; } @@ -311,46 +334,17 @@ impl PetClient { // Clone state reference for use in the loop let state = self.state.clone(); - // Read response - handle notifications until we get our response - let stdout = self.process.stdout.as_mut().ok_or("Failed to get stdout")?; - let mut reader = BufReader::new(stdout); - + // Read response - handle notifications until we get our response. + // The reader lives for the process lifetime so read-ahead bytes are never discarded. loop { - // Read headers until empty line - let mut content_length: Option = None; - loop { - let mut header_line = String::new(); - reader - .read_line(&mut header_line) - .map_err(|e| format!("Failed to read header: {}", e))?; - - let trimmed = header_line.trim(); - if trimmed.is_empty() { - // End of headers - break; - } - - if let Some(len_str) = trimmed.strip_prefix("Content-Length: ") { - content_length = Some( - len_str - .parse() - .map_err(|e| format!("Failed to parse content length: {}", e))?, - ); + let value = read_jsonrpc_message(&mut self.stdout).map_err(|error| { + let stderr = self.stderr_output(); + if stderr.is_empty() { + error + } else { + format!("{error}; PET stderr tail:\n{stderr}") } - // Ignore Content-Type and other headers - } - - let content_length = content_length.ok_or("Missing Content-Length header")?; - - // Read body - let mut body = vec![0u8; content_length]; - reader - .read_exact(&mut body) - .map_err(|e| format!("Failed to read body: {}", e))?; - - let body_str = String::from_utf8_lossy(&body); - let value: Value = serde_json::from_str(&body_str) - .map_err(|e| format!("Failed to parse response: {}", e))?; + })?; // Check if this is a notification or our response if let Some(notif_method) = value.get("method").and_then(|m| m.as_str()) { @@ -374,6 +368,16 @@ impl PetClient { } } + fn stderr_output(&self) -> String { + self.stderr_tail + .lock() + .expect("PET stderr tail mutex poisoned") + .iter() + .cloned() + .collect::>() + .join("\n") + } + /// Configure the server pub fn configure(&mut self, config: Value) -> Result { let start = Instant::now(); @@ -433,6 +437,9 @@ impl Drop for PetClient { fn drop(&mut self) { let _ = self.process.kill(); let _ = self.process.wait(); + if let Some(stderr_handle) = self.stderr_handle.take() { + let _ = stderr_handle.join(); + } } } @@ -504,6 +511,89 @@ fn get_workspace_dir() -> PathBuf { }) } +fn read_jsonrpc_message(reader: &mut impl BufRead) -> Result { + let mut content_length = None; + loop { + let mut header_line = String::new(); + let bytes_read = reader + .read_line(&mut header_line) + .map_err(|error| format!("Failed to read header: {error}"))?; + if bytes_read == 0 { + return Err("PET stdout closed while reading a JSONRPC header".to_string()); + } + + let trimmed = header_line.trim(); + if trimmed.is_empty() { + break; + } + if let Some(length) = trimmed.strip_prefix("Content-Length: ") { + content_length = Some( + length + .parse::() + .map_err(|error| format!("Failed to parse content length: {error}"))?, + ); + } + } + + let content_length = content_length.ok_or("Missing Content-Length header")?; + let mut body = vec![0u8; content_length]; + reader + .read_exact(&mut body) + .map_err(|error| format!("Failed to read body: {error}"))?; + serde_json::from_slice(&body).map_err(|error| format!("Failed to parse response: {error}")) +} + +fn spawn_stderr_reader( + stderr: impl Read + Send + 'static, + stderr_tail: Arc>>, +) -> JoinHandle<()> { + thread::spawn(move || { + for line in BufReader::new(stderr).lines() { + let line = match line { + Ok(line) => line, + Err(error) => format!("Failed to read PET stderr: {error}"), + }; + let mut tail = stderr_tail.lock().expect("PET stderr tail mutex poisoned"); + if tail.len() == STDERR_TAIL_LINES { + tail.pop_front(); + } + tail.push_back(line); + } + }) +} + +#[test] +fn jsonrpc_reader_preserves_buffered_follow_up_message() { + let first = json!({"jsonrpc": "2.0", "id": 1, "result": {"value": 1}}); + let second = json!({"jsonrpc": "2.0", "id": 2, "result": {"value": 2}}); + let framed = [first.clone(), second.clone()] + .into_iter() + .map(|message| { + let body = serde_json::to_string(&message).unwrap(); + format!("Content-Length: {}\r\n\r\n{}", body.len(), body) + }) + .collect::(); + let mut reader = BufReader::new(std::io::Cursor::new(framed.into_bytes())); + + assert_eq!(read_jsonrpc_message(&mut reader).unwrap(), first); + assert_eq!(read_jsonrpc_message(&mut reader).unwrap(), second); +} + +#[test] +fn stderr_reader_drains_input_and_bounds_diagnostic_tail() { + let input = (0..STDERR_TAIL_LINES + 5) + .map(|index| format!("line {index}\n")) + .collect::(); + let tail = Arc::new(Mutex::new(VecDeque::new())); + let handle = spawn_stderr_reader(std::io::Cursor::new(input.into_bytes()), tail.clone()); + handle.join().unwrap(); + + let tail = tail.lock().unwrap(); + assert_eq!(tail.len(), STDERR_TAIL_LINES); + assert_eq!(tail.front().map(String::as_str), Some("line 5")); + assert_eq!(tail.back().map(String::as_str), Some("line 104")); +} + // ============================================================================ // Performance Tests // ============================================================================ diff --git a/docs/QUALITY_SNAPSHOTS.md b/docs/QUALITY_SNAPSHOTS.md new file mode 100644 index 00000000..51ac322e --- /dev/null +++ b/docs/QUALITY_SNAPSHOTS.md @@ -0,0 +1,50 @@ +# Quality snapshots + +PET uses pull-request snapshots to prevent performance and coverage drift. Each pull request is compared with artifacts produced for the exact pull-request base commit, not the latest moving `main` tip. + +## Performance gate + +The performance workflow runs 10 end-to-end JSON-RPC iterations on Linux, Windows, and macOS. A comparison is valid only when: + +- current and baseline metrics contain at least five samples for every required distribution; +- environment and manager counts match exactly; and +- the benchmark command and JSON extraction both succeed. + +A metric blocks when it exceeds both its absolute and relative budget: + +| Metric | Linux | Windows | macOS | +| --- | ---: | ---: | ---: | +| Server startup P50 | 5 ms / 100% | 10 ms / 50% | 100 ms / 50% | +| Server startup P95 | 50 ms / 200% | 50 ms / 100% | 10,000 ms / 100% | +| Full refresh P50 | 25 ms / 30% | 50 ms / 30% | 100 ms / 50% | +| Full refresh P95 | 1,000 ms / 100% | 5,000 ms / 100% | 5,000 ms / 25% | +| Time to first environment P50 | 20 ms / 100% | 25 ms / 50% | 150 ms / 50% | +| Time to first environment P95 | 250 ms / 100% | 500 ms / 100% | 10,000 ms / 100% | + +Each cell is `absolute / relative`. The budgets reflect observed GitHub-hosted runner variance from 11 consecutive main-branch baselines. Tighten them when a noisy path is fixed rather than normalizing a known regression into the baseline. + +The dual budget avoids failing on tiny percentage changes while still blocking material latency regressions. Tail metrics remain mandatory; a healthy median does not excuse a degraded P95. + +## Coverage gate + +Linux and Windows line and function coverage are compared with the exact base commit. A decrease greater than 0.01 percentage points blocks the pull request. Coverage artifacts and comments remain available for inspection even when the comparison fails. + +## Running locally + +```powershell +python -m unittest discover -s scripts/tests -p 'test_*.py' -v +python scripts/quality_snapshot.py performance --current metrics.json --baseline baseline.json --platform Windows --report report.md +python scripts/quality_snapshot.py coverage --current lcov.info --baseline baseline.info --platform Windows --report report.md +``` + +Run the E2E benchmark with: + +```powershell +cargo test --release --features ci-perf --test e2e_performance test_performance_summary -- --nocapture +``` + +The E2E client keeps one buffered stdout reader for the process lifetime and continuously drains a bounded stderr tail so protocol read-ahead and pipe backpressure cannot distort measurements. + +## Known investigations + +The persistent macOS cold-refresh tail is tracked by issue #504. Existing tail latency is represented in the baseline, but any further regression is still gated. diff --git a/scripts/quality_snapshot.py b/scripts/quality_snapshot.py new file mode 100644 index 00000000..e934d7d4 --- /dev/null +++ b/scripts/quality_snapshot.py @@ -0,0 +1,345 @@ +#!/usr/bin/env python3 +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. +"""Validate and compare PET performance and coverage snapshots.""" + +from __future__ import annotations + +import argparse +import json +import math +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Sequence + + +class SnapshotError(ValueError): + """Raised when snapshot data is missing or malformed.""" + + +@dataclass(frozen=True) +class RegressionBudget: + absolute_ms: float + relative_percent: float + + +@dataclass(frozen=True) +class MetricSpec: + label: str + group: str + percentile: str + + +@dataclass(frozen=True) +class MetricComparison: + label: str + current: float + baseline: float + budget: RegressionBudget + + @property + def delta(self) -> float: + return self.current - self.baseline + + @property + def percent_change(self) -> float: + if self.baseline == 0: + return math.inf if self.current > 0 else 0.0 + return self.delta / self.baseline * 100 + + @property + def regressed(self) -> bool: + return self.delta > self.budget.absolute_ms and self.percent_change > self.budget.relative_percent + + +PERFORMANCE_METRICS = ( + MetricSpec('Server startup P50', 'server_startup', 'p50'), + MetricSpec('Server startup P95', 'server_startup', 'p95'), + MetricSpec('Full refresh P50', 'full_refresh', 'p50'), + MetricSpec('Full refresh P95', 'full_refresh', 'p95'), + MetricSpec('Time to first environment P50', 'time_to_first_env', 'p50'), + MetricSpec('Time to first environment P95', 'time_to_first_env', 'p95'), +) +PERFORMANCE_BUDGETS = { + 'linux': ( + RegressionBudget(5, 100), + RegressionBudget(50, 200), + RegressionBudget(25, 30), + RegressionBudget(1_000, 100), + RegressionBudget(20, 100), + RegressionBudget(250, 100), + ), + 'windows': ( + RegressionBudget(10, 50), + RegressionBudget(50, 100), + RegressionBudget(50, 30), + RegressionBudget(5_000, 100), + RegressionBudget(25, 50), + RegressionBudget(500, 100), + ), + 'macos': ( + RegressionBudget(100, 50), + RegressionBudget(10_000, 100), + RegressionBudget(100, 50), + RegressionBudget(5_000, 25), + RegressionBudget(150, 50), + RegressionBudget(10_000, 100), + ), +} +COVERAGE_BUDGET_PERCENTAGE_POINTS = 0.01 + + +def platform_key(platform: str) -> str: + normalized = platform.casefold() + if 'windows' in normalized: + return 'windows' + if 'macos' in normalized: + return 'macos' + if 'linux' in normalized or 'ubuntu' in normalized: + return 'linux' + raise SnapshotError(f'Unsupported performance platform: {platform}') + + +def performance_specs(platform: str) -> list[tuple[MetricSpec, RegressionBudget]]: + return list(zip(PERFORMANCE_METRICS, PERFORMANCE_BUDGETS[platform_key(platform)], strict=True)) + + +def load_json(path: Path) -> dict[str, Any]: + try: + value = json.loads(path.read_text(encoding='utf-8')) + except FileNotFoundError as error: + raise SnapshotError(f'Snapshot file does not exist: {path}') from error + except json.JSONDecodeError as error: + raise SnapshotError(f'Snapshot file is not valid JSON: {path}: {error}') from error + if not isinstance(value, dict): + raise SnapshotError(f'Snapshot root must be an object: {path}') + return value + + +def require_number(value: Any, name: str, *, minimum: float = 0) -> float: + if isinstance(value, bool) or not isinstance(value, (int, float)) or not math.isfinite(value): + raise SnapshotError(f'{name} must be a finite number') + numeric = float(value) + if numeric < minimum: + raise SnapshotError(f'{name} must be at least {minimum}') + return numeric + + +def require_integer(value: Any, name: str, *, minimum: int = 0) -> int: + numeric = require_number(value, name, minimum=minimum) + if not numeric.is_integer(): + raise SnapshotError(f'{name} must be an integer') + return int(numeric) + + +def performance_value(snapshot: dict[str, Any], spec: MetricSpec, source: str) -> float: + stats = snapshot.get('stats') + if not isinstance(stats, dict): + raise SnapshotError(f'{source}.stats must be an object') + group = stats.get(spec.group) + if not isinstance(group, dict): + raise SnapshotError(f'{source}.stats.{spec.group} must be an object') + require_integer(group.get('count'), f'{source}.stats.{spec.group}.count', minimum=5) + return require_number(group.get(spec.percentile), f'{source}.stats.{spec.group}.{spec.percentile}') + + +def compare_performance( + current: dict[str, Any], baseline: dict[str, Any], platform: str +) -> tuple[list[MetricComparison], list[str]]: + current_envs = require_integer(current.get('environments_count'), 'current.environments_count', minimum=1) + baseline_envs = require_integer(baseline.get('environments_count'), 'baseline.environments_count', minimum=1) + current_managers = require_integer(current.get('managers_count'), 'current.managers_count') + baseline_managers = require_integer(baseline.get('managers_count'), 'baseline.managers_count') + + failures: list[str] = [] + if current_envs != baseline_envs: + failures.append(f'Environment inventory changed: current={current_envs}, baseline={baseline_envs}') + if current_managers != baseline_managers: + failures.append(f'Manager inventory changed: current={current_managers}, baseline={baseline_managers}') + + comparisons = [ + MetricComparison( + spec.label, + performance_value(current, spec, 'current'), + performance_value(baseline, spec, 'baseline'), + budget, + ) + for spec, budget in performance_specs(platform) + ] + failures.extend( + f'{comparison.label} regressed by {comparison.delta:.0f}ms ({comparison.percent_change:.1f}%)' + for comparison in comparisons + if comparison.regressed + ) + return comparisons, failures + + +def parse_lcov(path: Path) -> tuple[int, int, int, int]: + try: + lines = path.read_text(encoding='utf-8', errors='replace').splitlines() + except FileNotFoundError as error: + raise SnapshotError(f'Coverage file does not exist: {path}') from error + lines_found = lines_hit = functions_found = functions_hit = 0 + try: + for line in lines: + if line.startswith('LF:'): + lines_found += int(line[3:]) + elif line.startswith('LH:'): + lines_hit += int(line[3:]) + elif line.startswith('FNF:'): + functions_found += int(line[4:]) + elif line.startswith('FNH:'): + functions_hit += int(line[4:]) + except ValueError as error: + raise SnapshotError(f'Coverage file has a malformed summary count: {path}') from error + if lines_found == 0 or functions_found == 0: + raise SnapshotError(f'Coverage file has no line/function summary data: {path}') + if lines_hit > lines_found or functions_hit > functions_found: + raise SnapshotError(f'Coverage file has invalid hit totals: {path}') + return lines_hit, lines_found, functions_hit, functions_found + + +def coverage_percent(hit: int, found: int) -> float: + return hit / found * 100 + + +def compare_coverage(current: Path, baseline: Path) -> tuple[dict[str, float], list[str]]: + current_lh, current_lf, current_fnh, current_fnf = parse_lcov(current) + baseline_lh, baseline_lf, baseline_fnh, baseline_fnf = parse_lcov(baseline) + values = { + 'current_lines': coverage_percent(current_lh, current_lf), + 'baseline_lines': coverage_percent(baseline_lh, baseline_lf), + 'current_functions': coverage_percent(current_fnh, current_fnf), + 'baseline_functions': coverage_percent(baseline_fnh, baseline_fnf), + } + values['line_delta'] = values['current_lines'] - values['baseline_lines'] + values['function_delta'] = values['current_functions'] - values['baseline_functions'] + failures = [] + if values['line_delta'] < -COVERAGE_BUDGET_PERCENTAGE_POINTS: + failures.append(f"Line coverage decreased by {abs(values['line_delta']):.3f} percentage points") + if values['function_delta'] < -COVERAGE_BUDGET_PERCENTAGE_POINTS: + failures.append(f"Function coverage decreased by {abs(values['function_delta']):.3f} percentage points") + return values, failures + + +def status_icon(failed: bool, delta: float) -> str: + if failed: + return ':x:' + if delta < 0: + return ':white_check_mark:' + if delta > 0: + return ':small_red_triangle:' + return ':heavy_minus_sign:' + + +def performance_report( + platform: str, + comparisons: Sequence[MetricComparison], + failures: Sequence[str], + current: dict[str, Any], + baseline: dict[str, Any], +) -> str: + rows = [] + for comparison in comparisons: + rows.append( + f'| {comparison.label} | {comparison.current:.0f}ms | {comparison.baseline:.0f}ms | ' + f'{comparison.delta:+.0f}ms | {comparison.percent_change:+.1f}% | ' + f'>{comparison.budget.absolute_ms:.0f}ms and >{comparison.budget.relative_percent:.0f}% | ' + f"{status_icon(comparison.regressed, comparison.delta)} |" + ) + result = ':x: Regression detected' if failures else ':white_check_mark: Within regression budgets' + report = [ + f'## Performance Report ({platform})', + '', + f'**Result:** {result}', + '', + '| Metric | PR | Baseline | Delta | Change | Blocking budget | Status |', + '|--------|----|----------|-------|--------|-----------------|--------|', + *rows, + '', + '| Workload | PR | Baseline |', + '|----------|---:|---------:|', + f"| Environments | {current['environments_count']} | {baseline['environments_count']} |", + f"| Managers | {current['managers_count']} | {baseline['managers_count']} |", + ] + if failures: + report.extend(['', '### Blocking findings', *[f'- {failure}' for failure in failures]]) + report.extend([ + '', + '> A regression must exceed both the documented absolute and relative budget. ' + 'Environment and manager inventories must match exactly.', + ]) + return '\n'.join(report) + '\n' + + +def coverage_report(platform: str, values: dict[str, float], failures: Sequence[str]) -> str: + result = ':x: Regression detected' if failures else ':white_check_mark: Within regression budget' + report = [ + f'## Test Coverage Report ({platform})', + '', + f'**Result:** {result}', + '', + '| Metric | PR | Baseline | Delta |', + '|--------|----|----------|-------|', + f"| Lines | {values['current_lines']:.3f}% | {values['baseline_lines']:.3f}% | {values['line_delta']:+.3f}pp |", + f"| Functions | {values['current_functions']:.3f}% | {values['baseline_functions']:.3f}% | {values['function_delta']:+.3f}pp |", + ] + if failures: + report.extend(['', '### Blocking findings', *[f'- {failure}' for failure in failures]]) + report.extend(['', f'> Allowed numerical tolerance: {COVERAGE_BUDGET_PERCENTAGE_POINTS:.2f} percentage points.']) + return '\n'.join(report) + '\n' + + +def write_report(report: str, report_path: Path, summary_path: Path | None) -> None: + report_path.write_text(report, encoding='utf-8') + if summary_path is not None: + with summary_path.open('a', encoding='utf-8') as summary: + summary.write(report) + + +def run_performance(args: argparse.Namespace) -> int: + try: + current = load_json(args.current) + baseline = load_json(args.baseline) + comparisons, failures = compare_performance(current, baseline, args.platform) + report = performance_report(args.platform, comparisons, failures, current, baseline) + except SnapshotError as error: + failures = [str(error)] + report = f'## Performance Report ({args.platform})\n\n:x: **Invalid snapshot:** {error}\n' + write_report(report, args.report, args.summary) + return 1 if failures else 0 + + +def run_coverage(args: argparse.Namespace) -> int: + try: + values, failures = compare_coverage(args.current, args.baseline) + report = coverage_report(args.platform, values, failures) + except SnapshotError as error: + failures = [str(error)] + report = f'## Test Coverage Report ({args.platform})\n\n:x: **Invalid snapshot:** {error}\n' + write_report(report, args.report, args.summary) + return 1 if failures else 0 + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + subparsers = parser.add_subparsers(dest='command', required=True) + for command, handler in (('performance', run_performance), ('coverage', run_coverage)): + subparser = subparsers.add_parser(command) + subparser.add_argument('--current', type=Path, required=True) + subparser.add_argument('--baseline', type=Path, required=True) + subparser.add_argument('--platform', required=True) + subparser.add_argument('--report', type=Path, required=True) + subparser.add_argument('--summary', type=Path) + subparser.set_defaults(handler=handler) + return parser + + +def main() -> int: + args = build_parser().parse_args() + return args.handler(args) + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/scripts/tests/test_quality_snapshot.py b/scripts/tests/test_quality_snapshot.py new file mode 100644 index 00000000..4d5a63a9 --- /dev/null +++ b/scripts/tests/test_quality_snapshot.py @@ -0,0 +1,212 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import argparse +import json +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from quality_snapshot import ( # noqa: E402 + SnapshotError, + compare_coverage, + compare_performance, + load_json, + run_coverage, + run_performance, +) + + +def performance_snapshot( + *, refresh_p50=100, refresh_p95=500, startup_p50=10, startup_p95=20, + first_p50=15, first_p95=30, environments=5, managers=1 +): + return { + 'server_startup_ms': startup_p50, + 'full_refresh_ms': refresh_p50, + 'time_to_first_env_ms': first_p50, + 'environments_count': environments, + 'managers_count': managers, + 'stats': { + 'server_startup': {'count': 10, 'p50': startup_p50, 'p95': startup_p95}, + 'full_refresh': {'count': 10, 'p50': refresh_p50, 'p95': refresh_p95}, + 'time_to_first_env': {'count': 10, 'p50': first_p50, 'p95': first_p95}, + }, + } + + +def write_lcov(path, *, lines_hit, lines_found, functions_hit, functions_found): + path.write_text( + f'SF:example.rs\nLF:{lines_found}\nLH:{lines_hit}\n' + f'FNF:{functions_found}\nFNH:{functions_hit}\nend_of_record\n', + encoding='utf-8', + ) + + +class PerformanceSnapshotTests(unittest.TestCase): + def test_unchanged_snapshot_passes(self): + comparisons, failures = compare_performance(performance_snapshot(), performance_snapshot(), 'Windows') + self.assertEqual(len(comparisons), 6) + self.assertEqual(failures, []) + + def test_p50_regression_fails_when_both_budgets_are_exceeded(self): + current = performance_snapshot(refresh_p50=180) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=100), 'Windows') + self.assertTrue(any('Full refresh P50' in failure for failure in failures)) + + def test_p95_regression_fails_even_when_p50_is_unchanged(self): + current = performance_snapshot(refresh_p95=7_000) + _, failures = compare_performance(current, performance_snapshot(refresh_p95=500), 'Windows') + self.assertTrue(any('Full refresh P95' in failure for failure in failures)) + + def test_noise_inside_absolute_budget_passes(self): + current = performance_snapshot(refresh_p50=140) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=100), 'Windows') + self.assertEqual(failures, []) + + + def test_relative_budget_must_also_be_exceeded(self): + current = performance_snapshot(refresh_p50=1_060) + _, failures = compare_performance(current, performance_snapshot(refresh_p50=1_000), 'Windows') + self.assertEqual(failures, []) + + def test_platform_specific_budget_changes_decision(self): + current = performance_snapshot(refresh_p50=140) + baseline = performance_snapshot(refresh_p50=100) + _, windows_failures = compare_performance(current, baseline, 'Windows') + _, linux_failures = compare_performance(current, baseline, 'Linux') + self.assertEqual(windows_failures, []) + self.assertTrue(any('Full refresh P50' in failure for failure in linux_failures)) + + def test_unknown_platform_is_invalid(self): + with self.assertRaises(SnapshotError): + compare_performance(performance_snapshot(), performance_snapshot(), 'unknown') + + def test_inventory_mismatch_fails(self): + current = performance_snapshot(environments=6, managers=2) + _, failures = compare_performance(current, performance_snapshot(), 'Windows') + self.assertTrue(any('Environment inventory changed' in failure for failure in failures)) + self.assertTrue(any('Manager inventory changed' in failure for failure in failures)) + + def test_missing_metric_is_invalid(self): + current = performance_snapshot() + del current['stats']['full_refresh']['p95'] + with self.assertRaises(SnapshotError): + compare_performance(current, performance_snapshot(), 'Windows') + + def test_too_few_samples_is_invalid(self): + current = performance_snapshot() + current['stats']['full_refresh']['count'] = 1 + with self.assertRaises(SnapshotError): + compare_performance(current, performance_snapshot(), 'Windows') + + +class CoverageSnapshotTests(unittest.TestCase): + def compare(self, current_values, baseline_values): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + write_lcov(current, **current_values) + write_lcov(baseline, **baseline_values) + return compare_coverage(current, baseline) + + def test_coverage_increase_passes(self): + _, failures = self.compare( + dict(lines_hit=91, lines_found=100, functions_hit=46, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertEqual(failures, []) + + def test_line_coverage_decrease_fails(self): + _, failures = self.compare( + dict(lines_hit=89, lines_found=100, functions_hit=45, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertTrue(any('Line coverage decreased' in failure for failure in failures)) + + def test_function_coverage_decrease_fails(self): + _, failures = self.compare( + dict(lines_hit=90, lines_found=100, functions_hit=44, functions_found=50), + dict(lines_hit=90, lines_found=100, functions_hit=45, functions_found=50), + ) + self.assertTrue(any('Function coverage decreased' in failure for failure in failures)) + + def test_invalid_lcov_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + current.write_text('SF:example.rs\nend_of_record\n', encoding='utf-8') + write_lcov(baseline, lines_hit=1, lines_found=1, functions_hit=1, functions_found=1) + with self.assertRaises(SnapshotError): + compare_coverage(current, baseline) + + def test_malformed_lcov_count_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + current.write_text('SF:example.rs\nLF:not-a-number\nLH:1\nFNF:1\nFNH:1\n', encoding='utf-8') + write_lcov(baseline, lines_hit=1, lines_found=1, functions_hit=1, functions_found=1) + with self.assertRaises(SnapshotError): + compare_coverage(current, baseline) + + +class JsonSnapshotTests(unittest.TestCase): + def test_malformed_json_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / 'metrics.json' + path.write_text('{', encoding='utf-8') + with self.assertRaises(SnapshotError): + load_json(path) + + +class CommandTests(unittest.TestCase): + def test_invalid_performance_snapshot_returns_failure_and_writes_report(self): + with tempfile.TemporaryDirectory() as directory: + directory = Path(directory) + current = directory / 'current.json' + baseline = directory / 'baseline.json' + report = directory / 'report.md' + current.write_text('{', encoding='utf-8') + baseline.write_text(json.dumps(performance_snapshot()), encoding='utf-8') + + exit_code = run_performance( + argparse.Namespace( + current=current, + baseline=baseline, + platform='Windows', + report=report, + summary=None, + ) + ) + + self.assertEqual(exit_code, 1) + self.assertIn('Invalid snapshot', report.read_text(encoding='utf-8')) + + def test_coverage_regression_returns_failure_and_writes_report(self): + with tempfile.TemporaryDirectory() as directory: + directory = Path(directory) + current = directory / 'current.info' + baseline = directory / 'baseline.info' + report = directory / 'report.md' + write_lcov(current, lines_hit=89, lines_found=100, functions_hit=44, functions_found=50) + write_lcov(baseline, lines_hit=90, lines_found=100, functions_hit=45, functions_found=50) + + exit_code = run_coverage( + argparse.Namespace( + current=current, + baseline=baseline, + platform='test', + report=report, + summary=None, + ) + ) + + self.assertEqual(exit_code, 1) + self.assertIn('Blocking findings', report.read_text(encoding='utf-8')) + + +if __name__ == '__main__': + unittest.main() From 17676500ac0633c3741a27007838233d2b9beee0 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 12:10:13 -0700 Subject: [PATCH 2/3] fix: address review feedback (PR #505) Convert internal budget-table drift into a reported SnapshotError and document the comparator's Python 3.10 minimum, with direct regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/QUALITY_SNAPSHOTS.md | 2 ++ scripts/quality_snapshot.py | 9 ++++++++- scripts/tests/test_quality_snapshot.py | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/QUALITY_SNAPSHOTS.md b/docs/QUALITY_SNAPSHOTS.md index 51ac322e..60791236 100644 --- a/docs/QUALITY_SNAPSHOTS.md +++ b/docs/QUALITY_SNAPSHOTS.md @@ -31,6 +31,8 @@ Linux and Windows line and function coverage are compared with the exact base co ## Running locally +The comparator requires Python 3.10 or newer. + ```powershell python -m unittest discover -s scripts/tests -p 'test_*.py' -v python scripts/quality_snapshot.py performance --current metrics.json --baseline baseline.json --platform Windows --report report.md diff --git a/scripts/quality_snapshot.py b/scripts/quality_snapshot.py index e934d7d4..a17987e2 100644 --- a/scripts/quality_snapshot.py +++ b/scripts/quality_snapshot.py @@ -102,7 +102,14 @@ def platform_key(platform: str) -> str: def performance_specs(platform: str) -> list[tuple[MetricSpec, RegressionBudget]]: - return list(zip(PERFORMANCE_METRICS, PERFORMANCE_BUDGETS[platform_key(platform)], strict=True)) + key = platform_key(platform) + budgets = PERFORMANCE_BUDGETS[key] + if len(budgets) != len(PERFORMANCE_METRICS): + raise SnapshotError( + f'Performance budget count for {key} does not match metric count: ' + f'{len(budgets)} != {len(PERFORMANCE_METRICS)}' + ) + return list(zip(PERFORMANCE_METRICS, budgets)) def load_json(path: Path) -> dict[str, Any]: diff --git a/scripts/tests/test_quality_snapshot.py b/scripts/tests/test_quality_snapshot.py index 4d5a63a9..dd6a8486 100644 --- a/scripts/tests/test_quality_snapshot.py +++ b/scripts/tests/test_quality_snapshot.py @@ -11,10 +11,12 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from quality_snapshot import ( # noqa: E402 + PERFORMANCE_BUDGETS, SnapshotError, compare_coverage, compare_performance, load_json, + performance_specs, run_coverage, run_performance, ) @@ -81,6 +83,15 @@ def test_platform_specific_budget_changes_decision(self): self.assertEqual(windows_failures, []) self.assertTrue(any('Full refresh P50' in failure for failure in linux_failures)) + def test_budget_metric_mismatch_is_invalid(self): + original = PERFORMANCE_BUDGETS['windows'] + PERFORMANCE_BUDGETS['windows'] = original[:-1] + try: + with self.assertRaisesRegex(SnapshotError, 'does not match metric count'): + performance_specs('Windows') + finally: + PERFORMANCE_BUDGETS['windows'] = original + def test_unknown_platform_is_invalid(self): with self.assertRaises(SnapshotError): compare_performance(performance_snapshot(), performance_snapshot(), 'unknown') From 2425a46b83ab40443c8dd15a2a52ccfceba15631 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 10 Aug 2026 12:29:14 -0700 Subject: [PATCH 3/3] fix: reject negative coverage counters (PR #505) Treat negative LCOV summary counts as invalid snapshots and cover the failure contract directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/quality_snapshot.py | 2 ++ scripts/tests/test_quality_snapshot.py | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/scripts/quality_snapshot.py b/scripts/quality_snapshot.py index a17987e2..250eaf49 100644 --- a/scripts/quality_snapshot.py +++ b/scripts/quality_snapshot.py @@ -200,6 +200,8 @@ def parse_lcov(path: Path) -> tuple[int, int, int, int]: functions_hit += int(line[4:]) except ValueError as error: raise SnapshotError(f'Coverage file has a malformed summary count: {path}') from error + if min(lines_hit, lines_found, functions_hit, functions_found) < 0: + raise SnapshotError(f'Coverage file has negative summary counts: {path}') if lines_found == 0 or functions_found == 0: raise SnapshotError(f'Coverage file has no line/function summary data: {path}') if lines_hit > lines_found or functions_hit > functions_found: diff --git a/scripts/tests/test_quality_snapshot.py b/scripts/tests/test_quality_snapshot.py index dd6a8486..6a5cb6a5 100644 --- a/scripts/tests/test_quality_snapshot.py +++ b/scripts/tests/test_quality_snapshot.py @@ -154,6 +154,15 @@ def test_invalid_lcov_is_rejected(self): with self.assertRaises(SnapshotError): compare_coverage(current, baseline) + def test_negative_lcov_count_is_rejected(self): + with tempfile.TemporaryDirectory() as directory: + current = Path(directory) / 'current.info' + baseline = Path(directory) / 'baseline.info' + current.write_text('SF:example.rs\nLF:-1\nLH:-1\nFNF:1\nFNH:1\n', encoding='utf-8') + write_lcov(baseline, lines_hit=1, lines_found=1, functions_hit=1, functions_found=1) + with self.assertRaisesRegex(SnapshotError, 'negative summary counts'): + compare_coverage(current, baseline) + def test_malformed_lcov_count_is_rejected(self): with tempfile.TemporaryDirectory() as directory: current = Path(directory) / 'current.info'