microsoft / microsoft/microsoft-ui-reactor

[Flaky test] Two selftest fixtures still throw on a missing precondition, truncating TAP and hiding every later check

Open
#998 3 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

### What happened?

A `throw` inside a selftest fixture aborts that fixture and **truncates the TAP stream**, so every check after it is never emitted. The run then reports a single failure with no attribution, which reads downstream as an arbitrary-victim flake rather than as a precondition that did not hold.

#971 fixed this in `TabViewFillContentAreaFixtures.cs` and measured the effect directly: with the guard reverted and the body unrealized, **only 9 of 16 checks were emitted**; with the fix in place, all 16. The failure count is not the interesting part — the *evidence destruction* is.

Two genuine instances remain after that fix. Both verified against source at `d79813be`, not inferred.

### Site 1 — `WrapElementSlotFixtures.cs:43-46`

A near-exact clone of the fixed case, and the cheapest possible fix because the `H.Check` is already there:

```csharp
var tabView = H.FindControl(_ => true);
H.Check("WrapElementSlot_TabViewMounted", tabView is not null);
if (tabView is null)
throw new InvalidOperationException("TabView control was not mounted."); // <- aborts + truncates
```

The check already reports the failure correctly. The `throw` on the next line then destroys the remaining output for no added signal. One-line change:

```csharp
if (tabView is null) return;
```

### Site 2 — `Win2DCanvasFixtures.cs:327-333` (worse, despite one caller)

```csharp
private static WeakReference CaptureControlWeakReference(Harness h)
where TControl : Microsoft.UI.Xaml.DependencyObject
{
var control = h.FindControl(_ => true);
if (control is null)
throw new InvalidOperationException($"Expected {typeof(TControl).Name} in visual tree.");
return new WeakReference(control);
}
```

Worse than site 1 for two reasons, even though it currently has exactly **one** caller (`Win2DCanvasFixtures.cs:42`, `CanvasControl`):

1. **There is no `H.Check` at all**, so the failure is not merely truncating — it is entirely unreported. Nothing in the TAP stream says a precondition failed.
2. **It is a helper**, so the throw propagates into whichever fixture called it, and the message names a **type** (`CanvasControl`) rather than the fixture. A future second caller inherits the hazard silently.

Suggested shape — make the failure reportable at the call site rather than fatal in the helper:

```csharp
private static WeakReference? TryCaptureControlWeakReference(Harness h, string checkName)
where TControl : Microsoft.UI.Xaml.DependencyObject
{
var control = h.FindControl(_ => true);
h.Check(checkName, control is not null);
return control is null ? null : new WeakReference(control);
}
```

### Important: do NOT sweep all `throw new` in this tree

An audit of `tests/Reactor.AppTests.Host/SelfTest/`:

| count | file | assessment |
|---|---|---|
| 35 | `DevtoolsFixtures.cs` | mechanical `Result(resp) ?? throw` — needs its own decision, not this issue |
| 4 | `ReconcilerBigCoverageFixtures.cs` | unaudited |
| 3 | `NativeDockingRoleAwareFixture.cs` | unaudited |
| 2 | `FlexPanelCssBehaviorFixtures.cs` | unaudited |
| 2 | `ErrorBoundaryFixtures.cs` | **almost certainly intentional — throwing is the thing under test** |
| 1 | `WrapElementSlotFixtures.cs` | **defect, site 1 above** |
| 1 | `Win2DCanvasFixtures.cs` | **defect, site 2 above** |

`ErrorBoundaryFixtures.cs` is the reason a blanket sweep would be wrong: a fixture that exercises error handling *must* throw, and converting it would delete the test. The defect is narrower than "a throw in a fixture" — it is specifically:

> **a `throw` used as a precondition guard, where the fixture could instead report `H.Check(name, false)` and `return`.**

Anything whose throw is the stimulus, or is caught by the fixture itself, is out of scope.

### Verification the fix needs

- After converting, confirm the check still **fails** when the precondition genuinely does not hold — a `return` that skips past the `H.Check` would silently drop the check entirely, which is worse than the throw it replaced.
- **Emit the check unconditionally, not only on the failure path.** #971's `TabViewFill_ExplicitMounted` is written this way deliberately: a check that appears *only* when it fails cannot be distinguished from a check that never ran, so the absence of a `not ok` line stops being evidence of success.

### Reactor version / commit

`main` as of 2026-07-31, plus #971 at `d79813be` (which fixes the `TabViewFillContentAreaFixtures.cs` instance and establishes the pattern). Neither file here is touched by #971 — filed rather than folded in, to keep that PR scoped.

### Notes

Sites identified by the PR merge coordinator session's audit; verified against source and the `ErrorBoundaryFixtures` exclusion added here. Related: #971 (fixes the first instance, with the 9-of-16 measurement), #988 (TAP truncation is one of the mechanisms that makes a failure unattributable).

Contributor guide

Open the contributing guide

Research direction

Start with WrapElementSlotFixtures.cs:43-46 and Win2DCanvasFixtures.cs:327-333, then inspect the CanvasControl caller at line 42 and the existing pattern from #971. Verify each precondition check is emitted unconditionally and that the fixture returns without truncating TAP output when the control is absent. Done means both sites report a failed check while preserving later checks, without changing intentional throws such as those in ErrorBoundaryFixtures.cs.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.