From 65b19516c8721a5ab1394440fff09055787cff1c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 12:44:14 -0400 Subject: [PATCH] =?UTF-8?q?test(storeandforward):=20isolate=20the=20proces?= =?UTF-8?q?s-global=20queue-depth=20gauge=20=E2=80=94=20register=20row=203?= =?UTF-8?q?2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QueueDepthGaugeTests failed intermittently whenever a sibling test class started a StoreAndForwardService concurrently. `scadabridge.store_and_forward.queue.depth` is an ObservableGauge on ScadaBridgeTelemetry's static meter reading ONE process-global provider slot; every StartAsync writes it, last registrant wins. A concurrent StartAsync clobbered the gauge mid-assertion and the test read the sibling's depth (observed: expected 0, actual 6 — 2 runs in 5 with parallel collections enabled). The global slot cannot be swapped for a per-test meter: three of the six facts assert the slot-ownership semantics themselves (compare-and-clear identity, stop/restart re-seed), so the slot IS their subject. The correct isolation is mutual exclusion — QueueDepthGaugeTests, StoreAndForwardServiceTests and CachedCallAttemptEmissionTests (the three classes that call StartAsync) now share the QueueDepthGaugeCollection, which xUnit runs serially; the other eight classes stay parallel. With real isolation in place the assembly-wide `parallelizeTestCollections: false` blanket switch is no longer needed and is flipped back on, so the isolation is actually exercised rather than papered over. QueueDepthGaugeCollectionGuardTests is a staleness self-test: it scans this project's sources (anchored by [CallerFilePath], not the output dir) for files that both construct and start a service, and fails if one omits the collection attribute — so a new test class cannot silently reintroduce the flake. Negative-controlled: dropping the attribute from StoreAndForwardServiceTests makes the guard fail with the offending file name, and the original gauge flake reappears in the same run. Verification: 8 consecutive full-suite runs green with parallel collections ON (135 tests), plus 5 more after the negative control was reverted. Test-only change; no src/ involvement. --- .../CachedCallAttemptEmissionTests.cs | 6 + .../QueueDepthGaugeCollection.cs | 124 ++++++++++++++++++ .../QueueDepthGaugeTests.cs | 6 + .../StoreAndForwardServiceTests.cs | 6 + .../xunit.runner.json | 2 +- 5 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeCollection.cs diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/CachedCallAttemptEmissionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/CachedCallAttemptEmissionTests.cs index f01746fa..3ee0ae61 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/CachedCallAttemptEmissionTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/CachedCallAttemptEmissionTests.cs @@ -17,7 +17,13 @@ namespace ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests; /// DefaultRetryInterval=Zero so the timer-driven retry sweep is /// short-circuited by directly invoking /// . +/// +/// Register row 32: one fact here calls StartAsync, which writes the +/// process-global queue-depth provider slot, so this class joins +/// to stay serialized against +/// . /// +[Collection(QueueDepthGaugeCollection.Name)] public class CachedCallAttemptEmissionTests : IAsyncLifetime, IDisposable { private readonly TestLocalDb _localDb; diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeCollection.cs b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeCollection.cs new file mode 100644 index 00000000..3baf5298 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeCollection.cs @@ -0,0 +1,124 @@ +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; + +namespace ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests; + +/// +/// Deferred-work register row 32 — isolation for the process-global StoreAndForward +/// queue-depth gauge. +/// +/// +/// scadabridge.store_and_forward.queue.depth is an +/// created once on ScadaBridgeTelemetry's static meter, reading a single +/// process-global provider slot (SetQueueDepthProvider / +/// ClearQueueDepthProvider). Every StoreAndForwardService.StartAsync +/// writes that one slot — last registrant wins. That is the PRODUCT design (one S&F +/// engine per node), and three of ' facts assert +/// exactly the slot-ownership semantics (compare-and-clear identity, stop/restart +/// re-seed), so they cannot be rewritten against a per-test meter without changing +/// what they prove — the global slot IS their subject. +/// +/// +/// +/// The resource therefore needs mutual exclusion, not duplication: every test class in +/// this assembly that starts a StoreAndForwardService (and so writes the slot) +/// joins this collection, which xUnit runs serially with respect to itself. Classes +/// that never start a service stay free to run in parallel. Before this, a concurrent +/// StartAsync in a sibling class clobbered the gauge mid-assertion and +/// Gauge_TracksBufferedDepth_AcrossEnqueueDrainAndPark read the sibling's depth +/// (observed: expected 0, actual 6 — roughly 2 runs in 5 with +/// parallelizeTestCollections: true). +/// +/// +/// +/// keeps the membership honest: a new +/// test class that starts a service without joining this collection fails the guard +/// rather than silently reintroducing the flake. +/// +/// +[CollectionDefinition(Name)] +public sealed class QueueDepthGaugeCollection +{ + /// Collection name; referenced from [Collection(...)] on member classes. + public const string Name = "StoreAndForward queue-depth gauge (process-global provider slot)"; +} + +/// +/// Staleness self-test for : asserts that every +/// source file in this test project which both constructs a StoreAndForwardService +/// and calls StartAsync — i.e. writes the process-global queue-depth provider +/// slot — declares [Collection(QueueDepthGaugeCollection.Name)]. +/// +/// +/// The project directory is resolved from (baked in +/// at compile time by the compiler that built these sources), not from the test host's +/// working directory or the output folder, so the scan finds the sources that produced +/// this assembly. +/// +/// +public class QueueDepthGaugeCollectionGuardTests +{ + /// Absolute path of this source file, captured at compile time. + private static string ThisFilePath([CallerFilePath] string path = "") => path; + + /// Matches a construction of the service under a global-slot-writing test. + private static readonly Regex ConstructsService = + new(@"new\s+StoreAndForwardService\s*\(", RegexOptions.Compiled); + + /// Matches a start call, which is what actually writes the global provider slot. + private static readonly Regex StartsService = + new(@"\.StartAsync\s*\(", RegexOptions.Compiled); + + /// Matches the collection attribute that puts a class in the serialized group. + private static readonly Regex DeclaresCollection = + new(@"\[\s*Collection\s*\(\s*QueueDepthGaugeCollection\.Name\s*\)\s*\]", RegexOptions.Compiled); + + [Fact] + public void EverySourceFileThatStartsAService_JoinsTheGaugeCollection() + { + var projectDir = Path.GetDirectoryName(ThisFilePath())!; + Assert.True(Directory.Exists(projectDir), + $"Guard could not locate the test project sources at '{projectDir}'. " + + "The [CallerFilePath] anchor must point at this project's directory."); + + var offenders = new List(); + var members = new List(); + + foreach (var file in Directory.EnumerateFiles(projectDir, "*.cs", SearchOption.AllDirectories)) + { + // Skip generated output (bin/obj) and this guard's own definition file. + var relative = Path.GetRelativePath(projectDir, file); + if (relative.StartsWith("bin", StringComparison.Ordinal) || + relative.StartsWith("obj", StringComparison.Ordinal)) + { + continue; + } + + var text = File.ReadAllText(file); + if (!ConstructsService.IsMatch(text) || !StartsService.IsMatch(text)) + { + continue; + } + + if (DeclaresCollection.IsMatch(text)) + { + members.Add(relative); + } + else + { + offenders.Add(relative); + } + } + + // Sanity: the scan must actually find files, or a rename has silently defanged it. + Assert.True(members.Count + offenders.Count > 0, + "Guard found no source file constructing and starting a StoreAndForwardService — " + + "the scan patterns are stale."); + + Assert.True(offenders.Count == 0, + "These test sources start a StoreAndForwardService (writing the process-global " + + $"queue-depth provider slot) without joining [Collection(QueueDepthGaugeCollection.Name)]: " + + string.Join(", ", offenders) + + ". Add the attribute, or the queue-depth gauge assertions will flake under parallel collections."); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeTests.cs index 6b435f5f..a58524ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/QueueDepthGaugeTests.cs @@ -17,7 +17,13 @@ namespace ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests; /// and does no I/O, which is the whole point of caching the count). /// seeds the counter from storage and registers the provider against this service /// instance, so the gauge resolves to this test's counter. +/// +/// Register row 32: that provider slot is process-global, so this class joins +/// along with every other class in this assembly +/// that starts a service — see that type for why mutual exclusion (rather than a per-test +/// meter) is the correct isolation here. /// +[Collection(QueueDepthGaugeCollection.Name)] public class QueueDepthGaugeTests : IAsyncLifetime, IDisposable { private readonly TestLocalDb _localDb; diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs index f24032d6..c18a8a74 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/StoreAndForwardServiceTests.cs @@ -9,7 +9,13 @@ namespace ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests; /// /// WP-10/12/13/14: Tests for the StoreAndForwardService retry engine and management. +/// +/// Register row 32: several facts here call StartAsync, which writes the +/// process-global queue-depth provider slot, so this class joins +/// to stay serialized against +/// . /// +[Collection(QueueDepthGaugeCollection.Name)] public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable { private readonly TestLocalDb _localDb; diff --git a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/xunit.runner.json b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/xunit.runner.json index 08c512b3..4c6a0fdf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/xunit.runner.json +++ b/tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests/xunit.runner.json @@ -1,4 +1,4 @@ { "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json", - "parallelizeTestCollections": false + "parallelizeTestCollections": true }