diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 04bc8f74b3..ab3116a508 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -185,6 +185,7 @@ jobs: name: trx-results path: | **/*.trx + **/TestResults/crash-retries.json if-no-files-found: warn - uses: actions/upload-artifact@v7 diff --git a/Lib/src/unit++/main.cc b/Lib/src/unit++/main.cc index 37c47eea46..742f92c14e 100644 --- a/Lib/src/unit++/main.cc +++ b/Lib/src/unit++/main.cc @@ -160,6 +160,13 @@ int main(int argc, const char* argv[]) retval = 1; } printf("DEBUG: unit++ main end (retval=%d)\n", retval); fflush(stdout); +#if defined(UNITPP_WINDOWS) + // The OS text-input stack loaded by text shaping never finishes teardown without a + // message pump, so end the process once tests and GlobalTeardown have run. Static + // destructors are skipped. + fflush(NULL); + ::TerminateProcess(::GetCurrentProcess(), static_cast(retval)); +#endif return retval; } diff --git a/Src/Common/RenderVerification/RenderSnapshotVerifier.cs b/Src/Common/RenderVerification/RenderSnapshotVerifier.cs index 7cf86e6588..3ff4464836 100644 --- a/Src/Common/RenderVerification/RenderSnapshotVerifier.cs +++ b/Src/Common/RenderVerification/RenderSnapshotVerifier.cs @@ -16,7 +16,11 @@ public static class RenderSnapshotVerifier { private const string UpdateBaselinesEnvVar = "FW_UPDATE_RENDER_BASELINES"; private const string FontQualityEnvVar = "FW_FONT_QUALITY"; - private const int MaxAllowedPixelDifferences = 4; + // Font-smoothing drift between machines peaks at 50 touched pixels and magnitude + // 1.57, three channels off about 8 levels each; a shifted glyph touches thousands + // at a magnitude near 1. + private const int MaxAllowedPixelDifferences = 100; + private const double MaxAllowedDifferenceMagnitude = 10.0; private const string DeterministicRenderFontFamily = "Segoe UI"; private const int DpiAwarenessInvalid = -1; private const int DpiAwarenessUnaware = 0; @@ -81,7 +85,11 @@ public static RenderBaselineVerificationResult Verify(Bitmap actualBitmap, strin { var savedArtifact = LoadSavedArtifact(expectedBitmap, verifiedPath, verifiedMetadataPath); var diffSummary = CompareBitmaps(expectedBitmap, actualBitmap); - if (diffSummary.DifferentPixelCount <= MaxAllowedPixelDifferences) + // A size change is a layout regression, so it fails regarldess of the pixel + // tolerance. + bool sizeMatches = expectedBitmap.Width == actualBitmap.Width + && expectedBitmap.Height == actualBitmap.Height; + if (sizeMatches && IsWithinTolerance(diffSummary)) { DeleteIfPresent(receivedPath); DeleteIfPresent(receivedMetadataPath); @@ -113,6 +121,7 @@ public static RenderBaselineVerificationResult Verify(Bitmap actualBitmap, strin ScenarioId = scenarioId, SnapshotName = name, AllowedDifferentPixelCount = MaxAllowedPixelDifferences, + AllowedDifferenceMagnitude = MaxAllowedDifferenceMagnitude, SavedBaseline = savedArtifact, CurrentRun = currentArtifact, Diff = diffSummary, @@ -299,11 +308,18 @@ private static string BuildFailureMessage( { var builder = new StringBuilder(); builder.AppendFormat(CultureInfo.InvariantCulture, - "Render output for '{0}' differed from baseline by {1} pixels; {2} or fewer differences are allowed.", + "Render output for '{0}' differed from baseline by {1} pixels ({2} full-pixel equivalents); fewer than {3} pixels and fewer than {4} full-pixel equivalents are allowed.", scenarioId, report.Diff.DifferentPixelCount, - report.AllowedDifferentPixelCount); + FormatMagnitude(report.Diff.DifferenceMagnitude), + report.AllowedDifferentPixelCount, + FormatMagnitude(report.AllowedDifferenceMagnitude)); builder.AppendLine(); + if (report.SavedBaseline.ImageWidth != report.CurrentRun.ImageWidth + || report.SavedBaseline.ImageHeight != report.CurrentRun.ImageHeight) + { + builder.AppendLine("Image size changed, which fails regardless of the pixel tolerance."); + } builder.AppendLine(FormatArtifactLine("Saved baseline", report.SavedBaseline)); builder.AppendLine(FormatArtifactLine("Current run", report.CurrentRun)); builder.AppendFormat(CultureInfo.InvariantCulture, @@ -530,6 +546,35 @@ private static void DeleteIfPresent(string path) File.Delete(path); } + /// + /// Answers whether a diff is small enough to be environment drift rather than a render + /// change. The touched-pixel count and the summed magnitude must both stay under their + /// limit. + /// + private static bool IsWithinTolerance(RenderPixelDiffSummary summary) + { + return summary.DifferentPixelCount < MaxAllowedPixelDifferences + && summary.DifferenceMagnitude < MaxAllowedDifferenceMagnitude; + } + + /// + /// Scores one differing pixel. All three channels fully inverted score 1, so a single + /// saturated channel scores a third and the magnitude limit admits three times as many + /// single-channel changes as whole-pixel ones. Alpha is not scored. + /// + private static double PixelDifferenceMagnitude(Color expected, Color actual) + { + int channelDelta = Math.Abs(expected.R - actual.R) + + Math.Abs(expected.G - actual.G) + + Math.Abs(expected.B - actual.B); + return channelDelta / (3.0 * 255.0); + } + + private static string FormatMagnitude(double magnitude) + { + return magnitude.ToString("0.##", CultureInfo.InvariantCulture); + } + private static RenderPixelDiffSummary CompareBitmaps(Bitmap expectedBitmap, Bitmap actualBitmap) { int maxWidth = Math.Max(expectedBitmap.Width, actualBitmap.Width); @@ -546,6 +591,7 @@ private static RenderPixelDiffSummary CompareBitmaps(Bitmap expectedBitmap, Bitm if (!expectedInBounds || !actualInBounds) { summary.DifferentPixelCount++; + summary.DifferenceMagnitude += 1.0; if (expectedInBounds) summary.ExpectedOnlyPixelDifferences++; else if (actualInBounds) @@ -554,11 +600,14 @@ private static RenderPixelDiffSummary CompareBitmaps(Bitmap expectedBitmap, Bitm continue; } - if (expectedBitmap.GetPixel(x, y) == actualBitmap.GetPixel(x, y)) + Color expectedPixel = expectedBitmap.GetPixel(x, y); + Color actualPixel = actualBitmap.GetPixel(x, y); + if (expectedPixel == actualPixel) continue; summary.DifferentPixelCount++; summary.InBoundsPixelDifferences++; + summary.DifferenceMagnitude += PixelDifferenceMagnitude(expectedPixel, actualPixel); UpdateDiffBounds(summary, x, y); } } @@ -670,6 +719,7 @@ public sealed class RenderSnapshotComparisonReport public string ScenarioId { get; set; } public string SnapshotName { get; set; } public int AllowedDifferentPixelCount { get; set; } + public double AllowedDifferenceMagnitude { get; set; } public RenderSnapshotArtifact SavedBaseline { get; set; } public RenderSnapshotArtifact CurrentRun { get; set; } public RenderPixelDiffSummary Diff { get; set; } @@ -679,6 +729,7 @@ public sealed class RenderSnapshotComparisonReport public sealed class RenderPixelDiffSummary { public int DifferentPixelCount { get; set; } + public double DifferenceMagnitude { get; set; } public int InBoundsPixelDifferences { get; set; } public int ExpectedOnlyPixelDifferences { get; set; } public int ActualOnlyPixelDifferences { get; set; } @@ -689,4 +740,4 @@ public sealed class RenderPixelDiffSummary public int DiffRegionWidth { get; set; } public int DiffRegionHeight { get; set; } } -} \ No newline at end of file +} diff --git a/Src/Common/RootSite/RootSiteTests/RenderVerifyTests.cs b/Src/Common/RootSite/RootSiteTests/RenderVerifyTests.cs index 893f1bef07..78279ced18 100644 --- a/Src/Common/RootSite/RootSiteTests/RenderVerifyTests.cs +++ b/Src/Common/RootSite/RootSiteTests/RenderVerifyTests.cs @@ -4,7 +4,6 @@ using System.Collections.Generic; using System.Threading; -using System.Threading.Tasks; using NUnit.Framework; using SIL.FieldWorks.Common.RootSites.RenderBenchmark; @@ -16,7 +15,7 @@ namespace SIL.FieldWorks.Common.RootSites /// Each run saves a .received.png and compares it against the committed /// .verified.png baseline by decoded pixel values, not by the PNG file bytes. /// Small encoder-level differences are therefore ignored as long as the rendered - /// image differs by fewer than five pixels. + /// image stays within the verifier's pixel-count and difference-magnitude tolerance. /// /// Each scenario is set up inside its own UndoableUnitOfWork, matching the pattern /// used by RenderTimingSuiteTests. @@ -40,13 +39,11 @@ protected override void CreateTestData() /// compares decoded pixels against the committed .verified.png baseline. /// [Test, TestCaseSource(nameof(GetVerifyScenarios))] - public async Task VerifyScenario(string scenarioId) + public void VerifyScenario(string scenarioId) { var execution = ExecuteScenarioAndCapture(scenarioId, includeWarmRender: false); if (!execution.Verification.Passed) Assert.Fail(execution.Verification.FailureMessage); - - await Task.CompletedTask; } /// diff --git a/test.ps1 b/test.ps1 index e68ad633cb..ed55bb1e7c 100644 --- a/test.ps1 +++ b/test.ps1 @@ -1,4 +1,4 @@ -<# +<# .SYNOPSIS Runs tests for the FieldWorks repository. @@ -64,6 +64,15 @@ (when installed; otherwise they run bare with a warning), writing native..cobertura.xml to the same TestResults folder. +.PARAMETER MaxCrashAttempts + How many times one managed test assembly is run when its test host crashes (default 5). + Only a crash is retried, never a reported test failure. A run that passed only after a + retry prints [FLAKY], writes TestResults/crash-retries.json, and exits with code 2. + +.PARAMETER Blame + Passes /Blame to vstest.console.exe so a host crash leaves a Sequence_*.xml naming the + test that was running. + .EXAMPLE .\test.ps1 Runs all tests in Debug configuration (builds first if needed). @@ -114,7 +123,10 @@ param( [ValidateSet('user', 'agent', 'unknown')] [string]$StartedBy = 'unknown', [switch]$CommentHygiene, - [switch]$TokenHygiene + [switch]$TokenHygiene, + [ValidateRange(1, 20)] + [int]$MaxCrashAttempts = 5, + [switch]$Blame ) $ErrorActionPreference = 'Stop' @@ -263,6 +275,59 @@ function Get-CentralPackageVersion { return $null } +function Test-HostCrashed { + param([string]$OutputText) + + # vstest also prints "The active test run was aborted" on Ctrl+C and CI cancellation, so only + # the crash line marks a run whose results are missing rather than failed. + return $OutputText -match 'Test host process crashed' +} + +function Invoke-VsTestWithCrashRetry { + param( + [string]$VsTestPath, + [string[]]$Arguments, + [string]$CoverageOutputPath, + [int]$MaxAttempts, + [string]$Label + ) + + # Only a crash is retried. A run that reported test failures is already an answer, and + # retrying it would turn a genuine failure into a false pass. + $attempt = 0 + $exitCode = 0 + $output = $null + $previousEap = $ErrorActionPreference + $ErrorActionPreference = 'Continue' + try { + while ($true) { + $attempt++ + if ($CoverageOutputPath) { + & dotnet tool run dotnet-coverage collect -f cobertura -o $CoverageOutputPath --nologo $VsTestPath @Arguments 2>&1 | Tee-Object -Variable output | Out-Host + } + else { + & $VsTestPath $Arguments 2>&1 | Tee-Object -Variable output | Out-Host + } + $exitCode = $LASTEXITCODE + + if ($exitCode -eq 0 -or $attempt -ge $MaxAttempts -or -not (Test-HostCrashed ($output | Out-String))) { + break + } + + Write-Host "[WARN] $Label crashed its test host (attempt $attempt of $MaxAttempts). Retrying; results so far are incomplete, not a pass." -ForegroundColor Yellow + } + } + finally { + $ErrorActionPreference = $previousEap + } + + return [pscustomobject]@{ + ExitCode = $exitCode + Output = $output + Attempts = $attempt + } +} + function Get-NUnitTestAdapterPaths { param( [string]$RepoRoot, @@ -326,6 +391,7 @@ $cleanupArgs = @{ } $testExitCode = 0 +$script:crashRetryReport = @() $script:coverageFailed = $false try { @@ -702,6 +768,10 @@ try { $vstestVerbosity = $verbosityMap[$Verbosity] $vstestArgs += "/Logger:trx" $vstestArgs += "/Logger:console;verbosity=$vstestVerbosity" + if ($Blame) { + # Records the test that was running when a host crashed, as Sequence_*.xml. + $vstestArgs += "/Blame" + } if ($TestFilter) { $vstestArgs += "/TestCaseFilter:$TestFilter" @@ -755,22 +825,22 @@ try { } Write-Host "" - $previousEap = $ErrorActionPreference - $ErrorActionPreference = 'Continue' - try { - if ($coverageOutputPath) { - & dotnet tool run dotnet-coverage collect -f cobertura -o $coverageOutputPath --nologo $vstestPath @vstestArgs 2>&1 | Tee-Object -Variable testOutput - } - else { - & $vstestPath $vstestArgs 2>&1 | Tee-Object -Variable testOutput - } - # Don't overwrite a non-zero exit code from native tests with a zero exit code from these tests. - if ($LASTEXITCODE -ne 0) { - $script:testExitCode = $LASTEXITCODE + # A multi-assembly run falls back to per-assembly runs below when a host crashes, so only + # a single-assembly run is retried here. + $mainAttempts = if ($testDlls.Count -eq 1) { $MaxCrashAttempts } else { 1 } + $mainRun = Invoke-VsTestWithCrashRetry -VsTestPath $vstestPath -Arguments $vstestArgs ` + -CoverageOutputPath $coverageOutputPath -MaxAttempts $mainAttempts -Label 'The test run' + $testOutput = $mainRun.Output + if ($mainRun.Attempts -gt 1) { + $script:crashRetryReport += [pscustomobject]@{ + Assembly = [System.IO.Path]::GetFileNameWithoutExtension($testDlls[0]) + Attempts = $mainRun.Attempts + Recovered = ($mainRun.ExitCode -eq 0) } } - finally { - $ErrorActionPreference = $previousEap + # Keep a non-zero exit code from native tests; a passing managed run must not clear it. + if ($mainRun.ExitCode -ne 0) { + $script:testExitCode = $mainRun.ExitCode } $vstestLogPath = Join-Path $resultsDir "vstest.console.log" @@ -782,19 +852,27 @@ try { Write-Host "[WARN] Failed to write VSTest output log to $vstestLogPath" -ForegroundColor Yellow } + $outputText = ($testOutput | Out-String) if ($script:testExitCode -ne 0) { - $outputText = ($testOutput | Out-String) if ($outputText -match 'used by another process|file is locked|cannot access the file') { throw "Detected possible file is locked during vstest execution." } } + # A crashed test host aborts the whole run, so the assemblies after it never report. + $hostCrashAborted = $script:testExitCode -ne 0 -and (Test-HostCrashed $outputText) + # ============================================================================= # Workaround: multi-assembly VSTest may fail with exit code -1 and minimal output # ============================================================================= - if (-not $ListTests -and $testDlls.Count -gt 1 -and $script:testExitCode -eq -1) { - Write-Host "[WARN] vstest.console.exe returned exit code -1 with multiple test assemblies. Retrying per-assembly to isolate failures." -ForegroundColor Yellow + if (-not $ListTests -and $testDlls.Count -gt 1 -and ($script:testExitCode -eq -1 -or $hostCrashAborted)) { + if ($hostCrashAborted) { + Write-Host "[WARN] A test host crashed and aborted the run. Retrying per-assembly so every assembly reports." -ForegroundColor Yellow + } + else { + Write-Host "[WARN] vstest.console.exe returned exit code -1 with multiple test assemblies. Retrying per-assembly to isolate failures." -ForegroundColor Yellow + } $timestamp = Get-Date -Format 'yyyyMMdd_HHmmss' $overallExitCode = 0 @@ -814,19 +892,30 @@ try { } $singleArgs += "/Logger:trx;LogFileName=${dllName}_${timestamp}.trx" $singleArgs += "/Logger:console;verbosity=$vstestVerbosity" + if ($Blame) { + $singleArgs += "/Blame" + } if ($TestFilter) { $singleArgs += "/TestCaseFilter:$TestFilter" } + $singleCoverageOutput = $null if ($coverageOutputPath) { $singleCoverageOutput = Join-Path $resultsDir "coverage.${dllName}.cobertura.xml" - & dotnet tool run dotnet-coverage collect -f cobertura -o $singleCoverageOutput --nologo $vstestPath @singleArgs 2>&1 | Tee-Object -Variable singleTestOutput } - else { - & $vstestPath $singleArgs 2>&1 | Tee-Object -Variable singleTestOutput + $singleRun = Invoke-VsTestWithCrashRetry -VsTestPath $vstestPath -Arguments $singleArgs ` + -CoverageOutputPath $singleCoverageOutput -MaxAttempts $MaxCrashAttempts -Label $dllName + $singleExitCode = $singleRun.ExitCode + $singleTestOutput = $singleRun.Output + if ($singleRun.Attempts -gt 1) { + $script:crashRetryReport += [pscustomobject]@{ + Assembly = $dllName + Attempts = $singleRun.Attempts + Recovered = ($singleExitCode -eq 0) + } } - $singleExitCode = $LASTEXITCODE + if ($singleExitCode -ne 0 -and $overallExitCode -eq 0) { $overallExitCode = $singleExitCode } @@ -961,9 +1050,53 @@ if ($script:coverageFailed -and $testExitCode -eq 0) { Write-Host "[FAIL] Tests passed but code coverage collection failed (see [ERROR] above)." -ForegroundColor Red } -if ($testExitCode -eq 0) { +# A crash that only passed on a retry is not a clean run, so it never reports as one. +if ($script:crashRetryReport.Count -gt 0) { + # $resultsDir was assigned inside the scriptblock that ran the tests, so it is out of + # scope here; rebuild the path as the failure summary above rebuilds its log path. + $crashReportPath = Join-Path $PSScriptRoot "Output/$Configuration/TestResults/crash-retries.json" + try { + $script:crashRetryReport | ConvertTo-Json -Depth 3 | Out-File -FilePath $crashReportPath -Encoding UTF8 + } + catch { + Write-Host "[WARN] Failed to write crash retry report to $crashReportPath" -ForegroundColor Yellow + } + + $crashLines = foreach ($entry in $script:crashRetryReport) { + $verdict = if ($entry.Recovered) { "passed on attempt $($entry.Attempts)" } else { "still failing after $($entry.Attempts) attempts" } + " {0}: {1}" -f $entry.Assembly, $verdict + } + Write-Host "" - Write-Host "[PASS] All tests passed" -ForegroundColor Green + Write-Host "========== FLAKY: TEST HOST CRASHED ==========" -ForegroundColor Magenta + $crashLines | ForEach-Object { Write-Host $_ -ForegroundColor Magenta } + Write-Host " This is a real defect. Retrying only recovered the results; it did not fix the crash." -ForegroundColor Magenta + Write-Host " Report: $crashReportPath" -ForegroundColor Magenta + Write-Host "==============================================" -ForegroundColor Magenta + + # GitHub Actions shows the step summary on the run page, where a retried crash stays visible. + if ($env:GITHUB_STEP_SUMMARY) { + try { + @("### Flaky: a test host crashed and was retried", "") + ($crashLines | ForEach-Object { "- " + $_.Trim() }) | + Out-File -FilePath $env:GITHUB_STEP_SUMMARY -Append -Encoding UTF8 + } + catch { + Write-Host "[WARN] Failed to append the crash retry report to the GitHub step summary" -ForegroundColor Yellow + } + } +} + +if ($testExitCode -eq 0) { + if ($script:crashRetryReport.Count -gt 0) { + # Exit code 2 keeps a recovered crash visible to gates that only read the exit code. + $testExitCode = 2 + Write-Host "" + Write-Host "[FLAKY] All tests passed, but a test host crashed and had to be retried (exit code: 2)" -ForegroundColor Magenta + } + else { + Write-Host "" + Write-Host "[PASS] All tests passed" -ForegroundColor Green + } } else { Write-Host ""