diff --git a/code-reviews/Driver.Historian.Wonderware.Client/findings.md b/code-reviews/Driver.Historian.Wonderware.Client/findings.md index fbdc2e3..7b2aff1 100644 --- a/code-reviews/Driver.Historian.Wonderware.Client/findings.md +++ b/code-reviews/Driver.Historian.Wonderware.Client/findings.md @@ -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 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/ContractsWireParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/ContractsWireParityTests.cs new file mode 100644 index 0000000..f073c86 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/ContractsWireParityTests.cs @@ -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; + +/// +/// 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 [Key] 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.) +/// +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(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(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(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(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(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(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(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(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); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/FakeSidecarServer.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/FakeSidecarServer.cs index 416d548..8d31706 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/FakeSidecarServer.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/FakeSidecarServer.cs @@ -26,6 +26,26 @@ internal sealed class FakeSidecarServer : IAsyncDisposable /// Force-disconnect the next accepted client mid-call to exercise reconnect. public bool DisconnectAfterHandshake { get; set; } + /// + /// 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. + /// + public bool DisconnectBeforeReply { get; set; } + + /// + /// Reply to the first non-Hello request with this kind instead of the expected kind, + /// to exercise detection in ExchangeAsync. + /// Reset to null after the first mis-routed reply. + /// + public MessageKind? ReplyWithWrongKind { get; set; } + + /// + /// Stall indefinitely after receiving a request before sending any reply, so the client's + /// call-timeout token fires. Used to test the CallTimeout path. + /// + 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: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs index 381d631..7e75a3f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs @@ -363,4 +363,132 @@ public sealed class WonderwareHistorianClientTests snap.LastError.ShouldNotBeNull(); snap.ProcessConnectionOpen.ShouldBeTrue(); } + + // ===== Finding-009: missing edge-case tests ===== + + /// + /// (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. + /// + [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); + } + + /// + /// (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. + /// + [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(() => + client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None)); + } + + /// + /// (4) A stalled sidecar that never sends a reply must cause an + /// within the configured CallTimeout. + /// + [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(() => + client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None)); + } + + /// + /// (5) must throw + /// because Wonderware AnalogSummary has no Total + /// aggregate column. + /// + [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(() => + client.ReadProcessedAsync("Tag", + DateTime.UtcNow, DateTime.UtcNow, TimeSpan.FromMinutes(1), + HistoryAggregateType.Total, CancellationToken.None)); + } + + /// + /// (6) When the sidecar replies with a the client does not + /// expect (e.g. ReadRawReply where ReadAtTimeReply was expected), the client must throw + /// . + /// + [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(() => + client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None)); + } }