Two alarm-ack tests assert an absence with AwaitAssert, so they pass instantly and prove nothing #501

New Issue

2026-07-25T18:07:33-04:00

dohertj2 commented

2026-07-25 18:07:33 -04:00

Found while root-causing #500. Not a flake - these always pass; the problem is that they pass without testing anything.

DriverHostActorNativeAlarmAckRoutingTests.cs:90 and :116:

// Give the (fire-and-forget) handler time to run; the unmapped node must produce no ack.
AwaitAssert(() => recorder.Acks.ShouldBeEmpty(), duration: TimeSpan.FromMilliseconds(800));

AwaitAssert polls and returns on the first success. recorder.Acks is empty at t=0, so the very first poll succeeds and the call returns immediately - the 800 ms is never spent. The comment says "give the handler time to run", but no time is given at all.

Consequence: if the routing regressed and an ack were produced for an unmapped node (or on a Secondary), these tests would still very likely pass, because the assertion is evaluated before the fire-and-forget handler has had any chance to run.

AwaitAssert is the right tool for a presence assertion (wait until X becomes true) and the wrong tool for an absence one (confirm X never becomes true). The absence form needs settle-then-assert:

ExpectNoMsg(TimeSpan.FromMilliseconds(800));
recorder.Acks.ShouldBeEmpty();

DriverHostActorUnreadableArtifactTests already does this correctly and documents why its settle window is calibrated against a positive control - that is the pattern to copy.

Note before fixing: changing these may turn them red. If it does, that is a genuine defect being surfaced, not a test regression, so it wants doing deliberately rather than folded into an unrelated change.

Checked and NOT affected: PeerProbeSupervisorTests's ChildCount.ShouldBe(0) waits are each preceded by a ShouldBe(1), so they assert a real transition rather than the initial state.

Found while root-causing #500. **Not a flake** - these always pass; the problem is that they pass without testing anything. `DriverHostActorNativeAlarmAckRoutingTests.cs:90` and `:116`: ```csharp // Give the (fire-and-forget) handler time to run; the unmapped node must produce no ack. AwaitAssert(() => recorder.Acks.ShouldBeEmpty(), duration: TimeSpan.FromMilliseconds(800)); ``` `AwaitAssert` polls and returns on the **first success**. `recorder.Acks` is empty at t=0, so the very first poll succeeds and the call returns immediately - the 800 ms is never spent. The comment says "give the handler time to run", but no time is given at all. Consequence: if the routing regressed and an ack *were* produced for an unmapped node (or on a Secondary), these tests would still very likely pass, because the assertion is evaluated before the fire-and-forget handler has had any chance to run. `AwaitAssert` is the right tool for a **presence** assertion (wait until X becomes true) and the wrong tool for an **absence** one (confirm X never becomes true). The absence form needs settle-then-assert: ```csharp ExpectNoMsg(TimeSpan.FromMilliseconds(800)); recorder.Acks.ShouldBeEmpty(); ``` `DriverHostActorUnreadableArtifactTests` already does this correctly and documents why its settle window is calibrated against a positive control - that is the pattern to copy. **Note before fixing:** changing these may turn them red. If it does, that is a genuine defect being surfaced, not a test regression, so it wants doing deliberately rather than folded into an unrelated change. Checked and NOT affected: `PeerProbeSupervisorTests`'s `ChildCount.ShouldBe(0)` waits are each preceded by a `ShouldBe(1)`, so they assert a real transition rather than the initial state.

dohertj2 referenced this issue

2026-07-25 18:39:57 -04:00

