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

fix(core-abstractions): resolve Low code-review findings (Core.Abstractions-004,005,006,007,008)

Browse Source 
- Core.Abstractions-004: guard DriverTypeRegistry.Register with a Lock so
  concurrent registrations are atomic.
- Core.Abstractions-005: narrow PollGroupEngine catch blocks to non-fatal
  exceptions, add optional onError callback, tolerate disposed-CTS races.
- Core.Abstractions-006: document the deliberate int-vs-uint asymmetry on
  IHistoryProvider.ReadEventsAsync / IHistorianDataSource.ReadEventsAsync.
- Core.Abstractions-007: pin the gaps with PollGroupEngine + DriverHealth
  contract tests.
- Core.Abstractions-008: correct XML docs on DriverHealth.LastError and
  the optional / required asymmetry on the history-read surfaces.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 05:37:54 -04:00
parent a02c0ffe36
commit ff2e75ab98
10 changed files with 422 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverHealthTests.cs Normal file
View File

@@ -0,0 +1,52 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests;
/// <summary>
/// Covers <see cref="DriverHealth"/> shape invariants — added to lock down the documented
/// contract after Core.Abstractions-008 reworded the <c>LastError</c> remark.
/// </summary>
public sealed class DriverHealthTests
{
/// <summary>
/// Core.Abstractions-008: <c>DriverHealth.LastError</c> is not constrained by the
/// <c>State</c> enum — a Healthy driver may legitimately retain the last error from a
/// recovered failure (for diagnostics), and Degraded / Reconnecting / Faulted states may
/// all carry a non-null message. The old XML doc "null when state is Healthy" was wrong;
/// this test makes the type's actual contract explicit so future doc churn cannot drift.
/// </summary>
[Theory]
[InlineData(DriverState.Unknown)]
[InlineData(DriverState.Initializing)]
[InlineData(DriverState.Healthy)]
[InlineData(DriverState.Degraded)]
[InlineData(DriverState.Reconnecting)]
[InlineData(DriverState.Faulted)]
public void LastError_IsIndependent_OfState(DriverState state)
{
var healthWithError = new DriverHealth(state, LastSuccessfulRead: DateTime.UtcNow, LastError: "earlier failure");
var healthWithoutError = new DriverHealth(state, LastSuccessfulRead: DateTime.UtcNow, LastError: null);
// Both shapes are constructible regardless of state — the type makes no enforcement.
healthWithError.LastError.ShouldBe("earlier failure");
healthWithoutError.LastError.ShouldBeNull();
healthWithError.State.ShouldBe(state);
healthWithoutError.State.ShouldBe(state);
}
[Fact]
public void DriverState_EnumContainsExpectedMembers()
{
// Pins the enum so finding-008's "more than Healthy can carry an error" claim
// does not bit-rot.
var names = Enum.GetNames<DriverState>();
names.ShouldContain(nameof(DriverState.Unknown));
names.ShouldContain(nameof(DriverState.Initializing));
names.ShouldContain(nameof(DriverState.Healthy));
names.ShouldContain(nameof(DriverState.Degraded));
names.ShouldContain(nameof(DriverState.Reconnecting));
names.ShouldContain(nameof(DriverState.Faulted));
}
}

74
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverTypeRegistryTests.cs
View File

@@ -1,5 +1,6 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests;
@@ -120,4 +121,77 @@ public sealed class DriverTypeRegistryTests
var registry = new DriverTypeRegistry();
Should.Throw<ArgumentException>(() => registry.Get(typeName!));
}
/// <summary>
/// Core.Abstractions-004: concurrent <see cref="DriverTypeRegistry.Register"/> calls for
/// two different driver types must both succeed and both end up visible to readers. A
/// non-atomic check-then-act would let the second swap silently discard the first
/// registration; this test asserts the "registered only once per process" guarantee
/// holds under concurrent writers too.
/// </summary>
[Fact]
public void Register_ConcurrentDistinctTypes_AllSucceed()
{
var registry = new DriverTypeRegistry();
const int parallelism = 32;
var ready = new ManualResetEventSlim(false);
var threads = Enumerable.Range(0, parallelism)
.Select(i => new Thread(() =>
{
ready.Wait();
registry.Register(SampleMetadata($"Driver-{i}"));
}))
.ToArray();
foreach (var t in threads) t.Start();
ready.Set(); // release all threads simultaneously
foreach (var t in threads) t.Join();
var all = registry.All();
all.Count.ShouldBe(parallelism);
for (var i = 0; i < parallelism; i++)
registry.TryGet($"Driver-{i}").ShouldNotBeNull(
$"Driver-{i} was lost to a race in concurrent Register");
}
/// <summary>
/// Core.Abstractions-004: concurrent <see cref="DriverTypeRegistry.Register"/> calls for
/// the SAME driver type must result in exactly one successful registration and exactly
/// (parallelism - 1) <see cref="InvalidOperationException"/> throws — not a silent
/// last-writer-wins replacement.
/// </summary>
[Fact]
public void Register_ConcurrentDuplicateType_ExactlyOneWins()
{
var registry = new DriverTypeRegistry();
const int parallelism = 16;
var ready = new ManualResetEventSlim(false);
var successes = 0;
var failures = 0;
var threads = Enumerable.Range(0, parallelism)
.Select(_ => new Thread(() =>
{
ready.Wait();
try
{
registry.Register(SampleMetadata("Contended"));
Interlocked.Increment(ref successes);
}
catch (InvalidOperationException)
{
Interlocked.Increment(ref failures);
}
}))
.ToArray();
foreach (var t in threads) t.Start();
ready.Set();
foreach (var t in threads) t.Join();
successes.ShouldBe(1);
failures.ShouldBe(parallelism - 1);
registry.All().Count.ShouldBe(1);
}
}

