From 1833099cbe5e2ed459bc3cc38a4da02bc7c936a8 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Sat, 22 Aug 2026 01:55:04 +0000 Subject: [PATCH] chore: buildless CodeQL and genuine fixes for the dismissed alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts NextIteration.Standards §4.4 (build-mode: none) and replaces every alert dismissal made earlier with a real code change. §4.4 / §3.0.1 — codeql.yml now analyses C# buildless. GitHub applies paths-ignore to a compiled language only when it is analysed without a build, so the mandated **/obj/** exclusion was silently inert and the xUnit auto-generated entry point was analysed and flagged anyway. build-mode: none makes the exclusion effective and reads source across every TFM at once, so the Setup .NET / Restore / Build steps are removed. This genuinely resolves the two cs/missed-ternary-operator alerts rather than dismissing them. cs/catch-of-all-exceptions (19) — each catch now takes only what its operation can produce: - filesystem best-effort: IOException, UnauthorizedAccessException - cache file: + JsonException - archive extraction: + InvalidDataException, NotSupportedException - HTTP sources / checker: HttpRequestException, JsonException, IOException, OperationCanceledException - gh source: GhProcessException in place of the HTTP pair - process kill: InvalidOperationException, Win32Exception, NotSupportedException - UpdateCheckCommand: now identical to UpdateCommand's already-accepted `when (ex is not OperationCanceledException)` — a command boundary still turns any failure into a message and exit code, but lets Ctrl-C through The two UpdateInstaller catches that catch broadly and rethrow are unchanged: they roll back state rather than swallow, and CodeQL does not flag them. cs/local-not-disposed (3) — the command harnesses hand their TestConsole to the caller, which reads console.Output after the harness returns, so it cannot be disposed at the creation site. Each test class now implements IDisposable and disposes every console it handed out at teardown. cs/useless-upcast (1) — typed locals pin which UpdateCleanup.Run overload each null-argument test targets, with no upcast expression at the call site. Narrowing surfaced two tests that encoded the old swallow-everything contract. GetLatestAsync_when_runner_throws_returns_null threw InvalidOperationException, which the real gh transport never produces; it now throws GhProcessException (what GhProcess actually wraps failures in) and a companion test asserts an unexpected exception propagates instead of being reported as "no release". This is a deliberate behaviour change: a defect that used to be masked as "no update available" now surfaces. Release build: 0 warnings. 410 tests (205 × net8.0/net10.0) pass, up from 408. CHANGELOG entries land under the unpublished [1.0.0] section, whose date moves to 2026-08-22; no tag exists and nothing has been published. Co-Authored-By: Claude Opus 5 --- .github/workflows/codeql.yml | 38 ++++++++++--------- CHANGELOG.md | 10 ++++- .../Commands/UpdateCheckCommand.cs | 5 ++- .../Pipeline/ArchiveExtractor.cs | 3 +- .../Pipeline/UpdateCacheFile.cs | 6 ++- .../Pipeline/UpdateChecker.cs | 8 +++- .../Pipeline/UpdateInstaller.cs | 15 ++++---- .../Sources/GhCliReleaseSource.cs | 8 +++- .../Sources/GhProcess.cs | 6 ++- .../Sources/HttpGitHubReleaseSource.cs | 6 ++- .../Sources/HttpManifestSource.cs | 6 ++- .../UpdateBanner.cs | 6 ++- .../CommandConfiguratorExtensionsTests.cs | 23 ++++++++--- .../Commands/UpdateCheckCommandTests.cs | 23 ++++++++--- .../Commands/UpdateCommandTests.cs | 23 ++++++++--- .../Infrastructure/TempDir.cs | 2 +- .../Pipeline/InstallLockTests.cs | 5 ++- .../Sources/GhCliReleaseSourceTests.cs | 20 +++++++++- .../UpdateCleanupTests.cs | 14 ++++++- 19 files changed, 169 insertions(+), 58 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 4e37bc7..776630f 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -32,23 +32,36 @@ jobs: - name: Checkout uses: actions/checkout@v7 - - name: Setup .NET - uses: actions/setup-dotnet@v6 - with: - dotnet-version: | - 8.0.x - 10.0.x - - name: Initialize CodeQL uses: github/codeql-action/init@v4 with: languages: csharp + # build-mode: none analyses the C# source directly, without a build. + # It is load-bearing, not a convenience, for two reasons: + # + # 1. paths-ignore (below) only takes effect in this mode. When CodeQL + # builds a compiled language, GitHub applies no path filter — every + # file the compiler sees is analysed, obj/ included — so under the + # explicit build this workflow used to run, paths-ignore was + # silently inert and the xUnit auto-generated entry point in obj/ + # was analysed and flagged in every repo. Buildless extraction + # honours the filter, so the exclusion the standard mandates + # actually happens. + # + # 2. It reads the source across every target framework at once. These + # repos multi-target, and autobuild has picked a single TFM in the + # past, silently analysing half the code; the explicit build existed + # to guard against that. Buildless extraction reads the source + # itself, not one TFM's build output, so it covers all of it with no + # build step to get wrong. + build-mode: none # security-and-quality is broader than the default security-extended; # these are small libraries, so the extra findings are affordable. queries: security-and-quality # Analyse source only. obj/ and bin/ hold generated and compiled # output — e.g. the xUnit auto-generated entry point — so findings - # there are noise against code no human maintains. + # there are noise against code no human maintains. Effective only + # under build-mode: none (above). # # query-filters excludes the two audit queries that fire on every # P/Invoke declaration and call site (cs/unmanaged-code, @@ -68,15 +81,6 @@ jobs: - exclude: id: cs/call-to-unmanaged-code - # Explicit build rather than autobuild: these repos multi-target, and - # autobuild has picked a single TFM in the past, silently analysing half - # the code. Restore is separate so a restore failure is legible. - - name: Restore - run: dotnet restore - - - name: Build - run: dotnet build --configuration Release --no-restore - - name: Perform CodeQL analysis uses: github/codeql-action/analyze@v4 with: diff --git a/CHANGELOG.md b/CHANGELOG.md index a1656d5..93e3099 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 --- -## [1.0.0] — 2026-08-21 +## [1.0.0] — 2026-08-22 First stable release. The public surface is now covered by Semantic Versioning: a breaking change to it requires a 2.0.0. @@ -46,6 +46,14 @@ the override-carrying method, so none of them changed behaviour. ### Changed +- **Exception handling narrowed across the pipeline — unexpected exceptions now surface instead of being swallowed.** Nineteen `catch`-everything blocks caught any exception, so a genuine defect anywhere in the update path was silently reported as "no update available", "already up to date", or nothing at all, and could persist indefinitely. Each now catches only what its operation can actually produce: filesystem best-effort helpers take `IOException`/`UnauthorizedAccessException`, the cache file adds `JsonException`, archive extraction adds `InvalidDataException`/`NotSupportedException`, the HTTP sources take `HttpRequestException`/`JsonException`/`IOException`/`OperationCanceledException`, the `gh` source takes `GhProcessException` in place of the HTTP pair, and process termination takes `InvalidOperationException`/`Win32Exception`/`NotSupportedException`. The rollback paths in `UpdateInstaller` that catch broadly and *rethrow* are unchanged — they restore state rather than swallow. `UpdateCheckCommand` now matches `UpdateCommand` exactly (`when (ex is not OperationCanceledException)`): a command boundary still turns any failure into a message and an exit code, but lets Ctrl-C through rather than reporting it as a failed check. **This is a deliberate behaviour change**: a bug that used to be masked will now surface as an unhandled exception. + +- **CodeQL analyses C# buildless** (NextIteration.Standards §4.4). GitHub applies `paths-ignore` to a compiled language *only* when it is analysed without a build, so the mandated `**/obj/**` exclusion was silently inert under the explicit build and the xUnit auto-generated entry point was analysed and flagged anyway. `build-mode: none` makes the exclusion take effect, and buildless extraction reads source across every target framework at once — which is what the explicit build existed to guarantee — so the Setup .NET, Restore and Build steps are gone. + +- **Test consoles are owned by the fixture.** The three command harnesses hand their `TestConsole` to the caller, which reads `console.Output` after the harness returns, so it cannot be disposed at the creation site. Each test class now implements `IDisposable` and disposes every console it handed out at teardown, which disposes them properly rather than documenting why they were left undisposed. + +- **Typed locals replace upcast arguments** in the `UpdateCleanup.Run` null-argument tests. `Run` is overloaded on `IServiceProvider` and `IUpdateInstaller`; a typed local pins which overload each test targets without an upcast expression at the call site. + - **The prerelease-override overloads were inverted on all three interfaces** — see the breaking-change note above. A regression test (`InterfaceDefaultsTests`) implements each interface with *only* its abstract member and asserts the override reaches it, so if the abstract member ever moves back to the no-override overload the test project stops compiling. - **Adopted the revised canonical `.editorconfig` and enabled `EnforceCodeStyleInBuild`** (NextIteration.Standards §5.2, §1.2.1 — the latter now a `MUST`). The canonical file is a deliberate allow-list of gated style rules rather than a blanket `dotnet_analyzer_diagnostic.severity`, so a style rule a future SDK ships never auto-gates the build. With the flag on, the gated rules fail the build under `TreatWarningsAsErrors` instead of merely showing in the IDE. Bringing the code green was a mechanical, behaviour-preserving reformat of 92 sites — braces on all single-statement `if`s (IDE0011, 64 of them), collection expressions (IDE0300/IDE0301/IDE0028), `var` usage, two expression-bodied members, one simplified null check, and five unnecessary usings — applied with `dotnet format` plus the collection-expression sites it cannot fix automatically. All 392 tests (196 × `net8.0`/`net10.0`) pass unchanged, and the build stays at zero warnings. diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCheckCommand.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCheckCommand.cs index a929351..2ef7821 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCheckCommand.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCheckCommand.cs @@ -60,7 +60,10 @@ protected override async Task ExecuteAsync(CommandContext context, Settings { info = await _checker.CheckAsync(prereleaseOverride, cancellationToken).ConfigureAwait(false); } - catch (Exception ex) + // Mirrors UpdateCommand: a command boundary turns any failure into a + // message and an exit code, but must let cancellation through so + // Ctrl-C is not reported as "could not determine the latest release". + catch (Exception ex) when (ex is not OperationCanceledException) { _console.MarkupLineInterpolated(CultureInfo.InvariantCulture, $"[red]Could not determine the latest release:[/] {ex.Message}"); diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/ArchiveExtractor.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/ArchiveExtractor.cs index c8af63f..3e7e609 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/ArchiveExtractor.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/ArchiveExtractor.cs @@ -39,7 +39,8 @@ public static async Task ExtractAsync(string archivePath, string destinationDire { throw; } - catch (Exception ex) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException + or InvalidDataException or NotSupportedException) { throw new UpdateException($"Failed to extract '{Path.GetFileName(archivePath)}': {ex.Message}", ex); } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateCacheFile.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateCacheFile.cs index 2263e09..e6558c5 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateCacheFile.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateCacheFile.cs @@ -30,8 +30,10 @@ internal static class UpdateCacheFile var json = File.ReadAllText(path); return JsonSerializer.Deserialize(json, JsonOpts); } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or JsonException) { + // A missing, unreadable or corrupt cache is not an error — the + // caller falls back to querying the source. return null; } } @@ -43,7 +45,7 @@ public static void TryWrite(string path, UpdateCacheEntry entry) Directory.CreateDirectory(Path.GetDirectoryName(path)!); File.WriteAllText(path, JsonSerializer.Serialize(entry, JsonOpts)); } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or JsonException) { // Cache-write failures are non-fatal — the source will be // hit again on the next invocation. diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateChecker.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateChecker.cs index 01e528f..ecccbe4 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateChecker.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateChecker.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using System.Reflection; using System.Text; @@ -98,8 +99,13 @@ internal UpdateChecker( { throw; } - catch (Exception) + catch (Exception ex) when (ex is UpdateException or HttpRequestException + or IOException or JsonException or OperationCanceledException) { + // A source that is unreachable, slow or serving junk means "no + // upgrade information; try again next tick". Anything else is a + // bug and propagates rather than being silently reported as + // "up to date". return null; } } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs index 2e0e78c..16fb1db 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs @@ -150,9 +150,10 @@ public void CleanupOldInstall() private static void TrySwallow(Action action) { try { action(); } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - // Non-fatal — will retry on next startup. + // Non-fatal — will retry on next startup. Only filesystem + // contention is swallowed; anything else is a bug and propagates. } } @@ -398,7 +399,7 @@ internal static void RestoreFromOld(string oldDirectory, string installDirectory Directory.Move(src, dest); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort — partial-restore failures are surfaced // implicitly through the pipeline exception. @@ -419,7 +420,7 @@ private static void TryDeleteEntry(string path) DeleteDirectoryRobustly(path); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort. } @@ -488,7 +489,7 @@ private static void ClearReadOnlyAttributes(string path) } } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort — if we can't clear an attribute (e.g. the file // is locked), the subsequent Delete will surface the real @@ -565,7 +566,7 @@ private static void ResetStaging(string stagingDir) { DeleteDirectoryRobustly(stagingDir); } - catch (Exception ex) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { throw new UpdateException( $"Unable to reset staging directory '{stagingDir}': {ex.Message}", ex); @@ -579,7 +580,7 @@ private static void TryDeleteDirectory(string path) { DeleteDirectoryRobustly(path); } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort — staging is under .update/ which the next install or // a subsequent CleanupOldInstall pass will eventually overwrite. diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhCliReleaseSource.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhCliReleaseSource.cs index ff2d7f3..e87a5ec 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhCliReleaseSource.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhCliReleaseSource.cs @@ -140,9 +140,13 @@ internal GhCliReleaseSource( { throw; } - catch (Exception) + catch (Exception ex) when (ex is GhProcessException or JsonException + or IOException or OperationCanceledException) { // Source contract: swallow transient failures and return null. + // Narrowed to the transport/parse failures a source can actually + // hit — an unexpected exception is a bug and now propagates + // instead of being reported as "no update available". return null; } } @@ -197,7 +201,7 @@ private static void TryDelete(string path) File.Delete(path); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort. } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhProcess.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhProcess.cs index 8bffdae..2cdfca7 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhProcess.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/GhProcess.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.Diagnostics; namespace NextIteration.SpectreConsole.SelfUpdate.Sources @@ -79,9 +80,10 @@ public static async Task RunCaptureStdoutAsync(IReadOnlyList arg private static void TryKill(Process proc) { try { proc.Kill(entireProcessTree: true); } - catch + catch (Exception ex) when (ex is InvalidOperationException or Win32Exception or NotSupportedException) { - // Best effort — the process may already have exited. + // Best effort — the process may already have exited + // (InvalidOperationException) or refused the signal. } } } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpGitHubReleaseSource.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpGitHubReleaseSource.cs index 1a30a0c..ae13fa2 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpGitHubReleaseSource.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpGitHubReleaseSource.cs @@ -120,9 +120,13 @@ internal HttpGitHubReleaseSource( { throw; } - catch (Exception) + catch (Exception ex) when (ex is HttpRequestException or JsonException + or IOException or OperationCanceledException) { // Source contract: swallow transient failures and return null. + // Narrowed to the transport/parse failures a source can actually + // hit — an unexpected exception is a bug and now propagates + // instead of being reported as "no update available". return null; } } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs index 6c6f935..07dc59a 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs @@ -137,9 +137,13 @@ private static bool IsHttps(Uri uri) => { throw; } - catch (Exception) + catch (Exception ex) when (ex is HttpRequestException or JsonException + or IOException or OperationCanceledException) { // Source contract: swallow transient failures and return null. + // Narrowed to the transport/parse failures a source can actually + // hit — an unexpected exception is a bug and now propagates + // instead of being reported as "no update available". return null; } } diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/UpdateBanner.cs b/src/NextIteration.SpectreConsole.SelfUpdate/UpdateBanner.cs index e9dc02d..871c949 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/UpdateBanner.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/UpdateBanner.cs @@ -64,8 +64,12 @@ public static void RenderIfAvailable( } info = checkTask.GetAwaiter().GetResult(); } - catch + catch (Exception ex) when (ex is AggregateException or UpdateException + or HttpRequestException or IOException or OperationCanceledException) { + // The banner is decoration: a check that failed or timed out + // simply renders nothing. Task.Wait surfaces a fault as an + // AggregateException, hence its presence here. return; } if (info is null || !info.IsUpdateAvailable) diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/CommandConfiguratorExtensionsTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/CommandConfiguratorExtensionsTests.cs index b9b3d66..95abaeb 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/CommandConfiguratorExtensionsTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/CommandConfiguratorExtensionsTests.cs @@ -11,8 +11,23 @@ namespace NextIteration.SpectreConsole.SelfUpdate.Tests { - public sealed class CommandConfiguratorExtensionsTests + public sealed class CommandConfiguratorExtensionsTests : IDisposable { + // The harness below hands its TestConsole to the caller, which reads + // console.Output after the harness method has returned — so the console + // cannot be disposed at its creation site. The fixture owns the lifetime + // instead and disposes every console it handed out when xUnit tears the + // class down. + private readonly List _consoles = []; + + public void Dispose() + { + foreach (var console in _consoles) + { + console.Dispose(); + } + } + private static readonly string[] HelpArgs = ["--help"]; private static readonly string[] UpdateHelpArgs = ["update", "--help"]; private static readonly string[] OtaHelpArgs = ["ota", "--help"]; @@ -84,12 +99,10 @@ public void AddUpdateBranch_rejects_blank_name() // ---------- helpers ---------- - private static (CommandApp App, TestConsole Console) BuildApp(Action configure) + private (CommandApp App, TestConsole Console) BuildApp(Action configure) { - // Deliberately not `using`: this console escapes via the return - // value and its lifetime belongs to the caller, which reads - // console.Output after the harness method has returned. var console = new TestConsole(); + _consoles.Add(console); var registrar = new TestRegistrar(s => { s.AddSingleton(console); diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCheckCommandTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCheckCommandTests.cs index 48a45ca..eb5f82b 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCheckCommandTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCheckCommandTests.cs @@ -11,8 +11,23 @@ namespace NextIteration.SpectreConsole.SelfUpdate.Tests.Commands { - public sealed class UpdateCheckCommandTests + public sealed class UpdateCheckCommandTests : IDisposable { + // The harness below hands its TestConsole to the caller, which reads + // console.Output after the harness method has returned — so the console + // cannot be disposed at its creation site. The fixture owns the lifetime + // instead and disposes every console it handed out when xUnit tears the + // class down. + private readonly List _consoles = []; + + public void Dispose() + { + foreach (var console in _consoles) + { + console.Dispose(); + } + } + private delegate Task Runner(params string[] args); [Fact] @@ -123,14 +138,12 @@ public async Task Execute_when_no_release_url_omits_release_notes_line() // ---------- helpers ---------- - private static (Runner Run, TestConsole Console, StubUpdateChecker Checker) BuildHarness(Action configChecker) + private (Runner Run, TestConsole Console, StubUpdateChecker Checker) BuildHarness(Action configChecker) { var checker = new StubUpdateChecker(); configChecker(checker); - // Deliberately not `using`: this console escapes via the return - // value and its lifetime belongs to the caller, which reads - // console.Output after the harness method has returned. var console = new TestConsole(); + _consoles.Add(console); var registrar = new TestRegistrar(s => { diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs index 714c1a9..c94c2bc 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs @@ -11,8 +11,23 @@ namespace NextIteration.SpectreConsole.SelfUpdate.Tests.Commands { - public sealed class UpdateCommandTests + public sealed class UpdateCommandTests : IDisposable { + // The harness below hands its TestConsole to the caller, which reads + // console.Output after the harness method has returned — so the console + // cannot be disposed at its creation site. The fixture owns the lifetime + // instead and disposes every console it handed out when xUnit tears the + // class down. + private readonly List _consoles = []; + + public void Dispose() + { + foreach (var console in _consoles) + { + console.Dispose(); + } + } + private delegate Task Runner(params string[] args); private static readonly RemoteRelease ReleaseV142 = new( @@ -246,7 +261,7 @@ public async Task Execute_prints_current_and_latest_versions() // ---------- helpers ---------- - private static (Runner Run, TestConsole Console, StubSelfUpdater Updater) BuildHarness( + private (Runner Run, TestConsole Console, StubSelfUpdater Updater) BuildHarness( Action? configChecker = null, Action? configUpdater = null, Action? configInstaller = null) @@ -257,10 +272,8 @@ private static (Runner Run, TestConsole Console, StubSelfUpdater Updater) BuildH configUpdater?.Invoke(updater); var installer = new StubUpdateInstaller(); configInstaller?.Invoke(installer); - // Deliberately not `using`: this console escapes via the return - // value and its lifetime belongs to the caller, which reads - // console.Output after the harness method has returned. var console = new TestConsole(); + _consoles.Add(console); var registrar = new TestRegistrar(s => { diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/TempDir.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/TempDir.cs index aeceade..fa44649 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/TempDir.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/TempDir.cs @@ -30,7 +30,7 @@ public void Dispose() Directory.Delete(Path, recursive: true); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best effort. } diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/InstallLockTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/InstallLockTests.cs index 068d7df..f845cc3 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/InstallLockTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/InstallLockTests.cs @@ -85,7 +85,10 @@ public void Acquire_when_directory_not_writable_throws_not_writable() { // Restore writability so TempDir.Dispose() can clean up. try { File.SetUnixFileMode(sub, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); } - catch { /* best effort */ } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // Best effort. + } } } } diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/GhCliReleaseSourceTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/GhCliReleaseSourceTests.cs index 9ecd5fc..bbdfef1 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/GhCliReleaseSourceTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/GhCliReleaseSourceTests.cs @@ -127,17 +127,33 @@ public async Task GetLatestAsync_when_channel_does_not_match_returns_null() Assert.Null(release); } + // GhProcess wraps every real `gh` failure — missing binary, non-zero + // exit, timeout — in GhProcessException, so that is what a transport + // failure looks like in production. [Fact] - public async Task GetLatestAsync_when_runner_throws_returns_null() + public async Task GetLatestAsync_when_runner_reports_transport_failure_returns_null() { var source = new GhCliReleaseSource(Repo, includePrereleases: false, - runner: (_, _, _) => throw new InvalidOperationException("gh missing")); + runner: (_, _, _) => throw new GhProcessException("gh missing")); var release = await source.GetLatestAsync(channel: null, CancellationToken.None); Assert.Null(release); } + // The source swallows transport failures, not bugs. An exception the gh + // transport cannot produce propagates rather than being reported as + // "no release available", which would hide the defect indefinitely. + [Fact] + public async Task GetLatestAsync_when_runner_throws_unexpected_exception_propagates() + { + var source = new GhCliReleaseSource(Repo, includePrereleases: false, + runner: (_, _, _) => throw new InvalidOperationException("bug in a custom runner")); + + await Assert.ThrowsAsync( + () => source.GetLatestAsync(channel: null, CancellationToken.None)); + } + [Fact] public async Task DownloadAssetAsync_when_metadata_missing_tag_throws() { diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/UpdateCleanupTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/UpdateCleanupTests.cs index 7326f9e..4e09f8b 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/UpdateCleanupTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/UpdateCleanupTests.cs @@ -51,14 +51,24 @@ public void Run_via_service_provider_resolves_installer_and_console() Assert.Contains("Cleaning up previous update", console.Output, StringComparison.Ordinal); } + // Run is overloaded on IServiceProvider and IUpdateInstaller. A typed + // local pins which overload each test targets without an upcast + // expression at the call site. [Fact] - public void Run_with_null_services_throws() => Assert.Throws(() => UpdateCleanup.Run((IServiceProvider)null!)); + public void Run_with_null_services_throws() + { + IServiceProvider services = null!; + + Assert.Throws(() => UpdateCleanup.Run(services)); + } [Fact] public void Run_with_null_installer_throws() { + IUpdateInstaller installer = null!; using var console = new TestConsole(); - Assert.Throws(() => UpdateCleanup.Run((IUpdateInstaller)null!, console)); + + Assert.Throws(() => UpdateCleanup.Run(installer, console)); } [Fact]