diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index 2263f11..1f519f3 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -228,13 +228,13 @@ | Severity | Medium | | Category | Testing coverage | | Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipStatusMapperTests.cs:28-40` | -| Status | Open | +| Status | Resolved | **Description:** `AbCipStatusMapperTests.MapLibplctagStatus_maps_known_codes` asserts the mapper against the same wrong integer constants (-5, -7, -14, -16, -17) the production code uses (see Driver.AbCip-002). The test locks in the bug rather than catching it, giving false confidence that libplctag error mapping is correct. There is no test that drives an actual libplctag `Status` enum value through `LibplctagTagRuntime.GetStatus()` plus `MapLibplctagStatus` end-to-end. Separately, the broken `ReinitializeAsync` config-discard behavior (Driver.AbCip-001) and the declaration-order whole-UDT decode risk (Driver.AbCip-003) have no test that would fail when those defects are present: `AbCipDriverWholeUdtReadTests` only exercises a UDT whose declaration order happens to match a simple alignment layout. **Recommendation:** Rewrite the libplctag-status test to use the real `libplctag.Status` enum members and their documented integer values. Add a test that `ReinitializeAsync` with a changed config JSON actually applies the change (or asserts the documented immutability contract). Add a whole-UDT decode test where the controller compiled layout differs from declaration order. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — status mapper test already uses real `Status` enum members (fixed with Driver.AbCip-002); `ReinitializeAsync` config-change coverage already added with Driver.AbCip-001. Added to `AbCipDriverCodeReviewRegressionTests`: three tests for 004 (LInt/ULInt/UDInt type mapping theory + UDInt uint-vs-negative-int read test), three tests for 005 (Structure parent is BadNodeIdUnknown, duplicate scalar key throws, member-collision-with-independent-tag throws), and one test for 010 (eviction on bad status means next read creates a fresh handle). `AbCipDriverTests.AbCipDataType_maps_atomics_to_driver_types` extended with LInt/ULInt/UDInt assertions. ### Driver.AbCip-015 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs index f2fe822..1e1d1c5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs @@ -1,3 +1,4 @@ +using libplctag; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; @@ -6,7 +7,8 @@ using ZB.MOM.WW.OtOpcUa.Driver.AbCip; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; /// -/// Regression tests for the High code-review findings Driver.AbCip-001 / -003 / -008. +/// Regression tests for the High code-review findings Driver.AbCip-001 / -003 / -008, +/// and the Medium findings Driver.AbCip-004 / -005 / -010. /// (Driver.AbCip-002 is covered by .) /// [Trait("Category", "Unit")] @@ -188,4 +190,147 @@ public sealed class AbCipDriverCodeReviewRegressionTests foreach (var result in allResults) result.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); } + + // ---- Driver.AbCip-004 — LInt/ULInt/UDInt declared type must agree with runtime value type ---- + + [Theory] + [InlineData(AbCipDataType.LInt, DriverDataType.Int64)] + [InlineData(AbCipDataType.ULInt, DriverDataType.UInt64)] + [InlineData(AbCipDataType.UDInt, DriverDataType.UInt32)] + public void AbCipDataType_maps_large_integer_types_to_correct_driver_types( + AbCipDataType abType, DriverDataType expected) + { + // Regression for Driver.AbCip-004: LInt/ULInt were mapped to Int32 (truncation); + // UDInt was mapped to Int32 (negative wrap for values > Int32.MaxValue). + abType.ToDriverDataType().ShouldBe(expected); + } + + [Fact] + public async Task Read_UDInt_tag_returns_uint_value_not_negative_wrapped_int() + { + // A UDInt value above Int32.MaxValue (e.g. uint.MaxValue) used to be decoded as + // (int)GetUInt32, which wraps to -1. After the fix it must be decoded as uint. + const uint largeUDInt = uint.MaxValue; // would wrap to -1 as (int) + var factory = new FakeAbCipTagFactory(); + factory.Customise = p => new FakeAbCipTag(p) { Value = largeUDInt }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + Tags = [new AbCipTagDefinition("Counter", Device, "Counter", AbCipDataType.UDInt)], + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.ReadAsync(["Counter"], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + // The value must be the uint, not a negative int + results.Single().Value.ShouldBe(largeUDInt); + results.Single().Value.ShouldNotBe(-1); + } + + // ---- Driver.AbCip-005 — Structure parent not registered; duplicate key check ---- + + [Fact] + public async Task Structure_parent_tag_is_not_readable_after_member_fan_out() + { + // Regression for Driver.AbCip-005: the bare parent "Motor" used to be added to + // _tagsByName before member fan-out, so ReadAsync("Motor") returned Good/null. + // After the fix, reading the parent name returns BadNodeIdUnknown. + var factory = new FakeAbCipTagFactory(); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + Tags = + [ + new AbCipTagDefinition("Motor", Device, "Motor", AbCipDataType.Structure, Members: + [ + new AbCipStructureMember("Speed", AbCipDataType.DInt), + new AbCipStructureMember("Torque", AbCipDataType.Real), + ]), + ], + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.ReadAsync(["Motor"], CancellationToken.None); + + // Parent is not a readable tag — BadNodeIdUnknown, not Good/null. + results.Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + } + + [Fact] + public void InitializeAsync_throws_on_duplicate_tag_name() + { + // Regression for Driver.AbCip-005: silently-overwritten duplicate keys. + // Two independently-declared tags with the same name must throw. + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + Tags = + [ + new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt), + new AbCipTagDefinition("Speed", Device, "SpeedAlias", AbCipDataType.Real), // same name + ], + }, "drv-1"); + + Should.Throw(() => + drv.InitializeAsync("{}", CancellationToken.None).GetAwaiter().GetResult()); + } + + [Fact] + public void InitializeAsync_throws_when_member_name_collides_with_independent_tag() + { + // A Structure fan-out member path ("Motor.Speed") that collides with a separately- + // declared tag ("Motor.Speed") must throw rather than silently overwrite. + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + Tags = + [ + new AbCipTagDefinition("Motor", Device, "Motor", AbCipDataType.Structure, Members: + [ + new AbCipStructureMember("Speed", AbCipDataType.DInt), + ]), + new AbCipTagDefinition("Motor.Speed", Device, "Motor.Speed", AbCipDataType.DInt), // collision + ], + }, "drv-1"); + + Should.Throw(() => + drv.InitializeAsync("{}", CancellationToken.None).GetAwaiter().GetResult()); + } + + // ---- Driver.AbCip-010 — stale runtime evicted on failure ---- + + [Fact] + public async Task Read_failure_evicts_runtime_so_next_read_creates_fresh_handle() + { + // Regression for Driver.AbCip-010: a non-zero status was returned forever because + // the cached runtime was never evicted. After the fix the next read creates a new one. + var factory = new FakeAbCipTagFactory(); + var callCount = 0; + factory.Customise = p => + { + // First tag creation → returns error; second → returns success. + callCount++; + return callCount == 1 + ? new FakeAbCipTag(p) { Status = (int)libplctag.Status.ErrorBadConnection } + : new FakeAbCipTag(p) { Status = 0, Value = 42 }; + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)], + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + // First read — bad connection, runtime is evicted. + var first = await drv.ReadAsync(["Speed"], CancellationToken.None); + first.Single().StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); + + // Second read — fresh handle, succeeds. + var second = await drv.ReadAsync(["Speed"], CancellationToken.None); + second.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + second.Single().Value.ShouldBe(42); + // The factory was called twice — once for the failed handle, once for the fresh one. + callCount.ShouldBe(2); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverTests.cs index 97393b0..7ebfcc9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverTests.cs @@ -111,5 +111,9 @@ public sealed class AbCipDriverTests AbCipDataType.Real.ToDriverDataType().ShouldBe(DriverDataType.Float32); AbCipDataType.LReal.ToDriverDataType().ShouldBe(DriverDataType.Float64); AbCipDataType.String.ToDriverDataType().ShouldBe(DriverDataType.String); + // Driver.AbCip-004: 64-bit and unsigned-32-bit types must map to their correct equivalents. + AbCipDataType.LInt.ToDriverDataType().ShouldBe(DriverDataType.Int64); + AbCipDataType.ULInt.ToDriverDataType().ShouldBe(DriverDataType.UInt64); + AbCipDataType.UDInt.ToDriverDataType().ShouldBe(DriverDataType.UInt32); } }