57
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/IHistorianDataSourceContractTests.cs
View File

@@ -118,4 +118,61 @@ public sealed class IHistorianDataSourceContractTests
healthy.ShouldNotBe(unhealthy);
}
/// <summary>
/// Core.Abstractions-006: the <c>maxValuesPerNode</c> (raw / processed) and <c>maxEvents</c>
/// parameter types are intentionally asymmetric — raw/processed reads use <c>uint</c>
/// because OPC UA HistoryRead's NumValuesPerNode is unsigned, while event reads use
/// <c>int</c> to allow zero / negative as a "use backend default cap" sentinel
/// (see <c>WonderwareHistorianClient</c> / <c>HistorianDataSource</c> usage). This test
/// pins both shapes so accidental changes are caught.
/// </summary>
[Theory]
[InlineData("ReadRawAsync", "maxValuesPerNode", typeof(uint))]
[InlineData("ReadEventsAsync", "maxEvents", typeof(int))]
public void HistoryRead_MaxParameter_TypePinned(string methodName, string parameterName, Type expectedType)
{
var method = typeof(IHistorianDataSource).GetMethod(methodName);
method.ShouldNotBeNull();
var parameter = method!.GetParameters().FirstOrDefault(p => p.Name == parameterName);
parameter.ShouldNotBeNull($"Method {methodName} should expose a '{parameterName}' parameter.");
parameter!.ParameterType.ShouldBe(expectedType);
}
[Theory]
[InlineData("ReadRawAsync", "maxValuesPerNode", typeof(uint))]
[InlineData("ReadEventsAsync", "maxEvents", typeof(int))]
public void HistoryProvider_MaxParameter_TypePinned(string methodName, string parameterName, Type expectedType)
{
var method = typeof(IHistoryProvider).GetMethod(methodName);
method.ShouldNotBeNull();
var parameter = method!.GetParameters().FirstOrDefault(p => p.Name == parameterName);
parameter.ShouldNotBeNull($"Method {methodName} should expose a '{parameterName}' parameter.");
parameter!.ParameterType.ShouldBe(expectedType);
}
/// <summary>
/// Core.Abstractions-008: <see cref="IHistoryProvider.ReadAtTimeAsync"/> and
/// <see cref="IHistoryProvider.ReadEventsAsync"/> are C# default interface methods
/// (drivers opt in), whereas <see cref="IHistorianDataSource.ReadAtTimeAsync"/> and
/// <see cref="IHistorianDataSource.ReadEventsAsync"/> are required (the server-side
/// historian must implement them). This test pins the asymmetry so an implementer cannot
/// accidentally collapse the two surfaces and so the documented rationale stays load-bearing.
/// </summary>
[Theory]
[InlineData("ReadAtTimeAsync")]
[InlineData("ReadEventsAsync")]
public void HistoryProvider_OptionalMethods_HaveDefaultImplementation(string methodName)
{
var providerMethod = typeof(IHistoryProvider).GetMethod(methodName);
providerMethod.ShouldNotBeNull();
// Default interface methods carry a method body — IsAbstract is false.
providerMethod!.IsAbstract.ShouldBeFalse(
$"IHistoryProvider.{methodName} must remain a default-impl-throws method so legacy drivers continue to compile.");
var dataSourceMethod = typeof(IHistorianDataSource).GetMethod(methodName);
dataSourceMethod.ShouldNotBeNull();
dataSourceMethod!.IsAbstract.ShouldBeTrue(
$"IHistorianDataSource.{methodName} must remain required — server-side historians own the full surface.");
}
}

