From 967df83925f1aef57138f4134e59dcc457eb45c1 Mon Sep 17 00:00:00 2001 From: gramps Date: Mon, 28 Sep 2026 11:53:52 -0700 Subject: [PATCH] tests: one source-assertion helper, so the prose trap can't recur MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trap I hit twice today — a file-wide DoesNotContain matching the doc comment that explains the ban — was possible because three tests each hand-rolled their own File.ReadAllText + path-walker, two of them mine. Fixed at the root rather than per-test: SourceAssert now owns path walking and literal extraction, with Literal(memberName, params parts) and Scopes() that assert on the parsed VALUE. Tests that genuinely assert structure (the #if DEBUG arm in InstanceIsolationTests, the absence of a whole API in the locator) still read the file themselves, because those claims cannot be reduced to a value — the helper is for literal facts only. Shipping the fix introduced two defects, both caught by the build: a method named Path shadowed System.IO.Path and broke the error message that names the missing file, and Literal(fileName, memberName) invited a call that silently resolved to a nonexistent path. Argument order is now required-context-first so a wrong path is visible at the call site. Suite 380/380, 0 warnings. LayerReorderPersistenceTests.RealMouseDrag flaked once mid-session with no ytLive instance running and passed in isolation and on re-run — the environment flake already recorded in HANDOFF.md, not a regression from this change. --- MyMistakes.md | 22 ++++++++ ytLive.Tests/FfmpegLocatorTests.cs | 24 ++------- ytLive.Tests/InstanceIsolationTests.cs | 14 +---- ytLive.Tests/SourceAssert.cs | 72 +++++++++++++++++++++++++ ytLive.Tests/YouTubeAuthServiceTests.cs | 22 ++------ 5 files changed, 101 insertions(+), 53 deletions(-) create mode 100644 ytLive.Tests/SourceAssert.cs diff --git a/MyMistakes.md b/MyMistakes.md index d5fe3e6..c0f7bd4 100644 --- a/MyMistakes.md +++ b/MyMistakes.md @@ -1340,3 +1340,25 @@ while actively documenting it. Repeating a documented mistake is not a knowledge knowledge was fine — it is a *check* failure. The check that would have caught it: when a `DoesNotContain` is aimed at a file, ask "does this file's own documentation have a legitimate reason to contain the string?" If yes, the assertion is aimed at the wrong target. + +### Extracting the duplicated helper created two fresh bugs in the same commit (2026-09-28) + +Pulling the three hand-rolled `RepoFile` path-walkers into one `SourceAssert` helper — the fix for +the repeated prose-matching trap — shipped with two defects of its own, both caught by the build: + +- **A method named `Path` shadowed `System.IO.Path` inside the class**, so the failure message that + wanted to *name the missing file* was itself a compile error (`CS0119`). Renamed to `RepoPath`. + A helper whose name collides with a BCL type is a trap for the next reader, not just for me. +- **The signature invited the bug it was meant to prevent.** `Literal(string fileName, string + memberName)` looked like it took a file name, but the body resolved the path from the repo root, + so the only legal call was a full directory path — and the call site passed + `"YouTubeAuthService.cs"`, which resolves to a path that does not exist. The failure was a + bare `FileNotFoundException` with no hint. Flipping the argument order to + `Literal(memberName, params string[] parts)` makes a wrong path *visible at the call site* + instead of buried in a resolved path. + +**The generalisation:** a parameter shape that cannot express the common legal call is a defect +even when the compiler accepts it. `fileName` + `memberName` looks fine and is wrong; the honest +shape puts the variable part last and the required context first. This is the same family as the +prose-matching lesson — **the aim is to make the wrong thing hard to write, not to detect it +afterward** — and I got it wrong in the very commit written to apply that idea. diff --git a/ytLive.Tests/FfmpegLocatorTests.cs b/ytLive.Tests/FfmpegLocatorTests.cs index 59df7dc..5e70cbd 100644 --- a/ytLive.Tests/FfmpegLocatorTests.cs +++ b/ytLive.Tests/FfmpegLocatorTests.cs @@ -32,24 +32,7 @@ public class FfmpegLocatorTests return dir; } - /// Repo root, found by walking up to a file only the repo has. - private static string RepoRoot() - { - var dir = new DirectoryInfo(AppContext.BaseDirectory); - while (dir != null) - { - if (File.Exists(Path.Combine(dir.FullName, "ytLive.csproj"))) return dir.FullName; - dir = dir.Parent; - } - throw new FileNotFoundException($"could not locate ytLive.csproj above {AppContext.BaseDirectory}"); - } - private static string RepoFile(params string[] parts) - { - var candidate = Path.Combine(new[] { RepoRoot() }.Concat(parts).ToArray()); - if (File.Exists(candidate)) return candidate; - throw new FileNotFoundException($"could not locate {Path.Combine(parts)} under {RepoRoot()}"); - } /// /// The Good Dog integration test for this unit: prove the whole chain end to end — @@ -66,7 +49,7 @@ public class FfmpegLocatorTests // repository folder name. The repo directory happens to also be called "ytLive", // so naively appending "ytLive" would resolve to a path that only works on this // machine's checkout. - var projectDir = Path.GetDirectoryName(RepoFile("ytLive.csproj"))!; + var projectDir = Path.GetDirectoryName(SourceAssert.RepoPath("ytLive.csproj"))!; var binRoot = Path.Combine(projectDir, "bin"); Assert.True(Directory.Exists(binRoot), $"no build output at {binRoot} — build ytLive.csproj before running this test"); @@ -224,8 +207,7 @@ public class FfmpegLocatorTests [InlineData("ExtractBinaries(")] public void TheLocatorCarriesNoNetworkCapability(string forbidden) { - var source = File.ReadAllText( - RepoFile("Services", "Encoder", "FfmpegLocator.cs")); + var source = SourceAssert.Text("Services", "Encoder", "FfmpegLocator.cs"); Assert.DoesNotContain(forbidden, source, StringComparison.OrdinalIgnoreCase); } @@ -238,7 +220,7 @@ public class FfmpegLocatorTests [Fact] public void TheBuildRefusesToCompileWithoutTheBundle() { - var csproj = File.ReadAllText(RepoFile("ytLive.csproj")); + var csproj = SourceAssert.Text("ytLive.csproj"); Assert.Contains("GuardBundledFfmpeg", csproj, StringComparison.Ordinal); Assert.Contains("ffmpeg\\ffmpeg.exe", csproj, StringComparison.Ordinal); Assert.Contains("CopyToPublishDirectory=\"Always\"", csproj, StringComparison.Ordinal); diff --git a/ytLive.Tests/InstanceIsolationTests.cs b/ytLive.Tests/InstanceIsolationTests.cs index ad4cd54..21c3bf6 100644 --- a/ytLive.Tests/InstanceIsolationTests.cs +++ b/ytLive.Tests/InstanceIsolationTests.cs @@ -202,7 +202,7 @@ public sealed class InstanceIsolationTests // DataRoot / WebViewDataFolder are present in both configurations. Note a // const like YTLIVE_INSTANCE is INLINED at compile time and therefore never // appears in either binary — grepping for the variable name proves nothing. - var source = File.ReadAllText(RepoFile("Helpers", "InstanceProfile.cs")); + var source = SourceAssert.Text("Helpers", "InstanceProfile.cs"); // The whole promise is that a Release build compiles to the #else arm, so assert // that arm IS production: the one real user profile, no alternate folder, and no @@ -225,16 +225,4 @@ public sealed class InstanceIsolationTests Assert.Contains("instances", debugArm); } - private static string RepoFile(params string[] parts) - { - var dir = new DirectoryInfo(AppContext.BaseDirectory); - while (dir != null) - { - var candidate = Path.Combine(new[] { dir.FullName }.Concat(parts).ToArray()); - if (File.Exists(candidate)) return candidate; - dir = dir.Parent; - } - throw new FileNotFoundException( - $"could not locate {Path.Combine(parts)} above {AppContext.BaseDirectory}"); - } } diff --git a/ytLive.Tests/SourceAssert.cs b/ytLive.Tests/SourceAssert.cs new file mode 100644 index 0000000..2d11fa8 --- /dev/null +++ b/ytLive.Tests/SourceAssert.cs @@ -0,0 +1,72 @@ +using System; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; +namespace ytLive.Tests; + +/// +/// Shared helpers for the source-level tests — the ones that assert on the text of a +/// source file because the behaviour they guard is structural (a field confined to +/// #if DEBUG, a type carrying no network capability, a build refusing to compile +/// without an asset). Those are legitimate assertions: InstanceIsolationTests has +/// carried one since 2026-08 and it caught a real hoisting bug. +/// +/// Use , not a file-wide string match. Three tests each grew +/// their own File.ReadAllText + RepoFile pair, and two of them then failed on the +/// file's own doc comment — the locator's DoesNotContain("Download") matched the sentence +/// saying "nothing is downloaded", and the auth test's DoesNotContain("force-ssl") matched +/// the comment explaining why force-ssl was removed. Both were fixed by parsing the literal out +/// of the source and asserting on the value. That failure is not a one-off: a file is +/// mostly prose, and the documentation is supposed to name the thing being banned. A +/// grep-based ban asserts "this string never appears", which is a much stronger and much less +/// useful claim than "this value is correct". +/// +/// Only the path-walking and the literal extraction live here. Tests that genuinely need to +/// assert on structure — the #if DEBUG arm, the absence of a whole API — should still read +/// the file themselves, because those claims are about code and cannot be reduced to a value. +/// +internal static class SourceAssert +{ + /// Walks up from the test output directory to the repo root and returns the + /// absolute path of the given repo-relative file. + public static string RepoPath(params string[] parts) + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null) + { + var candidate = System.IO.Path.Combine(new[] { dir.FullName }.Concat(parts).ToArray()); + if (File.Exists(candidate)) return candidate; + dir = dir.Parent; + } + throw new FileNotFoundException( + $"could not locate {System.IO.Path.Combine(parts)} above {AppContext.BaseDirectory}"); + } + + public static string Text(params string[] parts) => File.ReadAllText(RepoPath(parts)); + + /// Extracts the value of a C# string constant or const field declared in the + /// given source file, e.g. Literal("Scope", "Services", "YouTubeAuthService.cs") for + /// private const string Scope = "…";. Whitespace and the trailing semicolon are not + /// part of the value. The path parts are repo-relative and may span directories — the + /// member name comes first so a wrong path can never be mistaken for a member name. + /// + public static string Literal(string memberName, params string[] parts) + { + var source = Text(parts); + var match = Regex.Match( + source, + $@"(?:const|static readonly|readonly)\s+string\s+{Regex.Escape(memberName)}\s*=\s*""([^""]*)""\s*;"); + Assert.True(match.Success, + $"could not find a string constant named '{memberName}' in {System.IO.Path.Combine(parts)} — " + + "if it was renamed or its declaration reshaped, update this test rather than " + + "loosening the assertion."); + return match.Groups[1].Value; + } + + /// The scopes requested in a space-separated OAuth scope const, split into + /// individual scope URIs so a test can assert on the SET rather than on one string. + public static string[] Scopes(string memberName, params string[] parts) => + Literal(memberName, parts) + .Split(' ', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); +} diff --git a/ytLive.Tests/YouTubeAuthServiceTests.cs b/ytLive.Tests/YouTubeAuthServiceTests.cs index 899e72f..955954f 100644 --- a/ytLive.Tests/YouTubeAuthServiceTests.cs +++ b/ytLive.Tests/YouTubeAuthServiceTests.cs @@ -140,25 +140,9 @@ public class YouTubeAuthServiceTests [Fact] public void ExactlyOneScopeIsRequested_AndItIsTheBroadOne() { - var source = File.ReadAllText(RepoFile("Services", "YouTubeAuthService.cs")); - - var match = Regex.Match(source, @"private const string Scope = ""([^""]*)"";"); - Assert.True(match.Success, "could not find the Scope const in YouTubeAuthService.cs"); - - var scopes = match.Groups[1].Value.Split(' ', StringSplitOptions.RemoveEmptyEntries); - Assert.Equal(new[] { "https://www.googleapis.com/auth/youtube" }, scopes); + Assert.Equal( + new[] { "https://www.googleapis.com/auth/youtube" }, + SourceAssert.Scopes("Scope", "Services", "YouTubeAuthService.cs")); } - private static string RepoFile(params string[] parts) - { - var dir = new DirectoryInfo(AppContext.BaseDirectory); - while (dir != null) - { - var candidate = Path.Combine(new[] { dir.FullName }.Concat(parts).ToArray()); - if (File.Exists(candidate)) return candidate; - dir = dir.Parent; - } - throw new FileNotFoundException( - $"could not locate {Path.Combine(parts)} above {AppContext.BaseDirectory}"); - } }