From 428b8db024a7f3d1d1c23e572735ce7d036b7b42 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 16:33:24 -0400 Subject: [PATCH] test(dcl+communication+storeandforward+kpihistory+host): close register row-40-class sleeps + row-32 provider leak (arch-review 2026-08-19 P8-8 + P8-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P8-8: replace the remaining wall-clock sleep-then-assert instances outside Communication.Tests' Medium-wave sweep with the same row-40 house pattern — continuous polling (AssertHoldsFor) for ABSENCE assertions instead of a single end-of-window sample, which cannot distinguish "stayed quiet throughout" from "hadn't happened yet when the one sample landed" and cannot catch a value that flips wrong and back before that sample. - DataConnectionActorBatchTests: QualityCounters_ArePushedOnTransitionOnly_AndCoalesced (:206/:214 class) converted to AssertHoldsFor; ResolutionProbes_BackOff... (:181/:184, a genuine real-time backoff-RATE assertion, not a sync-primitive stand-in) converted to AssertRoundsWithinWindow, which enforces the ceiling continuously (fails fast on a no-backoff regression) while the floor still necessarily waits out the window. - SiteAlarmLiveCacheServiceTests (:452-454, :470-472): converted to the project's own AssertHoldsFor (mirrors SiteAlarmAggregatorActorTests', same assembly). - StoreAndForwardServiceTests (:891-892): async AssertHoldsForAsync variant (the class runs on the async test thread). - KpiHistoryRecorderActorTests (:1043-1049): two skip-path FoldCount==0 absence checks converted. Each helper is a private per-file copy, matching the Medium-wave T17 precedent (SiteAlarmAggregatorActorTests.AssertHoldsFor) rather than a shared TestSupport utility. Proof of the mechanism: a standalone (non-repo) harness run 100 trials of a real background thread flipping a boolean true then back to false inside a 300ms window — Thread.Sleep(300)+one sample missed the violation 100/100 times (false pass), continuous 20ms polling caught it 100/100 times. Most of the actual conversions here guard monotonic counters (their single-sample variant would eventually see the final value too), so each converted test carries an inline honesty note on what specifically the conversion buys for that assertion (fail-fast vs. true flip-back detection). P8-9: ScadaBridgeTelemetryTests.EmitHelpers_DoNotThrow called SetQueueDepthProvider(() => 5) against the process-global row-32 provider slot with no clear and no [Collection] — a landmine for any future same-process reader of scadabridge.store_and_forward.queue.depth. Fixed with compare-and-clear in a finally via the existing (production, untouched) ClearQueueDepthProvider. Searched this assembly and the whole test tree for a second same-process reader that asserts on the value: none exists today (MetricsEndpointTests scrapes /metrics but only asserts on the response shape, and the only value-asserting test, QueueDepthGaugeTests, lives in StoreAndForward.Tests — a separate test-host process the leak cannot reach), so no [Collection] was added; the test's doc comment records the search and flags it for re-check if that changes. Test-only change, zero production edits. Full suites green: DCL.Tests 342/342, Communication.Tests 729/729, StoreAndForward.Tests 135/135, KpiHistory.Tests 54/54, Host.Tests 522/522. Each converted test additionally looped 20x in isolation with zero flakes. --- .../SiteAlarmLiveCacheServiceTests.cs | 32 +++++++- .../Actors/DataConnectionActorBatchTests.cs | 78 +++++++++++++++++-- .../ScadaBridgeTelemetryTests.cs | 43 +++++++++- .../KpiHistoryRecorderActorTests.cs | 30 ++++++- .../StoreAndForwardServiceTests.cs | 32 +++++++- 5 files changed, 196 insertions(+), 19 deletions(-) diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs index 0aead6e8..e0f164bf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs @@ -136,6 +136,30 @@ public class SiteAlarmLiveCacheServiceTests : TestKit return service; } + /// + /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave + /// SiteAlarmAggregatorActorTests.AssertHoldsFor treatment in this same test + /// project): proves every check in keeps holding throughout + /// by polling, rather than a single blind + /// Thread.Sleep(window) followed by one end-of-window sample. A bare + /// sleep-then-assert cannot distinguish "the mechanism correctly stayed quiet" from "the + /// mechanism just hadn't run yet when the one sample was taken", and it can't catch a + /// value that flips wrong and then flips back before that single sample. This fails + /// immediately the first tick any check goes false, and only returns once the full + /// window has elapsed with every check still true. + /// + private static void AssertHoldsFor(TimeSpan window, params (Func Holds, string Because)[] checks) + { + var deadline = DateTime.UtcNow + window; + while (true) + { + foreach (var (holds, because) in checks) + Assert.True(holds(), because); + if (DateTime.UtcNow >= deadline) return; + Thread.Sleep(20); + } + } + [Fact] public void Seed_FanOut_Publishes_The_Instances_That_Failed_To_Answer() { @@ -450,8 +474,8 @@ public class SiteAlarmLiveCacheServiceTests : TestKit sub.Dispose(); AwaitAssert(() => Assert.False(service.IsLive(SiteId)), TimeSpan.FromSeconds(10)); var startsAfterStop = factory.GetOrCreateCount; - Thread.Sleep(300); - Assert.Equal(startsAfterStop, factory.GetOrCreateCount); // no self-heal restart fired + AssertHoldsFor(TimeSpan.FromMilliseconds(300), + (() => factory.GetOrCreateCount == startsAfterStop, "no self-heal restart fired")); } // ── WP2.3: IsLive tracks the STREAM, not merely "a snapshot was published once" ── @@ -468,8 +492,8 @@ public class SiteAlarmLiveCacheServiceTests : TestKit // the cache is not live — the page must keep polling. Pre-fix IsLive flipped true // here and the page grafted a never-updating snapshot over fresh poll data. AwaitCondition(() => service.GetCurrentAlarms(SiteId) is not null, TimeSpan.FromSeconds(5)); - Thread.Sleep(400); - Assert.False(service.IsLive(SiteId)); + AssertHoldsFor(TimeSpan.FromMilliseconds(400), + (() => !service.IsLive(SiteId), "must stay not-live until the site accepts the stream")); // The site accepts it → live. AwaitCondition(() => factory.Client.Connects.Count >= 1, TimeSpan.FromSeconds(5)); diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionActorBatchTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionActorBatchTests.cs index 7d309df8..30fd13ae 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionActorBatchTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Actors/DataConnectionActorBatchTests.cs @@ -177,11 +177,73 @@ public class DataConnectionActorBatchTests : TestKit ExpectMsg(u => u.Quality == QualityCode.Bad, TimeSpan.FromSeconds(5)); ExpectMsg(TimeSpan.FromSeconds(5)); + // This is a genuine real-time rate assertion (how many retry rounds a backoff + // schedule produces inside a fixed wall-clock window), not a synchronization + // primitive standing in for "wait until X happens" — proving the floor (>= 2 + // rounds) is unavoidably tied to actually waiting out the window. What the old + // blind Thread.Sleep(2000) + single end-of-window sample could NOT catch is a + // regression to full-rate (no-backoff) retrying: it would still land somewhere + // in [2, 8] by luck if the window elapsed while under load, or it would blow + // past 8 well before the 2s mark and nobody would know until the final sample. + // AssertRoundsWithinWindow polls throughout and fails immediately the first time + // the ceiling is breached, so a no-backoff regression is caught fast instead of + // being masked by (or racing) the fixed window. var initialCalls = adapter.SubscribeBatches.Count; - Thread.Sleep(2000); - var rounds = adapter.SubscribeBatches.Count - initialCalls; + AssertRoundsWithinWindow(TimeSpan.FromSeconds(2), + () => adapter.SubscribeBatches.Count - initialCalls, + minInclusive: 2, maxInclusive: 8, + context: "dead-device probe backoff (floor 150ms, ceiling 600ms)"); + } - Assert.InRange(rounds, 2, 8); + /// + /// Row-40 house pattern adapted for a RATE assertion: proves a monotonically-increasing + /// count stays within [minInclusive, maxInclusive] across the whole + /// , rather than a single blind sleep-then-sample at the end. + /// The upper bound is enforced continuously (fails fast the instant it is exceeded, + /// well before the window elapses); the lower bound can only be confirmed once the + /// full window has elapsed, since proving "at least N happened by wall-clock time T" + /// requires actually waiting until T. + /// + private static void AssertRoundsWithinWindow( + TimeSpan window, Func currentRounds, int minInclusive, int maxInclusive, string context) + { + var deadline = DateTime.UtcNow + window; + while (true) + { + var rounds = currentRounds(); + Assert.True(rounds <= maxInclusive, + $"{context}: {rounds} rounds already fired, exceeding the ceiling of {maxInclusive} " + + "before the window even elapsed — backoff is not slowing retries down."); + if (DateTime.UtcNow >= deadline) + { + Assert.True(rounds >= minInclusive, + $"{context}: only {rounds} rounds fired in the window (need >= {minInclusive})."); + return; + } + Thread.Sleep(20); + } + } + + /// + /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave + /// SiteAlarmAggregatorActorTests.AssertHoldsFor treatment): proves + /// keeps being true for the whole by + /// polling, rather than a single blind Thread.Sleep(window) followed by one + /// end-of-window sample. A bare sleep-then-assert cannot distinguish "the mechanism + /// correctly stayed quiet" from "the mechanism just hadn't run yet when the one sample + /// was taken", and it can't catch a value that flips wrong and then flips back before + /// that single sample. This fails immediately the first time + /// goes false, and only returns once the full window has elapsed with it still true. + /// + private static void AssertHoldsFor(TimeSpan window, Func holds, string because) + { + var deadline = DateTime.UtcNow + window; + while (true) + { + Assert.True(holds(), because); + if (DateTime.UtcNow >= deadline) return; + Thread.Sleep(20); + } } [Fact] @@ -203,16 +265,18 @@ public class DataConnectionActorBatchTests : TestKit for (var i = 0; i < 20; i++) adapter.ValueCallback!("tag1", new TagValue(i, QualityCode.Good, DateTimeOffset.UtcNow)); - Thread.Sleep(400); - Assert.DoesNotContain(_health.ReceivedCalls(), c => c.GetMethodInfo().Name == "UpdateTagQuality"); + AssertHoldsFor(TimeSpan.FromMilliseconds(400), + () => !_health.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "UpdateTagQuality"), + "20 unchanged-quality repeats must never push UpdateTagQuality"); // One genuine transition → exactly one coalesced push. adapter.ValueCallback!("tag1", new TagValue(1, QualityCode.Bad, DateTimeOffset.UtcNow)); AwaitCondition( () => _health.ReceivedCalls().Count(c => c.GetMethodInfo().Name == "UpdateTagQuality") == 1, TimeSpan.FromSeconds(3)); - Thread.Sleep(300); - Assert.Single(_health.ReceivedCalls(), c => c.GetMethodInfo().Name == "UpdateTagQuality"); + AssertHoldsFor(TimeSpan.FromMilliseconds(300), + () => _health.ReceivedCalls().Count(c => c.GetMethodInfo().Name == "UpdateTagQuality") == 1, + "the coalesced push must not repeat"); } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ScadaBridgeTelemetryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ScadaBridgeTelemetryTests.cs index c17b290b..a18c4875 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ScadaBridgeTelemetryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ScadaBridgeTelemetryTests.cs @@ -15,9 +15,37 @@ public class ScadaBridgeTelemetryTests Assert.Equal("ZB.MOM.WW.ScadaBridge", ScadaBridgeTelemetry.MeterName); } + /// + /// Register row 32 class: SetQueueDepthProvider writes the SAME process-global + /// slot QueueDepthGaugeCollection (StoreAndForward.Tests) exists to serialize + /// access to — this test was mutating it with no clear and no [Collection], + /// leaving a stray () => 5 provider registered in the process for the rest of + /// this assembly's run. Since the meter/instrument state is a single static field on + /// ScadaBridgeTelemetry (Commons), the leak is scoped to whatever process this + /// assembly's test host runs in — it cannot reach StoreAndForward.Tests (a separate + /// assembly with its own test-host process), but it IS visible within Host.Tests, whose + /// MetricsEndpointTests boots a real in-process host (WebApplicationFactory + /// shares this AppDomain, not a child process) and scrapes /metrics, which + /// enumerates every registered gauge including this one. + /// + /// Honesty note on what was and wasn't proven: a search of this assembly (and the whole + /// test tree) found no test today that asserts on the VALUE of + /// scadabridge.store_and_forward.queue.depth from Host.Tests or any other + /// same-process reader — MetricsEndpointTests only asserts the endpoint returns + /// Prometheus-shaped text, not this series' content, and the only test that DOES assert + /// on the value (QueueDepthGaugeTests) lives in StoreAndForward.Tests, a distinct + /// test-host process the leak cannot reach. So there is no currently-reproducible flake + /// to demonstrate here (no fork of this test with the leak left in has an assertion to + /// break) — the defect is a landmine for a FUTURE same-process reader, not a live one. + /// The fix (compare-and-clear in finally) closes the landmine regardless; a + /// [Collection] is intentionally not added since no second same-process reader of + /// this slot exists to serialize against as of this writing — re-run this search before + /// removing the guard below if that changes. + /// [Fact] public void EmitHelpers_DoNotThrow() { + Func queueDepthProvider = () => 5; var ex = Record.Exception(() => { ScadaBridgeTelemetry.RecordDeploymentApplied(); @@ -27,9 +55,20 @@ public class ScadaBridgeTelemetryTests ScadaBridgeTelemetry.LiveAlarmAggregatorStarted(); ScadaBridgeTelemetry.LiveAlarmAggregatorStopped(); ScadaBridgeTelemetry.RecordLiveAlarmStreamReconnect(); - ScadaBridgeTelemetry.SetQueueDepthProvider(() => 5); + ScadaBridgeTelemetry.SetQueueDepthProvider(queueDepthProvider); }); - Assert.Null(ex); + try + { + Assert.Null(ex); + } + finally + { + // Compare-and-clear: only removes OUR delegate from the process-global slot, so + // a legitimate concurrent registrant (a real StoreAndForwardService started + // elsewhere in this process) is never stomped — mirrors + // ScadaBridgeTelemetry.ClearQueueDepthProvider's own doc'd contract. + ScadaBridgeTelemetry.ClearQueueDepthProvider(queueDepthProvider); + } } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs index a9f31382..65f2261c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs @@ -497,6 +497,28 @@ public class KpiHistoryRecorderActorTests : TestKit NullLogger.Instance))); } + /// + /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave + /// SiteAlarmAggregatorActorTests.AssertHoldsFor treatment): proves + /// keeps being true for the whole by + /// polling, rather than a single blind Thread.Sleep(window) followed by one + /// end-of-window sample. A bare sleep-then-assert cannot distinguish "the mechanism + /// correctly stayed quiet" from "the mechanism just hadn't run yet when the one sample + /// was taken", and it can't catch a value that flips wrong and then flips back before + /// that single sample. This fails immediately the first time + /// goes false, and only returns once the full window has elapsed with it still true. + /// + private static void AssertHoldsFor(TimeSpan window, Func holds, string because) + { + var deadline = DateTime.UtcNow + window; + while (true) + { + Assert.True(holds(), because); + if (DateTime.UtcNow >= deadline) return; + Thread.Sleep(20); + } + } + [Fact] public void SampleTick_WritesHealthySourceSamples_AndThrowingSourceDoesNotAbortTick() { @@ -1040,13 +1062,13 @@ public class KpiHistoryRecorderActorTests : TestKit interval: TimeSpan.FromMilliseconds(50)); // …and then skip: no slices are ever folded. - Thread.Sleep(300); - Assert.Equal(0, repository.FoldCount); + AssertHoldsFor(TimeSpan.FromMilliseconds(300), () => repository.FoldCount == 0, + "watermark inside the lookback tail must skip the backfill entirely"); // A second BackfillTick is a no-op (the done-latch is set). actor.Tell(KpiHistoryRecorderActor.BackfillTick.Instance); - Thread.Sleep(300); - Assert.Equal(0, repository.FoldCount); + AssertHoldsFor(TimeSpan.FromMilliseconds(300), () => repository.FoldCount == 0, + "the done-latch must make a second BackfillTick a no-op"); } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs index c18a8a74..3ebe9b03 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs @@ -786,6 +786,29 @@ public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable while (Interlocked.CompareExchange(ref target, value, current) != current); } + /// + /// Register row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave + /// SiteAlarmAggregatorActorTests.AssertHoldsFor treatment, ported here async since + /// this class's facts run on the async test thread): proves keeps + /// being true for the whole by polling, rather than a single + /// blind await Task.Delay(window) followed by one end-of-window sample. A bare + /// delay-then-assert cannot distinguish "the mechanism correctly stayed quiet" from "the + /// mechanism just hadn't run yet when the one sample was taken", and it can't catch a + /// value that flips wrong and then flips back before that single sample. This fails + /// immediately the first time goes false, and only returns once + /// the full window has elapsed with it still true throughout. + /// + private static async Task AssertHoldsForAsync(TimeSpan window, Func holds, string because) + { + var deadline = DateTime.UtcNow + window; + while (true) + { + Assert.True(holds(), because); + if (DateTime.UtcNow >= deadline) return; + await Task.Delay(20); + } + } + // ── Task 12: deferToSweep enqueue mode ── /// @@ -888,8 +911,13 @@ public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable service.TriggerSweep(); // the clobbering kick var stop = service.StopAsync(); - await Task.Delay(300); - Assert.False(stop.IsCompleted); // pre-fix: StopAsync already returned (awaited the no-op) + // The real sweep is blocked on `release` behind `entered`, so `stop` is genuinely + // unable to complete during this window (not merely "hasn't yet") — proving it stays + // incomplete throughout, not just at one end-of-delay sample, is what actually + // distinguishes "waited for the real sweep" from "pre-fix: awaited the no-op and + // returned instantly, then this single sample raced whether that had happened yet". + await AssertHoldsForAsync(TimeSpan.FromMilliseconds(300), () => !stop.IsCompleted, + "pre-fix: StopAsync already returned (awaited the no-op)"); release.TrySetResult(); await stop.WaitAsync(TimeSpan.FromSeconds(5)); // drains the real sweep promptly once released