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

Open
#1,033 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug tests
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.