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

fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-009)

Browse Source 
Add six previously-missing edge-case tests to WonderwareHistorianClientTests:
(2) WriteBatchAsync transport-drop catch path returns RetryPlease for all events;
(3) InvokeAsync second-attempt-also-fails propagates the exception;
(4) stalled sidecar fires OperationCanceledException within CallTimeout;
(5) HistoryAggregateType.Total throws NotSupportedException via ReadProcessedAsync;
(6) sidecar wrong-MessageKind reply throws InvalidDataException.

Extend FakeSidecarServer with DisconnectBeforeReply, ReplyWithWrongKind, and
StallAfterRequest test knobs to support these scenarios.

Add ContractsWireParityTests.cs (11 tests) to pin the MessagePack byte layout,
round-trip correctness, MessageKind enum values, and Framing constants — catching
silent [Key] index drift between the client and sidecar mirror copies without
requiring a cross-TFM (net10 vs net48) project reference.

Test count grew from 11 to 27; all 27 pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 09:26:56 -04:00
parent 03c2028669
commit 1c6db86631
4 changed files with 399 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
code-reviews/Driver.Historian.Wonderware.Client/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 9 |
| Open findings | 5 |
## Checklist coverage
@@ -241,7 +241,7 @@ can be dropped.
| Severity | Medium |
| Category | Testing coverage |
| Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The suite covers happy paths, server-error, bad-secret, a single
reconnect and health counters, but several critical paths are untested:
@@ -263,7 +263,7 @@ wire-parity test the source comments commit to: serialize each DTO with the clie
and assert byte-equality against the sidecar `Driver.Historian.Wonderware.Ipc` copy, so a
silent `[Key]` drift between the two duplicated contract sets is caught at build time.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — added six missing tests to `WonderwareHistorianClientTests.cs` (WriteBatchAsync transport-drop catch path returns RetryPlease; InvokeAsync both-attempts-fail propagates exception; stalled sidecar fires OperationCanceledException within CallTimeout; ReadProcessedAsync Total aggregate throws NotSupportedException; sidecar wrong-kind reply throws InvalidDataException) and extended `FakeSidecarServer` with `DisconnectBeforeReply`, `ReplyWithWrongKind`, and `StallAfterRequest` test knobs; added new `ContractsWireParityTests.cs` with 11 tests pinning MessagePack byte layout, round-trip correctness, MessageKind enum values, and Framing constants to catch silent `[Key]` index drift between the client and sidecar mirror copies. Total test count grew from 11 to 27, all passing.
### Driver.Historian.Wonderware.Client-010

222
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/ContractsWireParityTests.cs Normal file
View File

