test: re-derive the numbers and reasons four guards document - #2116
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2110.
Four guards documented numbers or reasons that no longer matched the code they protect. None was failing, which is the point: a guard whose prose is wrong still passes, and the next reader takes its derivation as current fact.
1.
SetCustom's slice ran ten times past its own bodyEveryThemeEntryPoint_GoesThroughTheLegibilityCorrectionbounded each method with "up to the next member declaration", implemented as find the nextpublic, and only fall back toprivateif there is none. For a method followed by private members that skips over them to a later public one.Measured:
SetCustom's slice was 6727 characters against a real body of 675, swallowingIsDarkBackground,ApplyShade,ShadeandLegible. Nothing in that region callsApplyShade()today, soAssert.Contains("ApplyShade();", body)was still being satisfied bySetCustom's own call — but one added call anywhere in those six kilobytes and the guard could no longer tell whetherSetCustomstill does it.Now brace-matched, via the
BalancedBlockhelper added in #2112.2. Its justification was three fixes out of date
It said the assertion is on source rather than by calling the method "because
SetCustomends inSave(), which writes the user's real theme file". True before #1741 madeSettingsPathredirectable — and false since:ThemeServiceTestscallsSetCustomfor real against a temp directory. The guard is still worth having, for a different reason (an entry point nobody wrote a test for), and now says so.3. The admin-banner counts
Remarks and failure message both quoted "27 views", "27 of 27" and "29 of 29" — three numbers that disagreed with each other and with the tree. Re-derived: 62 banners across 31 views, 31 pairs, all on
CornerRadius="12" Padding="12,8". The failure message now computes the count from what it parsed instead of quoting a remembered one.4. The sidebar-label budget charged leaf rows for a deleted glyph column
The derivation subtracted "the 13px glyph and its 10px margin", leaving 155px. Leaf rows carry no glyph —
NavItemhas no such member, and the only two glyph bindings left inMainWindow.xamlare the group header's own and the single-item row reading its group's. A leaf label's real budget is220 - 28 - 14 = 178px, about 27 characters. The asserted budgets (21 and 23) come from the longest shipping labels, not the pixel math, so they are unchanged.5. The PR checklist omitted the one gate that fails after the merge
auto-release.ymlrequires 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 — the workflow's own comment says it has published yesterday's date twice. The checklist said nothing about it.ThePullRequestChecklist_AsksForWhatCiEnforcescould not see the omission either: it anchors onci.yml's step names, and this gate is not inci.yml. It now readsauto-release.ymltoo and requires the release-only section to mention UTC.Also: private workflow wording out of tracked files
Four tracked files described a particular two-machine development setup — "which happens on the other workstation", "verified on the laptop workstation", "not the build box". Rewritten to say what they mean ("needs the app actually running") in
ArchitectureTests.cs,SidebarSelectionContractTests.cs,AudioPolicyConfigTests.csandARCHITECTURE.md. Zero occurrences remain in the tree; the same pass was already applied to 23 issue and PR bodies.Verification
Mutation proof, both files restored byte-for-byte:
ThePullRequestChecklist_AsksForWhatCiEnforces, and only that testSetCustomdropsApplyShade()and a member in the previously-swallowed region gains oneEveryThemeEntryPoint_GoesThroughTheLegibilityCorrection, and only that testThe second mutation is deliberately the case the old slice would have accepted. Baseline and post-restore 2 green, 0 red.
107 cases green across
ArchitectureTests,SidebarSelectionContractTests,AudioPolicyConfigTestsandThemeServiceTests. Builds 0 errors / 0 warnings,dotnet format --verify-no-changesclean on both projects.test:— no version bump, no CHANGELOG entry, no release.