Integrate UI test category detection into PR review and fix gate reliability #35133
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -437,6 +437,51 @@ function Invoke-CopilotStep { | |
| return $exitCode | ||
| } | ||
|
|
||
| # ═════════════════════════════════════════════════════════════════════════════ | ||
| # STEP 0.5: DETECT UI Test Categories (detection only — no pipeline trigger) | ||
| # ═════════════════════════════════════════════════════════════════════════════ | ||
|
|
||
| Write-Host "" | ||
| Write-Host "╔═══════════════════════════════════════════════════════════╗" -ForegroundColor Cyan | ||
| Write-Host "║ STEP 0.5: DETECT UI TEST CATEGORIES ║" -ForegroundColor Cyan | ||
| Write-Host "╚═══════════════════════════════════════════════════════════╝" -ForegroundColor Cyan | ||
|
|
||
| $uitestCategories = "" | ||
|
|
||
| $detectScript = Join-Path $RepoRoot "eng/scripts/detect-ui-test-categories.ps1" | ||
| if (Test-Path $detectScript) { | ||
|
Contributor
There was a problem hiding this comment. 🟡 Moderate — 2/3 consensus | Variable name shadowing
Recommendation: Use distinct names — e.g., |
||
| try { | ||
| $detectOutput = & pwsh -NoProfile -File $detectScript -PrNumber "$PRNumber" 2>&1 | ||
|
Member
There was a problem hiding this comment. 🟡 MODERATE — Tier 3 AI categories is dead code in this workflow (3/3 after dispute) Step 0.5 calls the detect script with only The detection script is correctly wired for AzDO pipeline use (where Fix: Either (a) move Step 0.5 after Step 2 and read ai-categories.md, or (b) document that
Comment on lines
+451
to
+454
Member
There was a problem hiding this comment. 🔴 CRITICAL — Step 0.5 leaves repo in detached HEAD, corrupting Step 1 gate (3/3 reviewers) The detect script is called with Since the review branch is a squash-merge onto main (different SHA), Step 1's gate runs against the wrong tree state. Fix: Add
Member
There was a problem hiding this comment. 🔴 CRITICAL — Step 0.5 leaves repo in detached HEAD, corrupting Step 1 gate (Flagged by: 3/3 reviewers) The detect script is called with Since the review branch is a squash-merge onto main (different SHA), Step 1's gate runs against the wrong tree state. Fix: Add |
||
| $detectOutput | ForEach-Object { Write-Host " $_" } | ||
|
|
||
| foreach ($line in $detectOutput) { | ||
| $lineStr = $line.ToString() | ||
| if ($lineStr -match 'UITestCategoryList;isOutput=true\](.+)$') { | ||
| $uitestCategories = $Matches[1] | ||
| } | ||
| } | ||
|
|
||
| if ([string]::IsNullOrWhiteSpace($uitestCategories) -or $uitestCategories -eq 'NONE') { | ||
|
Contributor
There was a problem hiding this comment. This condition treats
Both currently produce "No UI test categories detected" which misleads reviewers — a PR touching Recommendation: Distinguish the two cases: if ($uitestCategories -eq 'NONE') {
Write-Host " i️ No UI test categories needed" -ForegroundColor DarkGray
} elseif ([string]::IsNullOrWhiteSpace($uitestCategories)) {
Write-Host " i️ Full UI test matrix (no specific categories detected)" -ForegroundColor DarkGray
} else {
Write-Host " 🎯 Detected categories: $uitestCategories" -ForegroundColor Green
} |
||
| Write-Host " ℹ️ No UI test categories detected" -ForegroundColor DarkGray | ||
| } else { | ||
| Write-Host " 🎯 Detected categories: $uitestCategories" -ForegroundColor Green | ||
|
There was a problem hiding this comment.
|
||
| } | ||
|
|
||
| # Write detection result for AI summary | ||
| $uitestOutputDir = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/uitests" | ||
| New-Item -ItemType Directory -Force -Path $uitestOutputDir | Out-Null | ||
| if ([string]::IsNullOrWhiteSpace($uitestCategories) -or $uitestCategories -eq 'NONE') { | ||
| "No UI test categories detected for this PR." | Set-Content (Join-Path $uitestOutputDir "content.md") -Encoding UTF8 | ||
| } else { | ||
| "**Detected UI test categories:** ``$uitestCategories``" | Set-Content (Join-Path $uitestOutputDir "content.md") -Encoding UTF8 | ||
| } | ||
| } catch { | ||
| Write-Host " ⚠️ Category detection failed (non-fatal): $_" -ForegroundColor Yellow | ||
| } | ||
| } else { | ||
| Write-Host " ⚠️ detect-ui-test-categories.ps1 not found" -ForegroundColor Yellow | ||
| } | ||
|
|
||
| # ═════════════════════════════════════════════════════════════════════════════ | ||
| # STEP 1: Gate - Test Before and After Fix (script, no copilot agent) | ||
| # ═════════════════════════════════════════════════════════════════════════════ | ||
|
|
@@ -452,16 +497,61 @@ New-Item -ItemType Directory -Force -Path $gateOutputDir | Out-Null | |
| # Detect tests in PR | ||
| Write-Host " 🔍 Detecting tests in PR #$PRNumber..." -ForegroundColor Cyan | ||
| $detectScript = Join-Path $PSScriptRoot "shared/Detect-TestsInDiff.ps1" | ||
|
Contributor
There was a problem hiding this comment. 💡 Suggestion —
Recommendation: Use a distinct name like |
||
| if (Test-Path $detectScript) { | ||
| $detectScript = (Resolve-Path $detectScript).Path | ||
| & pwsh -NoProfile -File $detectScript -PRNumber $PRNumber 2>&1 | ForEach-Object { Write-Host " $_" } | ||
| } else { | ||
| Write-Host " ⚠️ Detect-TestsInDiff.ps1 not found at $detectScript" -ForegroundColor Yellow | ||
| } | ||
|
|
||
| # Determine platform for gate | ||
| $gatePlatform = if ($Platform) { $Platform } else { "android" } | ||
| Write-Host " 🧪 Running gate on platform: $gatePlatform" -ForegroundColor Cyan | ||
|
|
||
| $verifyScript = [System.IO.Path]::GetFullPath((Join-Path $PSScriptRoot "../skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1")) | ||
| if (-not (Test-Path $verifyScript)) { | ||
| Write-Host " ❌ verify-tests-fail.ps1 not found at: $verifyScript" -ForegroundColor Red | ||
| $gateResult = "FAILED" | ||
|
Contributor
There was a problem hiding this comment. 💡 Suggestion — Dead
Recommendation: Either remove this line, or restructure so the "not found" path skips the switch entirely (e.g., by using an early |
||
| $gateExitCode = 1 | ||
| $gateOutput = @("verify-tests-fail.ps1 not found at: $verifyScript") | ||
|
Member
There was a problem hiding this comment. 🟡 MODERATE — Gate retry only triggers if report file already exists (Flagged by: 2/3 reviewers) The ENV ERROR check reads from Fix: Also treat "nonzero exit + missing report file" as a retryable environment failure: if ($gateExitCode -ne 0) {
if ((Test-Path $gateContentFile) -and (Get-Content $gateContentFile -Raw) -match 'ENV ERROR') {
$isEnvError = $true
} elseif (-not (Test-Path $gateContentFile)) {
$isEnvError = $true # No report = likely infra failure
}
} |
||
| } else { | ||
|
|
||
| $maxGateAttempts = 3 | ||
| $gateExitCode = 1 | ||
| $gateOutput = @() | ||
|
|
||
| for ($gateAttempt = 1; $gateAttempt -le $maxGateAttempts; $gateAttempt++) { | ||
| if ($gateAttempt -gt 1) { | ||
| Write-Host " 🔄 Retry $gateAttempt/$maxGateAttempts — previous attempt hit environment error" -ForegroundColor Yellow | ||
| } | ||
| $gateOutput = & pwsh -NoProfile -File "$verifyScript" -Platform $gatePlatform -PRNumber $PRNumber -RequireFullVerification 2>&1 | ||
| $gateExitCode = $LASTEXITCODE | ||
| $gateOutput | ForEach-Object { Write-Host " $_" } | ||
|
|
||
| # Check if this was an ENV ERROR (emulator timeout, ADB failure, etc.) | ||
| $gateContentFile = Join-Path $gateOutputDir "verify-tests-fail/verification-report.md" | ||
|
Contributor
There was a problem hiding this comment. 🟡 Moderate — 2/3 consensus | Stale report file across retries
Recommendation: Delete or rename if (Test-Path $gateContentFile) { Remove-Item $gateContentFile -Force } |
||
| $isEnvError = $false | ||
| if ($gateExitCode -ne 0 -and (Test-Path $gateContentFile)) { | ||
| $gateContent = Get-Content $gateContentFile -Raw -ErrorAction SilentlyContinue | ||
|
Contributor
There was a problem hiding this comment. The retry loop resets Recommendation: Delete the report file at the top of each loop iteration: for ($gateAttempt = 1; $gateAttempt -le $maxGateAttempts; $gateAttempt++) {
# Clear previous attempt's report to avoid stale classification
$gateContentFile = Join-Path $gateOutputDir "verify-tests-fail/verification-report.md"
Remove-Item $gateContentFile -ErrorAction SilentlyContinue
# ... rest of loop
} |
||
| if ($gateContent -match 'ENV ERROR') { | ||
| $isEnvError = $true | ||
| Write-Host " ⚠️ Environment error detected (attempt $gateAttempt/$maxGateAttempts)" -ForegroundColor Yellow | ||
| } | ||
| } | ||
|
|
||
| if ($gateExitCode -eq 0 -or -not $isEnvError) { | ||
| break # Real pass or real failure — don't retry | ||
| } | ||
| if ($gateAttempt -lt $maxGateAttempts) { | ||
| Write-Host " ⏳ Waiting 30s before retry..." -ForegroundColor DarkGray | ||
| Start-Sleep -Seconds 30 | ||
| } | ||
| } | ||
| if ($isEnvError) { | ||
|
Contributor
There was a problem hiding this comment. 🟡 Moderate — 2/3 consensus | The check Recommendation: Either add a clarifying comment explaining the invariant, or use a counter: |
||
| Write-Host " ⚠️ All $maxGateAttempts gate attempts hit environment errors" -ForegroundColor Yellow | ||
| } | ||
|
|
||
| } # end else (verify script exists) | ||
|
|
||
| # Exit code: 0 = passed, 1 = verification failed, 2 = no tests detected | ||
| $gateResult = switch ($gateExitCode) { | ||
|
|
@@ -472,30 +562,61 @@ $gateResult = switch ($gateExitCode) { | |
| $gateColor = switch ($gateResult) { "PASSED" { "Green" } "SKIPPED" { "Yellow" } default { "Red" } } | ||
| Write-Host " 📁 Gate result: $gateResult" -ForegroundColor $gateColor | ||
|
|
||
| # Copy the verification report to gate/content.md (always overwrite — the report is the source of truth) | ||
| $verificationReport = Join-Path $gateOutputDir "verify-tests-fail/verification-report.md" | ||
| # Capture last meaningful lines from gate output for fallback diagnostics | ||
| $gateLogTail = @($gateOutput | ForEach-Object { $_.ToString() } | Where-Object { $_ -match '\S' } | Select-Object -Last 20) -join "`n" | ||
|
|
||
| if (Test-Path $verificationReport) { | ||
| $reportContent = Get-Content $verificationReport -Raw -ErrorAction SilentlyContinue | ||
| if ($reportContent) { | ||
| # Strip broken "Test Summary" blocks with empty values (from old verify script format) | ||
| $reportContent = $reportContent -replace '(?s)\*\*Test Summary:\*\*\s*\n- Total:\s*\n- Passed:\s*(True|False)\s*\n- Failed:\s*\n- Skipped:\s*\n?', '' | ||
| $reportContent | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 | ||
| } else { | ||
| # Report exists but has bad format — generate fallback with logs | ||
| Write-Host " ⚠️ Verification report has invalid format — using fallback" -ForegroundColor Yellow | ||
| $resultIcon = switch ($gateResult) { "PASSED" { "✅" } "SKIPPED" { "⚠️" } default { "❌" } } | ||
| @" | ||
| ### Gate Result: $resultIcon $gateResult | ||
|
|
||
| **Platform:** $($gatePlatform.ToUpper()) | ||
|
|
||
| <details> | ||
| <summary>Gate output log</summary> | ||
|
|
||
| `````` | ||
| $gateLogTail | ||
| `````` | ||
|
|
||
| </details> | ||
| "@ | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 | ||
| } | ||
| } elseif (-not (Test-Path (Join-Path $gateOutputDir "content.md"))) { | ||
| if ($gateResult -eq "SKIPPED") { | ||
| @" | ||
| ### Gate Result: ⚠️ SKIPPED | ||
|
|
||
| No tests were detected in this PR. | ||
|
|
||
| **Recommendation:** Add tests to verify the fix using the ``write-tests-agent``. | ||
| "@ | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 | ||
| } else { | ||
| $resultIcon = switch ($gateResult) { "PASSED" { "✅" } default { "❌" } } | ||
| @" | ||
| ### Gate Result: $resultIcon $gateResult | ||
|
|
||
| **Platform:** $($gatePlatform.ToUpper()) | ||
|
|
||
| <details> | ||
| <summary>Gate output log</summary> | ||
|
|
||
| `````` | ||
| $gateLogTail | ||
| `````` | ||
|
|
||
| </details> | ||
| "@ | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -566,8 +687,10 @@ $autonomousRules | |
|
|
||
| **Gate result (already completed in a prior step):** $gateStatusForPrompt | ||
| Do NOT re-run gate verification. The gate phase is handled separately. | ||
| ⚠️ Do NOT create or overwrite ``gate/content.md`` — it is already generated by the gate script with detailed test output. | ||
|
|
||
| 📁 Write phase output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/{phase}/content.md`` | ||
| (phases: pre-flight, try-fix, report — NOT gate) | ||
| "@ | ||
|
|
||
| Invoke-CopilotStep -StepName "STEP 2: PR REVIEW" -Prompt $step2Prompt | Out-Null | ||
|
|
||
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.
If we don't include this hardcoded list does this still work? Like if we add a link to the file with all the categories it seems like AI can just read through to that?