microsoft / microsoft/microsoft-ui-reactor
Selftest: `Issue717_PinReleasedAfterRecovery` cannot detect "the pin never engaged", and `Issue717_Collapse_Pinned*` go vacuously green when their control fails
Nobody has claimed this yet.
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
Found by running the sweep proposed on the #990 discussion: *in a harness where checks record-and-continue, every precondition-shaped check is advisory unless explicitly followed by a gate.*
`H.Check` is non-halting (`Harness.cs:180-189`) — it prints `not ok`, increments `_failures`, and **returns**. So a failed control check does not stop a downstream assertion that is only meaningful when the control held.
## How the candidates were found (reproducible)
The corpus is too large to read (**5757 `H.Check` calls across 187 fixture files**), so I scanned for the specific vacuity shape rather than eyeballing: a tolerance comparison between a **live read** and a **baseline captured earlier from the same source** — `Math.Abs( - ) <= tol`. If the action under test no-ops, the difference is 0 and the check passes regardless of the product.
That narrowed 5757 → **6 candidates**: 2 already known in `Issue487ScrollAnchorFixtures.cs` (`:237`, `:368`), and 4 new, all in `Issue717ExtentPinFixtures.cs`. Reading them re-ranked them — the shape is a *filter*, not a verdict, because "X was restored" is vacuous on a no-op while "X was left alone" is *supposed* to pass.
## Finding 1 (most severe) — `Issue717_ExtentPinReleasesAfterContentRecovers` reports 2/2 green when the feature is entirely broken
`Issue717ExtentPinFixtures.cs:243-254`
```csharp
double originalMinHeight = rtb.MinHeight; // author default (0)
H.ClickButton("MutatePin717");
await host.WaitForIdleAsync();
await Harness.WaitFor(
() => Math.Abs(rtb.MinHeight - originalMinHeight) <= 0.5,
maxPasses: 90, perPassMs: 12);
H.Check("Issue717_PinReleasedAfterRecovery",
Math.Abs(rtb.MinHeight - originalMinHeight) <= 0.5);
```
If the product **stops engaging the pin at all**, `MinHeight` never leaves its original `0`:
1. the `WaitFor` predicate is **true at t=0**, so it returns immediately without waiting;
2. the assertion evaluates the *same* predicate → `ok`;
3. the fixture's only other check is `Issue717_Release_RtbMounted`, which also passes.
**Two green checks, nothing exercised, no red anywhere in the fixture.** This is the `WaitFor` survival-vs-eventual trap documented in `AGENTS.md`: *"if the predicate is true the instant `WaitFor` is called, does the next assertion still mean what I think it means?"* Here it does not — `WaitFor` and the assertion are the same predicate, so the wait cannot add information the assertion doesn't already have.
**The fix is already in this file.** Three fixtures click `MutatePin717`; the other two sample the product's engagement counter first, one of them for exactly this reason:
```csharp
// :286-291
int baseline = Reconciler.InlineUiPinEngagementCount;
H.ClickButton("MutatePin717");
await host.WaitForIdleAsync();
// Prove the pin actually engaged and raised the floor, so the no-clobber
// assertion below cannot pass vacuously (e.g. if the pin never ran).
```
| `MutatePin717` clicker | engagement control |
|---|---|
| `:122` | ✅ has it |
| **`:245` (release fixture)** | ❌ **missing** |
| `:287` | ✅ has it |
So this is not a missing convention or an unavailable instrument — the author identified the hazard, named it in a comment, and built the counter; the release fixture one screen away just doesn't use it.
**Suggested fix** — sample `Reconciler.InlineUiPinEngagementCount` before the click, assert it advanced, and **gate** on it using the `if (…) return;` idiom already used in this same file at `:155` and `:241`:
```csharp
int baseline = Reconciler.InlineUiPinEngagementCount;
H.ClickButton("MutatePin717");
await host.WaitForIdleAsync();
bool engaged = Reconciler.InlineUiPinEngagementCount > baseline;
H.Check("Issue717_Release_PinEngaged", engaged);
if (!engaged) return; // gate: the release assertion is meaningless if nothing engaged
```
## Finding 2 — `Issue717_Collapse_PinnedExtentHeld` / `PinnedOffsetPreserved` are green in the world where their control is red
`Issue717ExtentPinFixtures.cs:183-210`. `Issue717_Collapse_UnpinnedClamps` (`:185`) is the control — it proves the harness can actually reproduce the #717 clamp. In a degenerate environment (nothing scrollable) it fails, and then:
```
PinnedExtentHeld : sv.ScrollableHeight >= prePinScrollable - 1.0 → 0 >= -1.0 → ok
PinnedOffsetPreserved: |sv.VerticalOffset - prePinOffset| <= 2.0 → |0 - 0| → ok
```
Structurally identical to `Issue487_SV2ClampObserved` → `Issue487_SV2OffsetRestoredAfterMutation`. The run *does* go red (the control and two others fail), so CI is not blind — but **the wrong checks report `ok`**, which misattributes the failure and means removing or relaxing the control silently converts both "core assertions" into tautologies.
**Suggested fix** — `if (!clampWithoutPin) return;` between `:185` and `:207`, mirroring the `if (!clamp) return;` fix proposed for #487.
## Not a finding — `Issue717_Collapse_ParkedAtBottom` is the correct shape
`:167-168` matched the scan but is sound:
```csharp
Math.Abs(preOffset - preScrollable) <= 2.0 && preOffset > InlineHeight
```
The first term alone would pass at `0 == 0`; the `&& preOffset > InlineHeight` conjunct is exactly what defeats the degenerate case. Worth recording as the in-file exemplar — same role as `Drift_FinalOffsetAtBottom` in the #487 file, where the non-vacuous formulation sat between the two vacuous ones.
## Secondary — `H.ClickButton` is fail-open, and 685 call sites inherit it
`Harness.cs:365-375`:
```csharp
public void ClickButton(string label)
{
var btn = FindButton(label);
if (btn is not null && btn.IsEnabled)
{
…Invoke();
}
}
```
No `else`, no throw, no return value — a **missing, renamed, or disabled** button is a no-op that returns normally, so "clicked it" and "silently did nothing" are indistinguishable to every caller. There are **685 `H.ClickButton` call sites** in the fixture corpus; any whose assertions are satisfied by the pre-click state passes without the click ever happening. Finding 1 is one concrete instance.
Same fail-open family as `WinAppUi.SendKeys`'s `if (r.ExitCode != 0) throw` (which cannot separate "typed the keys" from "typed an unmapped glyph and did nothing") and `# Total failures: 0` from a stale binary. Filing here as context rather than proposing a signature change, since returning `bool` or throwing would touch all 685 sites and deserves its own decision.
## Scope note
The scan only covers **one** vacuity shape. It would not catch a check whose baseline is a literal, a differential that collapses because two arms converge, or a precondition/result pair with no arithmetic in it. It narrows where to look; it does not certify the rest of the corpus.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in Issue717ExtentPinFixtures.cs at the release fixture around lines 243-254 and the collapse checks around lines 183-210. Read the engagement-counter examples around lines 122 and 286-291, plus the existing gates near lines 155 and 241. Done means the release check proves engagement before recovery and the pinned checks are gated on the unpinned clamp; run the Issue 717 selftests to verify the controls and dependent assertions report meaningful results.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 70/100