dohertj2/ScadaBridge
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor(kpi): K4/K10/K12 review fixups — test data-race + faulted-tick liveness, dead-branch/unused removal, NaN-guard assertions, value clamp + doc

Browse Source
This commit is contained in:
Joseph Doherty
2026-06-17 20:15:47 -04:00
parent 5613a5efb7
commit cb2a516187
6 changed files with 105 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/KpiTrendChart.razor.cs
+6 -5
View File
@@ -82,9 +82,6 @@ public partial class KpiTrendChart
/// <summary>Exactly one sample — show the single-sample note, not a polyline.</summary>
private bool HasSingleSample => IsAvailable && Series.Count == 1;
/// <summary>Available + ≥1 point but not a full chart, or unavailable / empty.</summary>
private bool ShowPlaceholder => !IsAvailable || Series.Count == 0;
/// <summary>
/// Stable Playwright hook: <c>kpi-trend-&lt;slug&gt;</c> 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)
src/ZB.MOM.WW.ScadaBridge.Commons/Types/Kpi/KpiSeriesBucketer.cs
+2 -1
View File
@@ -27,7 +27,8 @@ public static class KpiSeriesBucketer
/// An <see cref="IReadOnlyList{T}"/> of at most <paramref name="maxPoints"/> bucketed points,
/// ordered by <see cref="KpiSeriesPoint.BucketStartUtc"/> ascending.
/// Returns <paramref name="raw"/> unchanged (same reference) when
/// <c>raw.Count &lt;= maxPoints</c>.
/// <c>raw.Count &lt;= maxPoints</c>; callers must not mutate the underlying
/// collection in that case, as it is the same object passed in.
/// </returns>
/// <exception cref="ArgumentOutOfRangeException">
/// Thrown when <paramref name="maxPoints"/> &lt; 2 or
src/ZB.MOM.WW.ScadaBridge.KpiHistory/KpiHistoryRecorderActor.cs
+2 -2
View File
@@ -87,9 +87,9 @@ public class KpiHistoryRecorderActor : ReceiveActor, IWithTimers
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
Receive<SampleTick>(_ => HandleSampleTick());
Receive<SampleComplete>(_ => { });
Receive<SampleComplete>(_ => { }); // best-effort: no actor state to reset on completion
Receive<PurgeTick>(_ => HandlePurgeTick());
Receive<PurgeComplete>(_ => { });
Receive<PurgeComplete>(_ => { }); // best-effort: no actor state to reset on completion
}
/// <inheritdoc />
tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/Shared/KpiTrendChartTests.cs
+12 -2
View File
@@ -106,7 +106,12 @@ public class KpiTrendChartTests : BunitContext
.Add(c => c.Title, "Flat Time")
.Add(c => c.IsAvailable, true));
Assert.Contains("<polyline", cut.Markup);
var markup = cut.Markup;
Assert.Contains("<polyline", markup);
// The even-spacing fallback must produce finite coordinates — no NaN or Infinity
// from the divide-by-zero guard path.
Assert.DoesNotContain("NaN", markup);
Assert.DoesNotContain("Infinity", markup);
}
[Fact]
@@ -123,7 +128,12 @@ public class KpiTrendChartTests : BunitContext
.Add(c => c.Title, "Quiet")
.Add(c => c.IsAvailable, true));
Assert.Contains("<polyline", cut.Markup);
var markup = cut.Markup;
Assert.Contains("<polyline", markup);
// The zero-max flat-line guard must produce finite coordinates — no NaN or Infinity
// from the yFrac divide-by-zero protection path.
Assert.DoesNotContain("NaN", markup);
Assert.DoesNotContain("Infinity", markup);
}
[Theory]
tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Kpi/KpiSeriesBucketerTests.cs
+29 -15
View File
@@ -190,33 +190,47 @@ public class KpiSeriesBucketerTests
[Fact]
public void Bucket_PointAtToUtc_LandsInLastBucket()
{
// 60-minute window / 3 buckets → 20 min each.
// A point exactly at T(60) = toUtc must go to bucket 2, not overflow.
// 60-minute window / 2 buckets → 30 min each.
// A point exactly at T(60) = toUtc must go to bucket 1 (the last bucket),
// not overflow to an out-of-range index.
// T(10) → bucket 0: [0, 30); T(35) → bucket 1: [30, 60]; T(60) → bucket 1.
var raw = new[]
{
new KpiSeriesPoint(T(10), 1.0),
new KpiSeriesPoint(T(30), 2.0),
new KpiSeriesPoint(T(60), 99.0), // right edge — must land in last bucket
};
// raw.Count (3) == maxPoints (3) so it normally returns as-is;
// use maxPoints=2 to force downsampling and expose the edge behaviour.
// Window: [T(0), T(60)], 2 buckets → 30 min each.
// T(10) → bucket 0, T(30) → bucket 1, T(60) → bucket 1 (last).
var raw2 = new[]
{
new KpiSeriesPoint(T(10), 5.0),
new KpiSeriesPoint(T(35), 6.0),
new KpiSeriesPoint(T(60), 7.0), // exactly toUtc → bucket 1
new KpiSeriesPoint(T(60), 7.0), // exactly toUtc → must land in last bucket
};
var result = KpiSeriesBucketer.Bucket(raw2, T(0), T(60), maxPoints: 2);
var result = KpiSeriesBucketer.Bucket(raw, T(0), T(60), maxPoints: 2);
Assert.Equal(2, result.Count);
// Bucket 1 holds both T(35) and T(60); T(60) is later → wins.
Assert.Equal(7.0, result[1].Value);
}
[Fact]
public void Bucket_ThreeBucketWindow_PointAtToUtc_LandsInLastBucket()
{
// 60-minute window / 3 buckets → 20 min each.
// A point exactly at T(60) = toUtc must land in bucket 2 (the last),
// not overflow. Use 4 raw points so downsampling is forced (4 > 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
// -----------------------------------------------------------------------
tests/ZB.MOM.WW.ScadaBridge.KpiHistory.Tests/KpiHistoryRecorderActorTests.cs
+54 -14
View File
@@ -64,6 +64,39 @@ public class KpiHistoryRecorderActorTests : TestKit
throw new InvalidOperationException("simulated source failure");
}
/// <summary>
/// A source that throws on the first <see cref="CollectAsync"/> 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 <em>same</em> actor instance.
/// </summary>
private sealed class ThrowOnceSource : IKpiSampleSource
{
private int _callCount;
public string Source => KpiSources.SiteCallAudit;
public Task<IReadOnlyList<KpiSample>> CollectAsync(
DateTime capturedAtUtc, CancellationToken cancellationToken = default)
{
if (Interlocked.Increment(ref _callCount) == 1)
throw new InvalidOperationException("simulated first-call source failure");
IReadOnlyList<KpiSample> samples = new[]
{
new KpiSample
{
Source = Source,
Metric = "RecoveredSample",
Scope = KpiScopes.Global,
ScopeKey = null,
Value = 1,
CapturedAtUtc = capturedAtUtc,
},
};
return Task.FromResult(samples);
}
}
/// <summary>
/// Recording repository fake. Captures the samples handed to
/// <see cref="RecordSamplesAsync"/> and the cut-off handed to
@@ -73,13 +106,19 @@ public class KpiHistoryRecorderActorTests : TestKit
{
private readonly object _gate = new();
private readonly List<KpiSample> _recorded = new();
private DateTime? _purgeCutoff;
public IReadOnlyList<KpiSample> 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<KpiSample> samples, CancellationToken cancellationToken = default)
@@ -98,7 +137,7 @@ public class KpiHistoryRecorderActorTests : TestKit
public Task<int> 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));
}