Fix integration test harness Docker detection + command timeout, and release-testing skill filter syntax#4244
Merged
mattleibow merged 1 commit intoJun 25, 2026
Conversation
…release-testing skill filter syntax Three reliability fixes found during 4.150.0-preview.2 release testing: - IsDockerAvailable() (LinuxConsoleTests.cs): drain stdout/stderr asynchronously before WaitForExit so a large `docker info` payload can't fill the OS pipe buffer and deadlock the child. Previously the 10s WaitForExit timed out and Docker was reported unavailable, so the Linux container tests were always silently skipped. - Run() helper (PlatformTestBase.cs): start both pipe reads first and enforce the timeout via WaitForExit while reads are in flight, then await the drained output. Previously ReadToEndAsync was awaited before WaitForExit, so the timeout was never enforced (a hung `docker build` ran unbounded) and reading stdout fully before stderr could deadlock. - release-testing skill (SKILL.md): replace stale VSTest `--filter "FullyQualifiedName~..."` syntax (silently ignored under Microsoft.Testing.Platform / xUnit v3 since #4143, running ALL tests) with MTP `--filter-class "*..."` args after `--`, keeping MSBuild `-p:` properties before the separator. Added a note explaining the migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4244PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4244"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4244/packages --name skiasharp-pr-4244More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4244 |
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
mattleibow
deleted the
mattleibow-fix-integration-test-harness-reliability
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three reliability fixes to the SkiaSharp integration test harness (
tests/SkiaSharp.Tests.Integration) and therelease-testingskill, all discovered during 4.150.0-preview.2 release testing.Fixes
1.
IsDockerAvailable()deadlock — Linux container tests always skippedLinuxConsoleTests.IsDockerAvailable()setRedirectStandardOutput/Error = truebut never drained the streams beforeWaitForExit(10000).docker infocan emit more than the 64KB OS pipe buffer, so the child blocked on write while the parent's 10sWaitForExitreturnedfalse→ExitCodeaccess failed → Docker was reported unavailable even when running. Net effect: the Linux container tests were always silently skipped.Fix: start
ReadToEndAsync()on both pipes beforeWaitForExit, kill on timeout, thenTask.WaitAllthe reads so a large payload can't deadlock the child.2.
Run()helper timeout never enforcedPlatformTestBase.Run(...)awaitedStandardOutput.ReadToEndAsync()/StandardError.ReadToEndAsync()beforeWaitForExit(timeoutSeconds * 1000).ReadToEndAsynconly completes when the stream closes (process exit), so the await blocked until the process finished — the timeout was effectively never enforced (a hungdocker buildran unbounded). Fully reading stdout before starting stderr could also deadlock if stderr filled.Fix: start both pipe reads first, enforce the timeout via
WaitForExitwhile reads are in flight (kill on timeout), thenawaitthe drained output. The rest of the method (combined output, exit-code check, return) is unchanged.3. release-testing skill — stale
--filtersyntax ignored under MTPThe project migrated to Microsoft.Testing.Platform (xUnit v3) in #4143. Under MTP the VSTest-style
dotnet test --filter "FullyQualifiedName~X"is silently ignored and runs ALL tests. The skill's Test Commands still used that syntax.Fix: updated every occurrence to the MTP form —
dotnet test ... -- --filter-class "*X"— keeping MSBuild-p:properties (-p:SkiaSharpVersion=,-p:iOSDevice=,-p:AndroidDeviceId=, etc.) before the--separator and only the test-platform filter args after it. Added a note explaining that this project uses MTP so the legacy--filter "FullyQualifiedName~..."syntax must be replaced by--filter-class/--filter-method/--filter-namespace(after--).Verification
dotnet build tests/SkiaSharp.Tests.Integration/SkiaSharp.Tests.Integration.csprojsucceeds (0 errors; only pre-existing obsolete-API / xUnit analyzer warnings).