102
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs
View File

@@ -342,6 +342,108 @@ public sealed class PollGroupEngineTests
shortReadCount.ShouldBeGreaterThanOrEqualTo(2);
}
/// <summary>
/// Core.Abstractions-005: the engine documents that "transient poll errors are logged on
/// the driver health surface", but until an error callback exists the driver has no way
/// to observe a caught reader exception. Subscribing without supplying an error callback
/// must continue to swallow exceptions (backward compatible). When an error callback IS
/// supplied, every exception caught during a poll cycle must be routed to it.
/// </summary>
[Fact]
public async Task Reader_exception_is_reported_to_onError_callback()
{
var observed = new ConcurrentQueue<Exception>();
var readCount = 0;
Task<IReadOnlyList<DataValueSnapshot>> Reader(IReadOnlyList<string> refs, CancellationToken ct)
{
if (Interlocked.Increment(ref readCount) <= 3)
throw new InvalidOperationException($"boom-{readCount}");
var now = DateTime.UtcNow;
return Task.FromResult<IReadOnlyList<DataValueSnapshot>>(
refs.Select(_ => new DataValueSnapshot(1, 0u, now, now)).ToList());
}
await using var engine = new PollGroupEngine(
Reader,
(_, _, _) => { },
minInterval: TimeSpan.FromMilliseconds(50),
onError: ex => observed.Enqueue(ex));
var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(50));
await WaitForAsync(() => observed.Count >= 3, TimeSpan.FromSeconds(3));
engine.Unsubscribe(handle);
observed.Count.ShouldBeGreaterThanOrEqualTo(3);
observed.All(e => e is InvalidOperationException).ShouldBeTrue();
observed.All(e => e.Message.StartsWith("boom-")).ShouldBeTrue();
}
/// <summary>
/// Core.Abstractions-005: a contract-violating reader (Core.Abstractions-002 path) that
/// throws the descriptive <see cref="InvalidOperationException"/> from inside the engine
/// must also be routed to the error callback so the driver health surface can observe
/// repeated contract violations.
/// </summary>
[Fact]
public async Task Reader_contract_violation_routes_to_onError_callback()
{
var observed = new ConcurrentQueue<Exception>();
Task<IReadOnlyList<DataValueSnapshot>> Reader(IReadOnlyList<string> refs, CancellationToken ct)
{
// Always return zero snapshots — short-result-list contract violation.
return Task.FromResult<IReadOnlyList<DataValueSnapshot>>(new List<DataValueSnapshot>());
}
await using var engine = new PollGroupEngine(
Reader,
(_, _, _) => { },
minInterval: TimeSpan.FromMilliseconds(50),
onError: ex => observed.Enqueue(ex));
var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(50));
await WaitForAsync(() => observed.Count >= 2, TimeSpan.FromSeconds(2));
engine.Unsubscribe(handle);
observed.Count.ShouldBeGreaterThanOrEqualTo(2);
observed.All(e => e is InvalidOperationException
&& e.Message.Contains("Reader contract violation"))
.ShouldBeTrue();
}
/// <summary>
/// Core.Abstractions-005: the engine must defend itself against an <c>onError</c> handler
/// that itself throws — otherwise a buggy health-surface forwarder would crash the poll
/// loop and silently stall the subscription, defeating the whole point of the callback.
/// </summary>
[Fact]
public async Task OnError_handler_that_throws_does_not_crash_loop()
{
var readCount = 0;
var events = new ConcurrentQueue<string>();
Task<IReadOnlyList<DataValueSnapshot>> Reader(IReadOnlyList<string> refs, CancellationToken ct)
{
if (Interlocked.Increment(ref readCount) <= 2)
throw new InvalidOperationException("boom");
var now = DateTime.UtcNow;
return Task.FromResult<IReadOnlyList<DataValueSnapshot>>(
refs.Select(_ => new DataValueSnapshot(1, 0u, now, now)).ToList());
}
await using var engine = new PollGroupEngine(
Reader,
(_, r, _) => events.Enqueue(r),
minInterval: TimeSpan.FromMilliseconds(50),
onError: _ => throw new ApplicationException("error-handler-bug"));
var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(50));
// Wait long enough for the reader to recover and for the engine to deliver a change.
await WaitForAsync(() => events.Count >= 1, TimeSpan.FromSeconds(3));
engine.Unsubscribe(handle);
events.Count.ShouldBeGreaterThanOrEqualTo(1);
}
private sealed record DummyHandle : ISubscriptionHandle
{
public string DiagnosticId => "dummy";