fix: uninstall temporary template packs in test teardown#420
fix: uninstall temporary template packs in test teardown#420jongalloway merged 3 commits intomainfrom
Conversation
…tover registrations Agent-Logs-Url: https://github.com/jongalloway/dotnet-mcp/sessions/79914a19-ec75-4f22-aa0b-6d5487ba0584 Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces test pollution from dotnet new install <tempDir> by ensuring temporary template-pack paths are unregistered from the global .NET template engine during teardown, preventing persistent “Failed to scan ” warnings in later dotnet new invocations.
Changes:
- In
DotnetSdk_InstallTemplatePack_WithValidPackageIdInline_Succeeds, uninstall the temp template pack path before deleting the temp directory. - Add explanatory comments describing the warning symptom and why uninstall must precede directory deletion.
| // Uninstall the template pack before deleting the directory so the .NET template engine | ||
| // does not retain a registration for a path that no longer exists. Without this step, | ||
| // subsequent `dotnet new` invocations emit "Failed to scan <path>" warnings for every | ||
| // abandoned temporary registration left behind by this test. | ||
| try | ||
| { | ||
| await _tools.DotnetSdk( | ||
| action: DotnetSdkAction.UninstallTemplatePack, | ||
| templatePackage: tempDir); | ||
| } | ||
| catch (Exception) { /* best-effort uninstall */ } |
There was a problem hiding this comment.
This teardown now uninstalls the temp template pack for this test, but there are other tests that call DotnetSdk(InstallTemplatePack, templatePackage: <tempDir>) and only Directory.Delete(...) the folder (e.g., SdkAndServerInfoToolsTests.DotnetSdk_TemplatePackInstall_BuildsCorrectCommand), which can still leave orphaned template-engine registrations and keep the dotnet new scan warnings around. Consider extracting a small helper for "install-temp-dir cleanup" (uninstall then delete) and using it in every test that installs a temp directory template pack.
|
@copilot address all comments |
…emplate pack install tests Agent-Logs-Url: https://github.com/jongalloway/dotnet-mcp/sessions/85b84403-acde-49c7-a5cd-346e3f85ee61 Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Done in commit 16cf18e. Extracted a
Each test now uses |
| action: DotnetSdkAction.UninstallTemplatePack, | ||
| templatePackage: Path); | ||
| } | ||
| catch (Exception) { /* best-effort uninstall */ } |
Check notice
Code scanning / CodeQL
Generic catch clause Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, to fix a generic catch clause, you either (1) catch only the specific exception types you truly expect and know how to handle, or (2) if you must have a broad catch for robustness, exclude truly critical exceptions (like OperationCanceledException in async paths) and at least log or rethrow unexpected cases.
Here, DisposeAsync is intended to be best-effort cleanup, so we should keep it resilient but avoid catching everything. A good compromise without changing functionality is:
- Narrow the catch to non-fatal, expected operational exceptions that likely arise from a failed CLI call: e.g.,
InvalidOperationException,IOException,UnauthorizedAccessException. - Optionally also catch
OperationCanceledExceptionseparately if we want to treat cancellation differently, but since this is disposal in test code, we can simply let cancellation bubble out instead of swallowing it silently.
Because the current behavior is “ignore failures”, we will keep ignoring (no logging) for the chosen exception types to preserve behavior, but we will not catch all Exception anymore. Concretely:
- In
DotNetMcp.Tests/Infrastructure/TempTemplatePackDirectory.cs, edit theDisposeAsyncmethod’s firsttry/catch:- Replace
catch (Exception) { /* best-effort uninstall */ } - With several more specific catches, for example:
catch (IOException) { /* best-effort uninstall */ }catch (UnauthorizedAccessException) { /* best-effort uninstall */ }catch (InvalidOperationException) { /* best-effort uninstall */ }
- Replace
No new methods or imports are needed; IOException, UnauthorizedAccessException, and InvalidOperationException are already available from the base class library and used elsewhere in the file.
| @@ -57,7 +57,9 @@ | ||
| action: DotnetSdkAction.UninstallTemplatePack, | ||
| templatePackage: Path); | ||
| } | ||
| catch (Exception) { /* best-effort uninstall */ } | ||
| catch (IOException) { /* best-effort uninstall */ } | ||
| catch (UnauthorizedAccessException) { /* best-effort uninstall */ } | ||
| catch (InvalidOperationException) { /* best-effort uninstall */ } | ||
|
|
||
| try | ||
| { |
|
@copilot review comments |
All PR review comments have been addressed. The reviewer's suggestion to extract a shared cleanup helper is implemented in commit |
dotnet new installregisters a path with the global template engine even if the directory contains no valid templates. Three tests were installing temp directories as template packs but only deleting them on teardown — leaving orphaned registrations that causedotnet newto emit warnings on every subsequent invocation:Change
Extracted a
TempTemplatePackDirectoryhelper (DotNetMcp.Tests/Infrastructure/TempTemplatePackDirectory.cs) — anIAsyncDisposablethat holds a temp directory path and onDisposeAsynccallsdotnet new uninstall <path>(best-effort) before deleting the directory.Applied to all three affected tests using
await using var temp = TempTemplatePackDirectory.Create(_tools, prefix):ConsolidatedSdkToolTests.DotnetSdk_InstallTemplatePack_WithVersion_UsesAtSymbolConsolidatedSdkToolTests.DotnetSdk_InstallTemplatePack_WithValidPackageIdInline_SucceedsSdkAndServerInfoToolsTests.DotnetSdk_TemplatePackInstall_BuildsCorrectCommand⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.