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

fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-014)

Browse Source 
Add regression tests for the Medium findings resolved in this series:
- AbCipDataType_maps_large_integer_types (theory: LInt→Int64, ULInt→UInt64,
  UDInt→UInt32) and Read_UDInt_tag_returns_uint_value_not_negative_wrapped_int
  cover Driver.AbCip-004.
- Structure_parent_tag_is_not_readable_after_member_fan_out,
  InitializeAsync_throws_on_duplicate_tag_name, and
  InitializeAsync_throws_when_member_name_collides_with_independent_tag
  cover Driver.AbCip-005.
- Read_failure_evicts_runtime_so_next_read_creates_fresh_handle covers
  Driver.AbCip-010.
AbCipDriverTests.AbCipDataType_maps_atomics_to_driver_types extended with
LInt/ULInt/UDInt assertions.

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

4
code-reviews/Driver.AbCip/findings.md
View File

@@ -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

147
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs
View File

@@ -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;
/// <summary>
/// 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 <see cref="AbCipStatusMapperTests"/>.)
/// </summary>
[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<InvalidOperationException>(() =>
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<InvalidOperationException>(() =>
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);
}
}

4
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverTests.cs
View File

@@ -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);
}
}