@@ -0,0 +1,222 @@
using MessagePack;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Ipc;
namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests;
/// <summary>
/// Wire-parity tests for the client-side IPC contracts (Contracts.cs + Framing.cs).
/// These tests pin the MessagePack byte representation of each DTO using known inputs
/// and assert byte-equality against expected values. Because the sidecar (.NET 4.8)
/// carries a byte-identical mirror of these DTOs, a silent <c>[Key]</c> index drift or
/// field-type change in either copy would cause a mismatch here and be caught at build
/// time — without needing to reference the net48 sidecar assembly from a net10 test
/// project (which the TFM mismatch prevents). (Finding 009.)
/// </summary>
public sealed class ContractsWireParityTests
{
// ---- HistorianSampleDto ----
// Fields at Key(0)=ValueBytes(null), Key(1)=Quality(0), Key(2)=TimestampUtcTicks(0)
// MessagePack fixarray(3) + nil + fixint(0) + fixint(0) = 93 c0 00 00
[Fact]
public void HistorianSampleDto_SerializedBytes_AreStable()
{
var dto = new HistorianSampleDto { ValueBytes = null, Quality = 0, TimestampUtcTicks = 0 };
var bytes = MessagePackSerializer.Serialize(dto);
// fixarray(3) = 0x93, nil = 0xC0, fixint(0) = 0x00, fixint(0) = 0x00
bytes.ShouldBe(new byte[] { 0x93, 0xC0, 0x00, 0x00 });
}
[Fact]
public void HistorianSampleDto_WithValue_RoundTrips()
{
var original = new HistorianSampleDto
{
ValueBytes = MessagePackSerializer.Serialize<object>(42.5),
Quality = 192,
TimestampUtcTicks = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc).Ticks,
};
var bytes = MessagePackSerializer.Serialize(original);
var roundTripped = MessagePackSerializer.Deserialize<HistorianSampleDto>(bytes);
roundTripped.Quality.ShouldBe((byte)192);
roundTripped.TimestampUtcTicks.ShouldBe(original.TimestampUtcTicks);
roundTripped.ValueBytes.ShouldBe(original.ValueBytes);
}
// ---- HistorianAggregateSampleDto ----
// Key(0)=Value(null), Key(1)=TimestampUtcTicks(0)
// fixarray(2) + nil + fixint(0) = 92 c0 00
[Fact]
public void HistorianAggregateSampleDto_SerializedBytes_AreStable()
{
var dto = new HistorianAggregateSampleDto { Value = null, TimestampUtcTicks = 0 };
var bytes = MessagePackSerializer.Serialize(dto);
// fixarray(2) = 0x92, nil = 0xC0, fixint(0) = 0x00
bytes.ShouldBe(new byte[] { 0x92, 0xC0, 0x00 });
}
// ---- ReadRawRequest ----
// 5 fields at Key(0..4). TagName="", StartUtcTicks=0, EndUtcTicks=0, MaxValues=0, CorrelationId=""
// fixarray(5) + fixstr(0)="" + fixint(0) + fixint(0) + fixint(0) + fixstr(0)=""
[Fact]
public void ReadRawRequest_EmptyInstance_SerializesAsFixArray5()
{
var req = new ReadRawRequest();
var bytes = MessagePackSerializer.Serialize(req);
// Should start with fixarray(5) = 0x95
bytes[0].ShouldBe((byte)0x95);
// Round-trip verification
var rt = MessagePackSerializer.Deserialize<ReadRawRequest>(bytes);
rt.TagName.ShouldBe(string.Empty);
rt.MaxValues.ShouldBe(0);
}
[Fact]
public void ReadRawRequest_WithValues_RoundTrips()
{
var original = new ReadRawRequest
{
TagName = "Tank.Level",
StartUtcTicks = 100L,
EndUtcTicks = 200L,
MaxValues = 500,
CorrelationId = "abc",
};
var bytes = MessagePackSerializer.Serialize(original);
var rt = MessagePackSerializer.Deserialize<ReadRawRequest>(bytes);
rt.TagName.ShouldBe("Tank.Level");
rt.StartUtcTicks.ShouldBe(100L);
rt.EndUtcTicks.ShouldBe(200L);
rt.MaxValues.ShouldBe(500);
rt.CorrelationId.ShouldBe("abc");
}
// ---- ReadRawReply ----
[Fact]
public void ReadRawReply_RoundTrips()
{
var original = new ReadRawReply
{
CorrelationId = "x",
Success = true,
Error = null,
Samples = [new HistorianSampleDto { Quality = 192, TimestampUtcTicks = 99L }],
};
var bytes = MessagePackSerializer.Serialize(original);
var rt = MessagePackSerializer.Deserialize<ReadRawReply>(bytes);
rt.CorrelationId.ShouldBe("x");
rt.Success.ShouldBeTrue();
rt.Error.ShouldBeNull();
rt.Samples.Length.ShouldBe(1);
rt.Samples[0].Quality.ShouldBe((byte)192);
rt.Samples[0].TimestampUtcTicks.ShouldBe(99L);
}
// ---- ReadAtTimeRequest / ReadAtTimeReply ----
[Fact]
public void ReadAtTimeRequest_RoundTrips()
{
var ticks = new long[] { 100L, 200L, 300L };
var original = new ReadAtTimeRequest { TagName = "T", TimestampsUtcTicks = ticks, CorrelationId = "c" };
var bytes = MessagePackSerializer.Serialize(original);
var rt = MessagePackSerializer.Deserialize<ReadAtTimeRequest>(bytes);
rt.TagName.ShouldBe("T");
rt.TimestampsUtcTicks.ShouldBe(ticks);
rt.CorrelationId.ShouldBe("c");
}
// ---- WriteAlarmEventsRequest / WriteAlarmEventsReply ----
[Fact]
public void WriteAlarmEventsRequest_RoundTrips()
{
var original = new WriteAlarmEventsRequest
{
Events =
[
new AlarmHistorianEventDto
{
EventId = "ev1",
SourceName = "Tank/HiHi",
ConditionId = "HiHi",
AlarmType = "LimitAlarm:Activated",
Message = "msg",
Severity = 700,
EventTimeUtcTicks = 999L,
AckComment = null,
},
],
CorrelationId = "r",
};
var bytes = MessagePackSerializer.Serialize(original);
var rt = MessagePackSerializer.Deserialize<WriteAlarmEventsRequest>(bytes);
rt.CorrelationId.ShouldBe("r");
rt.Events.Length.ShouldBe(1);
rt.Events[0].EventId.ShouldBe("ev1");
rt.Events[0].SourceName.ShouldBe("Tank/HiHi");
rt.Events[0].Severity.ShouldBe((ushort)700);
rt.Events[0].EventTimeUtcTicks.ShouldBe(999L);
}
[Fact]
public void WriteAlarmEventsReply_RoundTrips()
{
var original = new WriteAlarmEventsReply
{
CorrelationId = "r",
Success = true,
Error = null,
PerEventOk = [true, false, true],
};
var bytes = MessagePackSerializer.Serialize(original);
var rt = MessagePackSerializer.Deserialize<WriteAlarmEventsReply>(bytes);
rt.CorrelationId.ShouldBe("r");
rt.Success.ShouldBeTrue();
rt.PerEventOk.ShouldBe(new[] { true, false, true });
}
// ---- MessageKind enum values are pinned ----
// Changing a MessageKind value is a wire break; pin them explicitly.
[Fact]
public void MessageKind_Values_AreStable()
{
((byte)MessageKind.Hello).ShouldBe((byte)0x01);
((byte)MessageKind.HelloAck).ShouldBe((byte)0x02);
((byte)MessageKind.ReadRawRequest).ShouldBe((byte)0x10);
((byte)MessageKind.ReadRawReply).ShouldBe((byte)0x11);
((byte)MessageKind.ReadProcessedRequest).ShouldBe((byte)0x12);
((byte)MessageKind.ReadProcessedReply).ShouldBe((byte)0x13);
((byte)MessageKind.ReadAtTimeRequest).ShouldBe((byte)0x14);
((byte)MessageKind.ReadAtTimeReply).ShouldBe((byte)0x15);
((byte)MessageKind.ReadEventsRequest).ShouldBe((byte)0x16);
((byte)MessageKind.ReadEventsReply).ShouldBe((byte)0x17);
((byte)MessageKind.WriteAlarmEventsRequest).ShouldBe((byte)0x20);
((byte)MessageKind.WriteAlarmEventsReply).ShouldBe((byte)0x21);
}
// ---- Framing constants are pinned ----
[Fact]
public void Framing_Constants_AreStable()
{
Framing.LengthPrefixSize.ShouldBe(4);
Framing.KindByteSize.ShouldBe(1);
Framing.MaxFrameBodyBytes.ShouldBe(16 * 1024 * 1024);
}
}

