From a13741d7a721e66c6ebb5ebc42698905d2a6ed18 Mon Sep 17 00:00:00 2001 From: laurentiu021 Date: Fri, 4 Sep 2026 17:50:10 +0300 Subject: [PATCH] test: re-derive the numbers and reasons four guards document None of these was failing. That is the problem: a guard whose prose is wrong still passes, and the next reader takes its derivation as fact. EveryThemeEntryPoint bounded each method by "the next member declaration", looking for the next public and only falling back to private when there was none -- so a method followed by private members had its slice run past them. SetCustom measured 6727 characters against a real body of 675, swallowing four members. Nothing there calls ApplyShade() today, so the assertion was still honest, but one added call and it could no longer tell. Now brace-matched. Its justification also said SetCustom cannot be called because Save() writes the user's real theme file. True before #1741 made SettingsPath redirectable, false since -- ThemeServiceTests calls it against a temp directory. The reason is coverage of an untested entry point, and now says that. The admin-banner guard quoted 27 views / 27 of 27 / 29 of 29, three numbers that disagreed with each other and the tree. Re-derived: 62 banners across 31 views. The failure message now computes the count instead of quoting one. The label budget subtracted 23px for a glyph column leaf rows no longer have. Real budget 178px, about 27 characters. The asserted values come from the longest shipping labels, so they are unchanged. The PR checklist omitted auto-release's today-in-UTC date rule -- the only gate that runs after the squash merge, which has published yesterday's date twice. The guard could not see the omission because it reads ci.yml and that gate is elsewhere; it now reads auto-release.yml too. Also removes wording describing a particular two-machine development setup from four tracked files. Closes #2110 --- .github/PULL_REQUEST_TEMPLATE.md | 4 ++ ARCHITECTURE.md | 3 +- .../SysManager.Tests/ArchitectureTests.cs | 66 +++++++++++++------ .../AudioPolicyConfigTests.cs | 2 +- .../SidebarSelectionContractTests.cs | 4 +- 5 files changed, 54 insertions(+), 25 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index e91048da..87701f46 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -44,6 +44,10 @@ non-releasing PR leaves that version alone. - [ ] CHANGELOG entry added, opening with a one-line plain-English lead under the version heading before the first `###` category (CI checks the lead separately, because the release notes are copied from it verbatim) +- [ ] CHANGELOG heading dated **today in UTC** — and re-dated if the merge slips to another UTC day. + This is the only gate that runs *after* the squash merge, when the branch is already gone: the + tag push that triggers the release requires the date to be today, so a stale date fails the + release rather than the PR. It has published yesterday's date twice. - [ ] `Version` / `FileVersion` / `AssemblyVersion` in `SysManager/SysManager/SysManager.csproj` bumped one step from the newest release tag and equal to the new CHANGELOG heading (`fix:` = patch, `feat:` = minor) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index a565e8bd..4c33bf9d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -396,8 +396,7 @@ Key services: output device). Feature-detected (`TryCreate` returns null when it can't bind), guarded (the SET call is invoked only after a successful `QueryInterface` for the exact IID and any failure returns false), and its endpoint-id/process-token string helpers are pure + unit-tested. The - routing SET path can only be runtime-verified on a real desktop (laptop workstation), not the - build box. The READ path (`GetPersistedDefaultEndpoint`) is deliberately unimplemented and returns + routing SET path can only be runtime-verified by running the app on real audio hardware. The READ path (`GetPersistedDefaultEndpoint`) is deliberately unimplemented and returns null, so `IAudioMixerService.GetSessionOutputDevice` has a three-state contract — an endpoint id, `string.Empty` for "read succeeded, no override", or null for "could not read" — and the row VM renders null as unknown rather than as the default device. diff --git a/SysManager/SysManager.Tests/ArchitectureTests.cs b/SysManager/SysManager.Tests/ArchitectureTests.cs index 53141c0a..2e69b976 100644 --- a/SysManager/SysManager.Tests/ArchitectureTests.cs +++ b/SysManager/SysManager.Tests/ArchitectureTests.cs @@ -411,8 +411,12 @@ public void EveryViewModelCommand_IsReachableFromTheUi() /// never went through the correction at all — four typed hex values could produce white on white while /// every shipped preset was held to a contrast floor. It also silently discarded the shade slider's /// position. - /// Asserted on the source rather than by calling the method, because SetCustom ends in - /// Save(), which writes the user's real theme file. + /// Asserted on the source, and the reason is coverage rather than safety. It used to say that + /// SetCustom could not be called because it ends in Save() and would write the user's real + /// theme file — true before #1741 made SettingsPath redirectable, and false since: + /// ThemeServiceTests now calls SetCustom for real against a temp directory. What a source + /// check still buys is the entry point nobody wrote a test for. A behaviour test covers the three methods + /// named below; this covers a fourth added later, before anyone thinks to test it. /// [Fact] public void EveryThemeEntryPoint_GoesThroughTheLegibilityCorrection() @@ -425,11 +429,15 @@ public void EveryThemeEntryPoint_GoesThroughTheLegibilityCorrection() var at = service.IndexOf($"public void {entry}(", StringComparison.Ordinal); Assert.True(at > 0, $"ThemeService.{entry} was renamed — update this guard, don't drop it."); - // To the next method declaration, so the slice is this method and nothing after it. - var end = service.IndexOf("\n public ", at + 1, StringComparison.Ordinal); - if (end < 0) end = service.IndexOf("\n private ", at + 1, StringComparison.Ordinal); - Assert.True(end > at, $"could not bound {entry}'s body."); - var body = service[at..end]; + // Brace-matched, not "up to the next member declaration". That heuristic looked for the next + // `public` and only fell back to `private` when there was none — so for a method followed by + // private members it skipped past them to a later public one. SetCustom's slice measured 6727 + // characters against a real body of 675, swallowing IsDarkBackground, ApplyShade, Shade and + // Legible. Nothing in that region calls ApplyShade() today, so the assertion below was still + // being satisfied by SetCustom's own call — but one added call anywhere in those six kilobytes + // and this guard could no longer tell whether SetCustom still does it. + var body = BalancedBlock(service, $"public void {entry}("); + Assert.True(body.Length > 0, $"could not bound {entry}'s body."); Assert.Contains("ApplyShade();", body, StringComparison.Ordinal); Assert.DoesNotContain("CurrentTheme = new ThemePreset", body, StringComparison.Ordinal); @@ -610,11 +618,15 @@ public void EveryGoldElevationBanner_PromisesMoreAccess() /// The two elevation banners that share a slot must share one geometry. /// /// - /// 27 views render both banners in the same Grid.Row, swapped on IsElevated, and the two - /// were hand-written with different geometry: CornerRadius="8" Padding="14,12" when not elevated - /// in 27 of 27 views, CornerRadius="12" Padding="12,8" when elevated in 29 of 29. Measured, the - /// banner in that fixed slot was 61.29px in one state and 35.29px in the other, so everything below it - /// jumped 26px at the moment the user granted elevation — and the corners visibly changed shape with it. + /// 31 views render both banners in the same Grid.Row, swapped on IsElevated — 62 banners, + /// one pair per view — and the two states were hand-written with different geometry: + /// CornerRadius="8" Padding="14,12" when not elevated against CornerRadius="12" Padding="12,8" + /// when elevated. Measured, the banner in that fixed slot was 61.29px in one state and 35.29px in the + /// other, so everything below it jumped 26px at the moment the user granted elevation — and the corners + /// visibly changed shape with it. Both states are on CornerRadius="12" Padding="12,8" now. + /// The counts above are what this guard parses today, re-derived. It previously said "27 of 27" and + /// "29 of 29", which were the totals at the time it was written and disagreed with each other and with + /// the tree; a reader could not reconcile them against a failure message quoting the same numbers. /// 18px of that jump is the "Run as administrator" button, which the elevated state has nothing to /// replace with, and no geometry removes it. This asserts the part that was an accident: one radius, one /// padding, both states. @@ -664,9 +676,10 @@ public void BothAdminBanners_ShareOneGeometry() .ToArray(); Assert.True(geometries.Length == 1, - "the elevation banners do not agree on their geometry. Both states occupy the SAME slot in 27 " - + "views, so a difference here is the layout below them jumping the moment the user elevates — " - + "which is the one moment the app should look steady. Pick one radius and one padding:\n " + $"the elevation banners do not agree on their geometry. Both states occupy the SAME slot in the " + + $"{seen.Select(b => b.View).Distinct(StringComparer.Ordinal).Count()} views parsed here, so a " + + "difference is the layout below them jumping the moment the user elevates — which is the one " + + "moment the app should look steady. Pick one radius and one padding:\n " + string.Join("\n ", geometries)); } @@ -1595,6 +1608,16 @@ public void ThePullRequestChecklist_AsksForWhatCiEnforces() "dotnet format", File.ReadAllText(Path.Combine(root, "CONTRIBUTING.md")), StringComparison.Ordinal); + + // The one gate that is NOT in ci.yml, which is why the checklist omitted it and why this guard could + // not see the omission: auto-release requires the newest CHANGELOG heading to be dated today in UTC, + // and it runs AFTER the squash merge, when the branch is gone. A stale date fails the release rather + // than the pull request, and it has published yesterday's date twice. + var autoRelease = File.ReadAllText( + Path.Combine(root, ".github", "workflows", "auto-release.yml")); + Assert.Contains("TODAY=$(date -u +%Y-%m-%d)", autoRelease, StringComparison.Ordinal); + Assert.Contains(items, item => item.Line > releaseSection + && item.Text.Contains("UTC", StringComparison.Ordinal)); } /// @@ -4132,11 +4155,14 @@ public void EveryTabsSidebarLabel_MatchesItsPageHeader() /// entries that render a PREVIEW pill beside the text. /// /// - /// The sidebar is a fixed 220px column (MainWindow.xaml). After the row's 28px left padding, 14px - /// right padding, the 13px glyph and its 10px margin, the label has roughly 155px — about 24 - /// characters at FontSize 13. The longest label that ships, "Profile Export / Import", is 23. So this - /// budget says "no longer than what is already there", NOT "proven to fit": whether that one already - /// ellipsizes cannot be settled without running the app, which happens on the other workstation. + /// The sidebar is a fixed 220px column (MainWindow.xaml). A leaf row's Padding="28,9,14,9" leaves + /// the label 178px — roughly 27 characters at FontSize 13. The longest label that ships, "Profile Export / + /// Import", is 23. So this budget says "no longer than what is already there", NOT "proven to fit": + /// whether that one already ellipsizes cannot be settled without running the app. + /// The derivation used to subtract "the 13px glyph and its 10px margin" as well, leaving 155px. + /// Leaf rows carry no glyph — NavItem has no such member, and the only two glyph bindings left in + /// MainWindow.xaml are the group header's own and the single-item row reading its group's — so the budget + /// was charging every leaf 23px it does not spend. /// Two budgets, because one number cannot express the constraint. A PREVIEW pill takes fixed /// width out of the same column, so a pilled row has less room for text — and the pill is exactly /// where this went wrong before: a horizontal StackPanel measured with infinite width pushed it past diff --git a/SysManager/SysManager.Tests/AudioPolicyConfigTests.cs b/SysManager/SysManager.Tests/AudioPolicyConfigTests.cs index 466adb2c..c2ad5b79 100644 --- a/SysManager/SysManager.Tests/AudioPolicyConfigTests.cs +++ b/SysManager/SysManager.Tests/AudioPolicyConfigTests.cs @@ -9,7 +9,7 @@ namespace SysManager.Tests; /// /// Tests for the pure, easy-to-get-wrong string helpers in — /// the endpoint-id wrapping and the process token. The COM activation path itself is undocumented -/// and can only be exercised on a real Windows desktop (verified on the laptop workstation), so it +/// and can only be exercised by running the app on a real Windows desktop, so it /// is intentionally not unit-tested here; these pin the formatting the routing SET call depends on. /// public class AudioPolicyConfigTests diff --git a/SysManager/SysManager.Tests/SidebarSelectionContractTests.cs b/SysManager/SysManager.Tests/SidebarSelectionContractTests.cs index ee4a5828..909e3417 100644 --- a/SysManager/SysManager.Tests/SidebarSelectionContractTests.cs +++ b/SysManager/SysManager.Tests/SidebarSelectionContractTests.cs @@ -289,8 +289,8 @@ private static void AssertSetter(XElement owner, string property, string value) /// clearly built for the keyboard — but opening the panel left focus behind on the chip, so none of /// it could be reached and there was no way out but the mouse. Key.Escape appeared nowhere in /// the app. - /// Asserted rather than demonstrated: the behaviour needs the app running, which happens on the - /// other workstation. What is pinned here is the wiring that makes it possible, on both sides — the + /// Asserted rather than demonstrated: the behaviour needs the app running, which a source-level + /// test cannot do. What is pinned here is the wiring that makes it possible, on both sides — the /// XAML hooks and the handlers they name. Half of it is useless alone: an Opened attribute /// with a handler that does not move focus reads as fixed and is not. ///