50 sub-second presence budgets remain in Runtime.Tests (the class behind three of #500's five defects) #502

dohertj2 referenced this issue

2026-07-25 20:20:36 -04:00

Intermittent single-test failure in Runtime.Tests under full-sweep load (unidentified) #500

dohertj2 referenced this issue

2026-07-25 20:20:37 -04:00

50 sub-second presence budgets remain in Runtime.Tests (the class behind three of #500's five defects) #502

dohertj2 referenced this issue from a commit

2026-07-25 20:24:23 -04:00

test(runtime): fix the intermittent Runtime.Tests failure — 4 ordering/logic defects + the presence-budget class (#500)

dohertj2 referenced this issue from a commit

2026-07-25 22:44:40 -04:00

merge: Sql driver follow-ups #496/#497/#498 + the Runtime.Tests flake #500 (fix/sql-driver-followups)

dohertj2 commented

2026-07-26 10:43:01 -04:00

Fixed on fix/issues-503-501-493-499-488, commit 240d7aa8.

Replaced with positive controls rather than ExpectNoMsg sleeps — ordering does the work a timer was pretending to do:

RouteNativeAlarmAck_unknown_node_is_dropped — follow the unmapped ack with a mapped one. The host forwards via Tell and DriverInstanceActor handles RouteAlarmAck with ReceiveAsync (which suspends its own mailbox), so once the control reaches the driver the unmapped one has provably been handled.
RouteNativeAlarmAck_on_non_primary_is_dropped — the role itself is the control: return the node to Primary and re-send. The comments (gated / control) are the discriminator, so a leaked ack is visible rather than merged into a count.

A fourth site, not listed in this issue. The note here says the PeerProbeSupervisorTests ChildCount.ShouldBe(0) waits are each preceded by a ShouldBe(1). That holds for three of the four — Single_node_snapshot_spawns_no_children has nothing before it and asserts the initial state, so it has the identical defect. Now followed by a two-node snapshot: the count reaching 1 proves the single-node snapshot was processed, and a local-node child would have made it 2.

On the warning that these might turn red: they didn't — the routing is correct. Since "still passes" is indistinguishable from "still proves nothing", I broke the production code deliberately instead:

Break	Result
Primary gate disabled for acks	`..._on_non_primary_is_dropped` FAILS
Unmapped condition ids routed to an arbitrary mapping	`..._unknown_node_is_dropped` FAILS

Under the old AwaitAssert form, both breaks passed. Runtime.Tests 507/507 green after restore.

Fixed on `fix/issues-503-501-493-499-488`, commit `240d7aa8`. Replaced with **positive controls** rather than `ExpectNoMsg` sleeps — ordering does the work a timer was pretending to do: - `RouteNativeAlarmAck_unknown_node_is_dropped` — follow the unmapped ack with a **mapped** one. The host forwards via `Tell` and `DriverInstanceActor` handles `RouteAlarmAck` with **`ReceiveAsync`** (which suspends its own mailbox), so once the control reaches the driver the unmapped one has provably been handled. - `RouteNativeAlarmAck_on_non_primary_is_dropped` — the **role itself** is the control: return the node to Primary and re-send. The comments (`gated` / `control`) are the discriminator, so a leaked ack is *visible* rather than merged into a count. **A fourth site, not listed in this issue.** The note here says the `PeerProbeSupervisorTests` `ChildCount.ShouldBe(0)` waits are each preceded by a `ShouldBe(1)`. That holds for **three of the four** — `Single_node_snapshot_spawns_no_children` has nothing before it and asserts the initial state, so it has the identical defect. Now followed by a two-node snapshot: the count reaching 1 proves the single-node snapshot was processed, and a local-node child would have made it 2. **On the warning that these might turn red:** they didn't — the routing is correct. Since "still passes" is indistinguishable from "still proves nothing", I broke the production code deliberately instead: | Break | Result | |---|---| | Primary gate disabled for acks | `..._on_non_primary_is_dropped` **FAILS** | | Unmapped condition ids routed to an arbitrary mapping | `..._unknown_node_is_dropped` **FAILS** | Under the old `AwaitAssert` form, **both breaks passed**. Runtime.Tests 507/507 green after restore.

dohertj2 closed this issue

2026-07-26 10:43:01 -04:00

dohertj2 referenced this issue from a commit

2026-07-26 10:53:53 -04:00

fix(test): give the three absence assertions a positive control (#501)

dohertj2 referenced this issue from a commit

2026-07-26 10:53:53 -04:00

merge: test-correctness, config-secret and AdminUI fixes (#503 #501 #493 #499 #488 #505)

Sign in to join this conversation.