From cb2a5161876bd725f82e1dfe87d4bfc60880e84f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 17 Jun 2026 20:15:47 -0400 Subject: [PATCH] =?UTF-8?q?refactor(kpi):=20K4/K10/K12=20review=20fixups?= =?UTF-8?q?=20=E2=80=94=20test=20data-race=20+=20faulted-tick=20liveness,?= =?UTF-8?q?=20dead-branch/unused=20removal,=20NaN-guard=20assertions,=20va?= =?UTF-8?q?lue=20clamp=20+=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Components/Shared/KpiTrendChart.razor.cs | 11 +-- .../Types/Kpi/KpiSeriesBucketer.cs | 3 +- .../KpiHistoryRecorderActor.cs | 4 +- .../Components/Shared/KpiTrendChartTests.cs | 14 +++- .../Kpi/KpiSeriesBucketerTests.cs | 44 ++++++++---- .../KpiHistoryRecorderActorTests.cs | 68 +++++++++++++++---- 6 files changed, 105 insertions(+), 39 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/KpiTrendChart.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/KpiTrendChart.razor.cs index 27f2a2cb..56e19117 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/KpiTrendChart.razor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/KpiTrendChart.razor.cs @@ -82,9 +82,6 @@ public partial class KpiTrendChart /// Exactly one sample — show the single-sample note, not a polyline. private bool HasSingleSample => IsAvailable && Series.Count == 1; - /// Available + ≥1 point but not a full chart, or unavailable / empty. - private bool ShowPlaceholder => !IsAvailable || Series.Count == 0; - /// /// Stable Playwright hook: kpi-trend-<slug> where the slug is the /// title lowercased with each run of non-alphanumerics collapsed to a single @@ -175,13 +172,17 @@ public partial class KpiTrendChart var p = Series[i]; // X — by time fraction, or even index spacing when all timestamps equal. + // n >= 2 is guaranteed by the HasChart precondition (Series.Count >= 2), + // so the (n - 1) divisor is always ≥ 1 and the n == 1 arm is unreachable. double xFrac = timeSpanTicks > 0 ? (p.BucketStartUtc.Ticks - firstTicks) / timeSpanTicks - : (n == 1 ? 0.0 : (double)i / (n - 1)); + : (double)i / (n - 1); var x = PadX + (xFrac * plotW); // Y — baseline at 0, top at max. Flat at baseline when max == 0. - double yFrac = max > 0 ? p.Value / max : 0.0; + // Clamp to non-negative so a stale negative Value cannot push a point + // above the baseline or outside the viewBox. + double yFrac = max > 0 ? Math.Max(0, p.Value) / max : 0.0; var y = baselineY - (yFrac * plotH); if (i > 0) diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Kpi/KpiSeriesBucketer.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Kpi/KpiSeriesBucketer.cs index 78e87ba7..9eb512bd 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Kpi/KpiSeriesBucketer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Kpi/KpiSeriesBucketer.cs @@ -27,7 +27,8 @@ public static class KpiSeriesBucketer /// An of at most bucketed points, /// ordered by ascending. /// Returns unchanged (same reference) when - /// raw.Count <= maxPoints. + /// raw.Count <= maxPoints; callers must not mutate the underlying + /// collection in that case, as it is the same object passed in. /// /// /// Thrown when < 2 or diff --git a/src/ZB.MOM.WW.ScadaBridge.KpiHistory/KpiHistoryRecorderActor.cs b/src/ZB.MOM.WW.ScadaBridge.KpiHistory/KpiHistoryRecorderActor.cs index f2f942a0..d85dda0f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.KpiHistory/KpiHistoryRecorderActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.KpiHistory/KpiHistoryRecorderActor.cs @@ -87,9 +87,9 @@ public class KpiHistoryRecorderActor : ReceiveActor, IWithTimers _logger = logger ?? throw new ArgumentNullException(nameof(logger)); Receive(_ => HandleSampleTick()); - Receive(_ => { }); + Receive(_ => { }); // best-effort: no actor state to reset on completion Receive(_ => HandlePurgeTick()); - Receive(_ => { }); + Receive(_ => { }); // best-effort: no actor state to reset on completion } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/Shared/KpiTrendChartTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/Shared/KpiTrendChartTests.cs index ca5d14d2..e60a1e23 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/Shared/KpiTrendChartTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/Shared/KpiTrendChartTests.cs @@ -106,7 +106,12 @@ public class KpiTrendChartTests : BunitContext .Add(c => c.Title, "Flat Time") .Add(c => c.IsAvailable, true)); - Assert.Contains(" c.Title, "Quiet") .Add(c => c.IsAvailable, true)); - Assert.Contains(" 3). + // T(5) → bucket 0: [0,20); T(25) → bucket 1: [20,40); + // T(45) → bucket 2: [40,60]; T(60) → bucket 2 (right edge, later → wins). + var raw = new[] + { + new KpiSeriesPoint(T(5), 1.0), + new KpiSeriesPoint(T(25), 2.0), + new KpiSeriesPoint(T(45), 3.0), + new KpiSeriesPoint(T(60), 99.0), // right edge — must land in last bucket + }; + + var result = KpiSeriesBucketer.Bucket(raw, T(0), T(60), maxPoints: 3); + + Assert.Equal(3, result.Count); + // Bucket 2 holds T(45)=3.0 and T(60)=99.0; T(60) is later → wins. + Assert.Equal(99.0, result[2].Value); + } + // ----------------------------------------------------------------------- // Empty buckets omitted — no gap-filling // ----------------------------------------------------------------------- diff --git a/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs index d244e701..80f21421 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs @@ -64,6 +64,39 @@ public class KpiHistoryRecorderActorTests : TestKit throw new InvalidOperationException("simulated source failure"); } + /// + /// A source that throws on the first call and returns a + /// healthy sample on every subsequent call. Used to drive a faulted first tick followed + /// by a healthy second tick on the same actor instance. + /// + private sealed class ThrowOnceSource : IKpiSampleSource + { + private int _callCount; + + public string Source => KpiSources.SiteCallAudit; + + public Task> CollectAsync( + DateTime capturedAtUtc, CancellationToken cancellationToken = default) + { + if (Interlocked.Increment(ref _callCount) == 1) + throw new InvalidOperationException("simulated first-call source failure"); + + IReadOnlyList samples = new[] + { + new KpiSample + { + Source = Source, + Metric = "RecoveredSample", + Scope = KpiScopes.Global, + ScopeKey = null, + Value = 1, + CapturedAtUtc = capturedAtUtc, + }, + }; + return Task.FromResult(samples); + } + } + /// /// Recording repository fake. Captures the samples handed to /// and the cut-off handed to @@ -73,13 +106,19 @@ public class KpiHistoryRecorderActorTests : TestKit { private readonly object _gate = new(); private readonly List _recorded = new(); + private DateTime? _purgeCutoff; public IReadOnlyList Recorded { get { lock (_gate) { return _recorded.ToArray(); } } } - public DateTime? PurgeCutoff { get; private set; } + // PurgeOlderThanAsync runs on a threadpool thread; guard the field with + // the same _gate lock used by _recorded so test-thread reads are race-free. + public DateTime? PurgeCutoff + { + get { lock (_gate) { return _purgeCutoff; } } + } public Task RecordSamplesAsync( IReadOnlyCollection samples, CancellationToken cancellationToken = default) @@ -98,7 +137,7 @@ public class KpiHistoryRecorderActorTests : TestKit public Task PurgeOlderThanAsync(DateTime before, CancellationToken cancellationToken = default) { - PurgeCutoff = before; + lock (_gate) { _purgeCutoff = before; } return Task.FromResult(0); } } @@ -188,27 +227,28 @@ public class KpiHistoryRecorderActorTests : TestKit [Fact] public void FaultedTick_DoesNotCrashActor_AndSubsequentTickStillRuns() { + // ThrowOnceSource throws on the first CollectAsync call and returns a healthy + // sample on every subsequent call. This lets us send two ticks to the SAME + // actor instance and verify that: + // • The first tick (faulted source) records nothing but does not crash the actor. + // • The second tick reaches the same actor and records the recovered sample, + // proving the singleton's message loop is still alive after a faulted pass. var repository = new RecordingRepository(); - // A pass containing ONLY a throwing source records nothing but must not crash the - // actor; a later healthy tick proves the singleton survived. - var sp = BuildServiceProvider(repository, new ThrowingSource()); + var sp = BuildServiceProvider(repository, new ThrowOnceSource()); var actor = CreateActor(sp); - // First tick: the only source throws — caught per-source, nothing written, actor lives. + // First tick: source throws on first call — caught per-source, nothing written, actor lives. actor.Tell(KpiHistoryRecorderActor.SampleTick.Instance); AwaitAssert( () => Assert.Empty(repository.Recorded), - duration: TimeSpan.FromSeconds(1), + duration: TimeSpan.FromSeconds(2), interval: TimeSpan.FromMilliseconds(50)); - // Second tick on a fresh actor backed by a healthy source proves the message loop is - // still alive and the recorder still records after a faulted pass on the prior actor. - var healthyRepo = new RecordingRepository(); - var healthySp = BuildServiceProvider(healthyRepo, new HealthySource()); - var healthyActor = CreateActor(healthySp); - healthyActor.Tell(KpiHistoryRecorderActor.SampleTick.Instance); + // Second tick to the SAME actor: source now returns a healthy sample. + // AwaitAssert confirms the actor processed the message and recorded it. + actor.Tell(KpiHistoryRecorderActor.SampleTick.Instance); AwaitAssert( - () => Assert.Equal(2, healthyRepo.Recorded.Count), + () => Assert.Single(repository.Recorded), duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); }