From 26e3a1b9a6b2af910c173899ddcc933fabe56b1b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:27:22 -0400 Subject: [PATCH] =?UTF-8?q?test(siteruntime):=20close=20register=20row=209?= =?UTF-8?q?1=20=E2=80=94=20compile-cache=20asserts=20become=20key-scoped?= =?UTF-8?q?=20deltas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SiteScriptCompileCacheTests.GetOrAdd_SameCodeAndGlobals_ComputesOnce asserted Assert.Equal(1, SiteScriptCompileCache.Hits) after a Clear() on what is a process-wide static cache. The [Collection("SiteScriptCompileCache")] attribute serializes this class against ScriptCompilationServiceTests and DeploymentWarmThenGateTests, but NOT against the rest of the assembly: every class that compiles a script — Instance/Script actor tests, deployment tests — goes through the same GetOrAdd in parallel and bumps the same global Hits/Count. Failed ~1 run in 4 under a full parallel solution run, green in isolation. Fixed with the register's row-74 pattern (98c94588): assert the DELTA attributable to this test's OWN keys, never an absolute global value. Two habits carry it. 1. Every test now keys its entries on a per-test-instance guid token (the new Code() helper embeds it as a comment), so no other class in the assembly can collide with or serve them. That makes the factory-call counts and Assert.Same identity checks exact and fully owned — and those are the real claims: "computed once" and "the second lookup returned that instance". 2. The process-global counters are only ever asserted as one-sided bounds (>= baseline + n), since foreign traffic can only add. There is no concurrent Clear() to make a delta go negative: all three classes that call Clear() are in the serialized collection. Four assertions changed shape, and two got stronger rather than weaker. GetOrAdd_SameCode_DifferentGlobals_AreSeparateEntries asserted Count == 2 and Hits == 0 — both only true while nothing else was compiling. "No cross-globals hit" is now asserted where it is observable and ours: the second lookup HAD to compute, the two results are not the same instance, and re-getting either serves its own entry rather than the other globals surface's. Overflow_StaysWithinTheBound gained an eviction probe over its own keys — a bound with foreign slack is not proof a sweep ran; a recompute of an entry we inserted is. Overflow_EvictsOldestBatch_… had Assert.Equal(hitsBefore + 1, Hits), the same absolute-delta shape, now a lower bound alongside the sharp Assert.Same. DeploymentWarmThenGateTests carried the identical latent shape one line before its own delta assertion — Assert.Equal(0, Hits) immediately after Clear() — and is converted to a baseline snapshot too. SiteRuntime.Tests: 633/633 green, twice, at full parallelism (43 s, 42 s). --- .../Actors/DeploymentWarmThenGateTests.cs | 11 +- .../Scripts/SiteScriptCompileCacheTests.cs | 118 ++++++++++++++---- 2 files changed, 105 insertions(+), 24 deletions(-) diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs index 58f23c41..7028e886 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs @@ -248,15 +248,20 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable "BatchTwo", ConfigJson("BatchTwo", sharedCode), "d2", "h2", true); SiteScriptCompileCache.Clear(); - Assert.Equal(0, SiteScriptCompileCache.Hits); + // Baseline rather than Assert.Equal(0, Hits): Hits is a process-global counter and test + // classes outside this collection compile scripts in parallel, so a foreign hit can land + // between the Clear() and the read (deferred-work register row 91). Everything below is + // asserted as a delta over this baseline. + var hitsBefore = SiteScriptCompileCache.Hits; CreateDeploymentManager(); AwaitAssert(() => { // One compile, then repeated hits: the second pre-warm plus both PreStarts. - Assert.True(SiteScriptCompileCache.Hits >= 3, - $"expected the pre-warmed body to be served from cache, saw {SiteScriptCompileCache.Hits} hits"); + Assert.True(SiteScriptCompileCache.Hits - hitsBefore >= 3, + $"expected the pre-warmed body to be served from cache, saw " + + $"{SiteScriptCompileCache.Hits - hitsBefore} hits over baseline {hitsBefore}"); }, TimeSpan.FromSeconds(20)); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs index d25f7e9c..125b415b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs @@ -7,39 +7,98 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts; /// /// Tests for the process-wide . The cache is /// static/process-wide state, so this class shares the "SiteScriptCompileCache" xunit -/// collection with ScriptCompilationServiceTests (no cross-class parallelism) and -/// calls at the top of each test. +/// collection with ScriptCompilationServiceTests and DeploymentWarmThenGateTests +/// (no cross-class parallelism between those three, so no concurrent +/// ). /// +/// +/// +/// Deferred-work register row 91. That collection does NOT stop the rest of the +/// assembly: any test class that compiles a script — Instance/Script actor tests, deployment +/// tests — goes through too, in parallel, and +/// bumps the same process-global / +/// . Absolute assertions such as +/// Assert.Equal(1, Hits) after a Clear() therefore only held while nothing else +/// happened to be compiling; they failed roughly 1 run in 4 of a full parallel solution run and +/// were always green in isolation. +/// +/// +/// The fix follows the register's row-74 pattern: assert the DELTA attributable to this test's +/// OWN keys across its own scoped action, never an absolute global value. Two habits carry it: +/// every test keys its cache entries on a per-test-instance unique token (), +/// so no other class can collide with or serve them; and the global counters are only ever +/// asserted as one-sided bounds (>= baseline + n), since foreign traffic can only add. +/// A collection fixture spanning every script-compiling class in the assembly would fix it too, +/// but would serialise most of the suite for the benefit of five facts. +/// +/// [Collection("SiteScriptCompileCache")] public class SiteScriptCompileCacheTests { + /// + /// Slack allowed on bounds for entries inserted + /// by test classes outside this collection while a bound test runs (register row 91). The + /// exactness of the eviction claims comes from the per-key probes, not from this bound. + /// + private const int ForeignEntrySlack = 512; + + /// Per-test-instance token making every cache key in this test uniquely ours. + private readonly string _uniq = Guid.NewGuid().ToString("N"); + + /// Builds a cache key body that no other test class in the assembly can produce. + private string Code(string body) => $"/*{_uniq}*/ {body}"; + private static ScriptCompilationResult Ok() => ScriptCompilationResult.Succeeded(CSharpScript.Create("1", ScriptOptions.Default, typeof(ScriptGlobals))); [Fact] public void GetOrAdd_SameCodeAndGlobals_ComputesOnce() { - SiteScriptCompileCache.Clear(); + var code = Code("return 1;"); + var hitsBefore = SiteScriptCompileCache.Hits; var factoryCalls = 0; ScriptCompilationResult Factory() { factoryCalls++; return Ok(); } - var r1 = SiteScriptCompileCache.GetOrAdd("return 1;", typeof(ScriptGlobals), Factory); - var r2 = SiteScriptCompileCache.GetOrAdd("return 1;", typeof(ScriptGlobals), Factory); + var r1 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); + var r2 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); + // Scoped to our own key, so these two are exact and are the real claim: one compile, + // and the second lookup returned that very instance rather than a re-computation. Assert.Equal(1, factoryCalls); Assert.Same(r1, r2); - Assert.Equal(1, SiteScriptCompileCache.Hits); + + // Hits is process-global; a parallel class's compile can only push the delta up, so the + // claim "the second lookup was counted as a hit" is a lower bound, not an equality. + Assert.True(SiteScriptCompileCache.Hits >= hitsBefore + 1, + $"expected the second GetOrAdd to register a hit; hits went {hitsBefore} -> {SiteScriptCompileCache.Hits}"); } [Fact] public void GetOrAdd_SameCode_DifferentGlobals_AreSeparateEntries() { - SiteScriptCompileCache.Clear(); - SiteScriptCompileCache.GetOrAdd("1 > 0", typeof(ScriptGlobals), Ok); - SiteScriptCompileCache.GetOrAdd("1 > 0", typeof(TriggerExpressionGlobals), Ok); + var code = Code("1 > 0"); + var scriptCompiles = 0; + var triggerCompiles = 0; - Assert.Equal(2, SiteScriptCompileCache.Count); - Assert.Equal(0, SiteScriptCompileCache.Hits); // no cross-globals hit + var script = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), + () => { scriptCompiles++; return Ok(); }); + var trigger = SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + () => { triggerCompiles++; return Ok(); }); + + // "No cross-globals hit" asserted where it is actually observable and ours: the second + // lookup had to compute, and the two globals surfaces hold distinct entries. (The old + // form — Count == 2 and Hits == 0 on the process-global counters — was only true while + // no other class was compiling.) + Assert.Equal(1, scriptCompiles); + Assert.Equal(1, triggerCompiles); + Assert.NotSame(script, trigger); + + // Each entry is independently addressable afterwards: re-getting either must serve the + // matching cached result, never the other globals surface's. + Assert.Same(script, SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), + () => throw new InvalidOperationException("script entry was not cached"))); + Assert.Same(trigger, SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + () => throw new InvalidOperationException("trigger entry was not cached"))); } [Fact] @@ -47,9 +106,22 @@ public class SiteScriptCompileCacheTests { SiteScriptCompileCache.Clear(); for (var i = 0; i <= SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); - Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries); + // MaxEntries + 1 distinct inserts must not all be resident — the sweep ran. The bound + // carries slack for foreign inserts (register row 91); the sharp claim is the eviction + // probe below, which is scoped to keys only this test can produce. + Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + ForeignEntrySlack, + $"cache grew past its bound: {SiteScriptCompileCache.Count} vs max {SiteScriptCompileCache.MaxEntries}"); + + var recomputed = 0; + for (var i = 0; i < 16; i++) + { + var code = Code($"return {i};"); + SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); + } + + Assert.True(recomputed > 0, "no oldest-batch entry was evicted — the overflow sweep did not run"); } // ── WP3.1: approximate LRU replaced the wholesale-Clear overflow cliff ────────── @@ -70,15 +142,15 @@ public class SiteScriptCompileCacheTests // Fill to the bound. Entries 1..N are inserted oldest-first. for (var i = 0; i < SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); // Touch entry 0 so it carries the NEWEST access stamp despite being inserted first. - var hot = SiteScriptCompileCache.GetOrAdd("return 0;", typeof(ScriptGlobals), Ok); + var hot = SiteScriptCompileCache.GetOrAdd(Code("return 0;"), typeof(ScriptGlobals), Ok); var hitsAfterTouch = SiteScriptCompileCache.Hits; // Push well past the bound so at least one eviction sweep definitely runs. for (var i = 0; i < 200; i++) - SiteScriptCompileCache.GetOrAdd($"return overflow{i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return overflow{i};"), typeof(ScriptGlobals), Ok); // Nothing was cleared wholesale: the cache is still most of the way full and the hit // counter survived (Clear() would have reset it to 0). @@ -89,16 +161,20 @@ public class SiteScriptCompileCacheTests // The touched entry survived the sweep — a hit, not a recompile. This is the LRU // property: recency, not insertion order, decides what stays. var hitsBefore = SiteScriptCompileCache.Hits; - var again = SiteScriptCompileCache.GetOrAdd("return 0;", typeof(ScriptGlobals), + var again = SiteScriptCompileCache.GetOrAdd(Code("return 0;"), typeof(ScriptGlobals), () => throw new InvalidOperationException("hot entry was evicted — LRU is not keeping recently-used entries")); + // Assert.Same is the sharp claim (our own key, our own instance). The counter check is a + // lower bound: Hits is process-global and a parallel class's hit lands in the same window + // (register row 91 — this was `Assert.Equal(hitsBefore + 1, …)`). Assert.Same(hot, again); - Assert.Equal(hitsBefore + 1, SiteScriptCompileCache.Hits); + Assert.True(SiteScriptCompileCache.Hits >= hitsBefore + 1, + $"expected the surviving hot entry to register a hit; hits went {hitsBefore} -> {SiteScriptCompileCache.Hits}"); // …and untouched old entries genuinely were evicted, so the sweep really ran. var recomputed = 0; for (var i = 1; i <= 16; i++) { - var code = $"return {i};"; + var code = Code($"return {i};"); SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); } @@ -119,7 +195,7 @@ public class SiteScriptCompileCacheTests var tasks = Enumerable.Range(0, 8).Select(t => Task.Run(() => { for (var i = 0; i < perTask; i++) - SiteScriptCompileCache.GetOrAdd($"return {t}_{i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {t}_{i};"), typeof(ScriptGlobals), Ok); })); await Task.WhenAll(tasks); @@ -131,7 +207,7 @@ public class SiteScriptCompileCacheTests // eviction bug would show up here as unbounded growth, not as a handful of extra rows. // (Other test classes compile into this process-wide cache concurrently, which is the // other reason the bound is asserted with slack rather than exactly.) - Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + 512, + Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + ForeignEntrySlack, $"cache grew past its bound: {SiteScriptCompileCache.Count} vs max {SiteScriptCompileCache.MaxEntries} " + $"after {8 * perTask} distinct inserts"); }