Xunit 3 upgrade hang fix#1113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes xUnit 3 test hang issues in the PowerShell integration by adding proper runspace pool shutdown hooks and refactoring the initialization pattern to use modern Aspire helper methods.
Changes:
- Added
IHostApplicationLifetimehook to close PowerShell runspace pools on application shutdown, preventing xUnit 3 from hanging on open foreground threads - Refactored initialization pattern to use
OnInitializeResourcehelper method instead of raw event subscriptions for improved clarity - Removed flawed script completion tracking logic that had inverted conditional logic
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CommunityToolkit.Aspire.Hosting.PowerShell/PowerShellRunspacePoolResource.cs | Added IHostApplicationLifetime parameter and shutdown hook registration to close runspace pool on application stop; removed flawed script tracking mechanism |
| src/CommunityToolkit.Aspire.Hosting.PowerShell/PowerShellRunspacePoolResourceBuilderExtensions.cs | Refactored script initialization to use OnInitializeResource helper; removed manual script disposal and completion tracking; removed unused System.Drawing import |
| src/CommunityToolkit.Aspire.Hosting.PowerShell/DistributedApplicationBuilderExtensions.cs | Refactored pool initialization pattern using OnInitializeResource helper; reordered code for clarity; added debugger language mode support |
| tests/CommunityToolkit.Aspire.Hosting.PowerShell.Tests/AppHostTests.cs | Removed redundant assertions that are covered by Task.WhenAll behavior |
| examples/powershell/CommunityToolkit.Aspire.PowerShell.AppHost/Properties/launchSettings.json | Added DCP debug logging environment variable |
| examples/powershell/CommunityToolkit.Aspire.PowerShell.AppHost/Program.cs | Added Azurite workaround for known Aspire issue; minor formatting change |
Comments suppressed due to low confidence (1)
src/CommunityToolkit.Aspire.Hosting.PowerShell/PowerShellRunspacePoolResource.cs:46
- The condition logic is inverted. The code checks
ScriptsCompleted(which returnstruewhen any script hasfalsevalue, i.e., NOT completed), but then publishes an update marking the resource as Finished. This appears to be a bug in the removed code that was being fixed.
The removed tracking logic had scriptResourceCompletion.Any(r => r.Value == false) which returns true if any script is NOT completed, but the name ScriptsCompleted suggests it should return true when all scripts ARE completed. This was a logical error that has now been corrected by removing the flawed tracking mechanism.
internal Task StartAsync(InitialSessionState sessionState, ResourceNotificationService notificationService, ILogger logger, IHostApplicationLifetime hostLifetime, CancellationToken token = default)
{
logger.LogInformation(
"Starting PowerShell runspace pool '{PoolName}' with {MinRunspaces} to {MaxRunspaces} runspaces",
Name, MinRunspaces, MaxRunspaces);
examples/powershell/CommunityToolkit.Aspire.PowerShell.AppHost/Properties/launchSettings.json
Show resolved
Hide resolved
| .WithArgs(2, 3) | ||
| .WaitForCompletion(script1); | ||
|
|
||
|
|
There was a problem hiding this comment.
There's an extra blank line at the end of the file. While this is a minor style issue, it's good practice to maintain consistent formatting without trailing blank lines unless required by the project's style guide.
src/CommunityToolkit.Aspire.Hosting.PowerShell/PowerShellRunspacePoolResource.cs
Show resolved
Hide resolved
15e7108
into
CommunityToolkit:xunit-3-upgrade
Closes #<ISSUE_NUMBER>
PR Checklist
Other information