46
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/FakeSidecarServer.cs
View File

@@ -26,6 +26,26 @@ internal sealed class FakeSidecarServer : IAsyncDisposable
/// <summary>Force-disconnect the next accepted client mid-call to exercise reconnect.</summary>
public bool DisconnectAfterHandshake { get; set; }
/// <summary>
/// Drop the connection after the handshake but before replying to any non-Hello request.
/// Armed for every connection until reset. Used to exercise the WriteBatchAsync catch
/// path and the second-attempt-also-fails propagation path.
/// </summary>
public bool DisconnectBeforeReply { get; set; }
/// <summary>
/// Reply to the first non-Hello request with this kind instead of the expected kind,
/// to exercise <see cref="System.IO.InvalidDataException"/> detection in ExchangeAsync.
/// Reset to null after the first mis-routed reply.
/// </summary>
public MessageKind? ReplyWithWrongKind { get; set; }
/// <summary>
/// Stall indefinitely after receiving a request before sending any reply, so the client's
/// call-timeout token fires. Used to test the CallTimeout path.
/// </summary>
public bool StallAfterRequest { get; set; }
public FakeSidecarServer(string pipeName, string expectedSecret)
{
_pipeName = pipeName;
@@ -83,6 +103,32 @@ internal sealed class FakeSidecarServer : IAsyncDisposable
var frame = await reader.ReadFrameAsync(ct).ConfigureAwait(false);
if (frame is null) break;
// Drop before sending any reply — lets the client fall into its catch /
// retry path or propagate on second failure.
if (DisconnectBeforeReply)
{
pipe.Disconnect();
break;
}
// Stall indefinitely to let the client's call-timeout token fire.
if (StallAfterRequest)
{
await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false);
break;
}
// Optionally send a deliberately wrong kind back to exercise
// InvalidDataException detection in the client's ExchangeAsync.
if (ReplyWithWrongKind.HasValue)
{
var wrongKind = ReplyWithWrongKind.Value;
ReplyWithWrongKind = null; // arm once
// Send an empty body with the wrong kind so the client can parse it.
await writer.WriteAsync(wrongKind, new ReadRawReply { Success = false }, ct);
continue;
}
switch (frame.Value.Kind)
{
case MessageKind.ReadRawRequest:

128
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs
View File

@@ -363,4 +363,132 @@ public sealed class WonderwareHistorianClientTests
snap.LastError.ShouldNotBeNull();
snap.ProcessConnectionOpen.ShouldBeTrue();
}
// ===== Finding-009: missing edge-case tests =====
/// <summary>
/// (2) A transport drop during a write (the catch path in WriteBatchAsync) must return
/// RetryPlease for every event in the batch — never throw, never PermanentFail.
/// </summary>
[Fact]
public async Task WriteBatchAsync_TransportDropDuringWrite_ReturnsRetryPleaseForEveryEvent()
{
var pipe = UniquePipeName();
// Server disconnects before replying to the write request. The client's single retry
// reconnects; on the second attempt the server is still armed to disconnect, so both
// attempts fail and the catch block fires.
await using var server = new FakeSidecarServer(pipe, Secret)
{
DisconnectBeforeReply = true,
};
await server.StartAsync();
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
var batch = new[]
{
new AlarmHistorianEvent("ev-1", "Tank/HiHi", "HiHi", "LimitAlarm", AlarmSeverity.High, "Activated", "msg", "u", null, DateTime.UtcNow),
new AlarmHistorianEvent("ev-2", "Tank/HiHi", "HiHi", "LimitAlarm", AlarmSeverity.High, "Cleared", "msg", "u", null, DateTime.UtcNow),
};
// WriteBatchAsync must not throw — it absorbs transport failures as RetryPlease.
var outcomes = await client.WriteBatchAsync(batch, CancellationToken.None);
outcomes.Count.ShouldBe(2);
outcomes[0].ShouldBe(HistorianWriteOutcome.RetryPlease);
outcomes[1].ShouldBe(HistorianWriteOutcome.RetryPlease);
}
/// <summary>
/// (3) When both the first attempt and the single retry fail (the "second attempt also
/// fails" path in InvokeAsync), the exception propagates to the caller.
/// </summary>
[Fact]
public async Task InvokeAsync_BothAttemptsFailTransport_PropagatesException()
{
var pipe = UniquePipeName();
// DisconnectBeforeReply stays true so both the first attempt and the single retry
// inside InvokeAsync are dropped, causing the second ExchangeAsync to throw.
await using var server = new FakeSidecarServer(pipe, Secret)
{
DisconnectBeforeReply = true,
};
await server.StartAsync();
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
// ReadRawAsync uses Invoke, which propagates the exception when both attempts fail.
await Should.ThrowAsync<Exception>(() =>
client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None));
}
/// <summary>
/// (4) A stalled sidecar that never sends a reply must cause an
/// <see cref="OperationCanceledException"/> within the configured CallTimeout.
/// </summary>
[Fact]
public async Task ReadRawAsync_StalledSidecar_TimesOutWithOperationCanceledException()
{
var pipe = UniquePipeName();
await using var server = new FakeSidecarServer(pipe, Secret)
{
StallAfterRequest = true,
};
await server.StartAsync();
var opts = new WonderwareHistorianClientOptions(
PipeName: pipe,
SharedSecret: Secret,
PeerName: "test",
ConnectTimeout: TimeSpan.FromSeconds(2),
CallTimeout: TimeSpan.FromMilliseconds(500)); // short timeout for test speed
await using var client = new WonderwareHistorianClient(opts);
// The stall means neither the first nor the retry can complete, so the timeout
// linked-token should cancel the operation.
await Should.ThrowAsync<OperationCanceledException>(() =>
client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None));
}
/// <summary>
/// (5) <see cref="HistoryAggregateType.Total"/> must throw
/// <see cref="NotSupportedException"/> because Wonderware AnalogSummary has no Total
/// aggregate column.
/// </summary>
[Fact]
public async Task ReadProcessedAsync_TotalAggregate_ThrowsNotSupported()
{
var pipe = UniquePipeName();
await using var server = new FakeSidecarServer(pipe, Secret);
await server.StartAsync();
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
await Should.ThrowAsync<NotSupportedException>(() =>
client.ReadProcessedAsync("Tag",
DateTime.UtcNow, DateTime.UtcNow, TimeSpan.FromMinutes(1),
HistoryAggregateType.Total, CancellationToken.None));
}
/// <summary>
/// (6) When the sidecar replies with a <see cref="MessageKind"/> the client does not
/// expect (e.g. ReadRawReply where ReadAtTimeReply was expected), the client must throw
/// <see cref="InvalidDataException"/>.
/// </summary>
[Fact]
public async Task ReadRawAsync_SidecarRepliesWithWrongKind_ThrowsInvalidDataException()
{
var pipe = UniquePipeName();
await using var server = new FakeSidecarServer(pipe, Secret)
{
// Force the server to reply with ReadAtTimeReply instead of ReadRawReply.
ReplyWithWrongKind = MessageKind.ReadAtTimeReply,
};
await server.StartAsync();
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
await Should.ThrowAsync<InvalidDataException>(() =>
client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None));
}
}