a6ae4e22d1
Driver.Cli.Common-007 + Driver.Cli.Common-008 resolution.
Driver.Cli.Common-007 (High, Correctness):
0x80550000 is the canonical OPC UA spec value for BadSecurityPolicyRejected,
not BadDeviceFailure. The correct spec value for BadDeviceFailure is
0x808B0000 (verified against OPC Foundation Opc.Ua.StatusCodes;
corroborated locally by Driver.Galaxy.Runtime.StatusCodeMap and both
Wonderware historian quality mappers which all hand-pin the correct
value).
The bug was duplicated across six driver modules:
- FocasStatusMapper.BadDeviceFailure
- AbCipStatusMapper.BadDeviceFailure
- AbLegacyStatusMapper.BadDeviceFailure
- TwinCATStatusMapper.BadDeviceFailure
- ModbusDriver.StatusBadDeviceFailure
- S7Driver.StatusBadDeviceFailure
Plus the SnapshotFormatter shortlist that named 0x80550000 as
BadDeviceFailure, and three downstream Modbus tests that asserted
against the wrong value (so CI was blind).
This commit fixes all six native-mapper constants, the formatter
shortlist, and the three Modbus tests in one pass. Added a regression
guard to FormatStatus_does_not_apply_pre_fix_wrong_names that pins
0x80550000 never renders as BadDeviceFailure (mirroring the existing
-001 wrong-name guards).
Behavior change: OPC UA clients consuming the native drivers now see
the canonical BadDeviceFailure (0x808B0000) on device-fault paths
instead of the misnamed BadSecurityPolicyRejected (0x80550000). Wire-
level status semantics now match operator-facing CLI labels.
Driver.Cli.Common-008 (Low, Testing):
Deleted the redundant FormatStatus_names_native_driver_emitted_codes
Theory — its five InlineData rows were already covered by the
well-known Theory in the same commit (5a9c459), and used a weaker
ShouldContain vs the well-known Theory's ShouldBe (exact match).
Verification:
- Driver.Cli.Common.Tests: 43/43 pass (was 48 after the -008 deletion).
- Driver.Modbus.Tests: 263/263 pass.
- Driver.AbCip.Tests: 262/262.
- Driver.AbLegacy.Tests: 157/157.
- Driver.FOCAS.Tests: 178/178.
- Driver.S7.Tests: 112/112.
- Driver.TwinCAT.Tests: 131/131.
Total: 1146 tests across the affected modules, all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
365 lines
20 KiB
Markdown
365 lines
20 KiB
Markdown
# Code Review — Driver.Cli.Common
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common` |
|
|
| Reviewer | Claude Code |
|
|
| Review date | 2026-05-23 |
|
|
| Commit reviewed | `a9be809` |
|
|
| Status | Reviewed |
|
|
| Open findings | 0 |
|
|
|
|
## Checklist coverage
|
|
|
|
A comprehensive review completes every category, recording "No issues found" where
|
|
a category produced nothing rather than leaving it blank.
|
|
|
|
| # | Category | Result |
|
|
|---|---|---|
|
|
| 1 | Correctness & logic bugs | Driver.Cli.Common-001, Driver.Cli.Common-002, Driver.Cli.Common-007 |
|
|
| 2 | OtOpcUa conventions | No issues found |
|
|
| 3 | Concurrency & thread safety | Driver.Cli.Common-003 |
|
|
| 4 | Error handling & resilience | Driver.Cli.Common-004 |
|
|
| 5 | Security | No issues found |
|
|
| 6 | Performance & resource management | No issues found |
|
|
| 7 | Design-document adherence | No issues found |
|
|
| 8 | Code organization & conventions | No issues found |
|
|
| 9 | Testing coverage | Driver.Cli.Common-005, Driver.Cli.Common-008 |
|
|
| 10 | Documentation & comments | Driver.Cli.Common-006 |
|
|
|
|
## Re-review 2026-05-23 (commit `a9be809`)
|
|
|
|
Delta scope: commit `5a9c459` extends the `FormatStatus` shortlist with five
|
|
`Bad*` codes (`BadInternalError` 0x80020000, `BadNotWritable` 0x803B0000,
|
|
`BadOutOfRange` 0x803C0000, `BadNotSupported` 0x803D0000, `BadDeviceFailure`
|
|
0x80550000) the FOCAS / AbCip / AbLegacy native-protocol mappers emit. Tests
|
|
extended with parallel `[InlineData]` rows on the well-known Theory plus a new
|
|
`FormatStatus_names_native_driver_emitted_codes` Theory.
|
|
|
|
Cross-checked the five new hex literals against the OPC Foundation
|
|
`Opc.Ua.StatusCodes` table via DeepWiki:
|
|
|
|
| Name added | Code in shortlist | Spec value | Verdict |
|
|
|---|---|---|---|
|
|
| `BadInternalError` | `0x80020000` | `0x80020000` | Correct |
|
|
| `BadNotWritable` | `0x803B0000` | `0x803B0000` | Correct |
|
|
| `BadOutOfRange` | `0x803C0000` | `0x803C0000` | Correct |
|
|
| `BadNotSupported` | `0x803D0000` | `0x803D0000` | Correct |
|
|
| `BadDeviceFailure` | `0x80550000` | **`0x808B0000`** | **WRONG — `0x80550000` is `BadSecurityPolicyRejected`** |
|
|
|
|
The `BadDeviceFailure` mismapping is the same shape of bug as the original
|
|
Driver.Cli.Common-001 (wrong hex literal copied into the shortlist); recorded
|
|
as Driver.Cli.Common-007. The wrong constant also lives in
|
|
`FocasStatusMapper.cs`, `AbCipStatusMapper.cs`, `AbLegacyStatusMapper.cs`,
|
|
`TwinCATStatusMapper.cs`, `S7Driver.cs`, and `ModbusDriver.cs` — those are in
|
|
other modules' review scope but are noted here so future re-reviewers know
|
|
this isn't isolated. (`StatusCodeMap.cs` in Driver.Galaxy + the Wonderware
|
|
historian mappers use the correct `0x808B0000`, confirming the discrepancy.)
|
|
|
|
Testing observation: the new `FormatStatus_names_native_driver_emitted_codes`
|
|
Theory is fully redundant with the well-known Theory (the five rows were also
|
|
added there in the same commit) and uses `ShouldContain` rather than
|
|
`ShouldBe` — recorded as Driver.Cli.Common-008.
|
|
|
|
Other categories (concurrency, security, performance, design-doc adherence,
|
|
code organisation, documentation) are unchanged by this delta — no new
|
|
issues found.
|
|
|
|
## Findings
|
|
|
|
### Driver.Cli.Common-001
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | High |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:106-119` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** The `FormatStatus` shortlist maps four OPC UA status names to incorrect
|
|
numeric codes. The correct OPC UA spec values (verified against the OPC Foundation
|
|
UA-.NETStandard `Opc.Ua.StatusCodes` table) are:
|
|
|
|
| Name in shortlist | Code used | Correct code | What the used code actually is |
|
|
|---|---|---|---|
|
|
| `BadTimeout` | `0x80060000` | `0x800A0000` | `0x80060000` = `BadOutOfMemory` |
|
|
| `BadNoCommunication` | `0x80070000` | `0x80310000` | `0x80070000` = `BadResourceUnavailable` |
|
|
| `BadWaitingForInitialData` | `0x80080000` | `0x80320000` | `0x80080000` is not this name |
|
|
| `BadNodeIdInvalid` | `0x80350000` | `0x80330000` | `0x80350000` = `BadNodeClassInvalid` |
|
|
|
|
`Good` (`0x00000000`), `Bad` (`0x80000000`), `BadCommunicationError` (`0x80050000`),
|
|
`BadNodeIdUnknown` (`0x80340000`), `BadTypeMismatch` (`0x80740000`), and `Uncertain`
|
|
(`0x40000000`) are correct.
|
|
|
|
This is operator-facing and load-bearing: the CLI whole purpose is to label driver
|
|
status codes so a human can interpret a probe/read/write. A real device timeout
|
|
(`0x800A0000`) renders as bare `0x800A0000` with no name, while an out-of-memory
|
|
status (`0x80060000`) is mislabeled `BadTimeout`. A driver returning
|
|
`BadNodeClassInvalid` (`0x80350000`) is mislabeled `BadNodeIdInvalid`. The
|
|
`SnapshotFormatterTests` `[Theory]` cases for these codes assert against the wrong
|
|
expectations and therefore pass while the mapping is wrong (see Driver.Cli.Common-005).
|
|
|
|
**Recommendation:** Correct the four mappings to the spec values. Prefer deriving names
|
|
from the OPC Foundation `Opc.Ua.StatusCodes` constants (the stack the project already
|
|
depends on transitively) rather than hand-maintaining a hex shortlist, so the table
|
|
cannot drift from the spec again. If a hand-list is kept, add a test that cross-checks
|
|
each entry against `Opc.Ua.StatusCodes` reflection.
|
|
|
|
**Resolution:** Resolved 2026-05-22 — corrected the four mismapped `FormatStatus` codes
|
|
to their canonical `Opc.Ua.StatusCodes` values (`BadTimeout` 0x800A0000, `BadNoCommunication`
|
|
0x80310000, `BadWaitingForInitialData` 0x80320000, `BadNodeIdInvalid` 0x80330000); the CLI
|
|
project does not reference the `Opc.Ua` package so the hex literals were corrected in place
|
|
with a sync note, and `SnapshotFormatterTests` was updated with corrected expectations plus
|
|
a regression `[Theory]` asserting the pre-fix wrong names no longer apply.
|
|
|
|
### Driver.Cli.Common-002
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Medium |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:101-122` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** `FormatStatus` matches the full 32-bit status word for exact equality
|
|
against the shortlist. OPC UA status codes carry sub-code/flag bits in the low 16 bits
|
|
(info type, structure-changed, semantics-changed, limit bits, overflow, etc.). A
|
|
driver-supplied status such as `0x80050001` or any `Good` value with info bits set
|
|
(e.g. an overflow bit) falls through the `switch` and renders as bare hex even though
|
|
the high bits clearly identify the severity class. The doc comment on `FormatStatus`
|
|
claims the well-known statuses are named, but only the bit-exact canonical forms are.
|
|
|
|
**Recommendation:** Either (a) narrow the doc-comment claim to bit-exact canonical
|
|
codes, or (b) match on the severity bits (`code & 0xC0000000`) to at least always emit
|
|
`Good` / `Uncertain` / `Bad` even when sub-code bits are set, and match the named codes
|
|
on the masked code (`code & 0xFFFF0000`).
|
|
|
|
**Resolution:** Resolved 2026-05-22 — `FormatStatus` now matches named codes on `code & 0xFFFF0000` and falls back to a severity-class label (`Good`/`Uncertain`/`Bad`) via `code & 0xC0000000` for unknown sub-codes; the stale "bare-hex for unknown codes" test expectation was corrected to reflect the new severity-class fallback.
|
|
|
|
### Driver.Cli.Common-003
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Medium |
|
|
| Category | Concurrency & thread safety |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** `ConfigureLogging` assigns the process-global `Serilog.Log.Logger`
|
|
without disposing the previously assigned logger and the library never calls
|
|
`Log.CloseAndFlush()`. Each call creates a fresh `Logger` via `CreateLogger()` and
|
|
overwrites `Log.Logger`; the prior instance (and its console sink) is never disposed
|
|
or flushed. The class is the shared base for every driver CLI and the `subscribe` verb
|
|
is long-running — if any command path re-invokes `ConfigureLogging` the buffered
|
|
console sink is abandoned without a flush, and on process exit the final logger is also
|
|
never flushed. Verbose debug output written just before exit can be lost.
|
|
|
|
**Recommendation:** Call `Log.CloseAndFlush()` on shutdown (e.g. in a `finally` in the
|
|
command `ExecuteAsync`, or via a `protected` disposal helper on this base). Treat
|
|
`ConfigureLogging` as call-once / idempotent and document that. At minimum capture and
|
|
dispose the previous logger if reconfiguration is genuinely intended.
|
|
|
|
**Resolution:** Resolved 2026-05-22 — `ConfigureLogging` is now idempotent (guarded by `_loggingConfigured` field) and disposes the previous `Log.Logger` before overwriting; a new `protected static FlushLogging()` helper calls `Log.CloseAndFlush()` for commands to call in their `finally` blocks; XML doc updated accordingly.
|
|
|
|
### Driver.Cli.Common-004
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Error handling & resilience |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the
|
|
value and status columns) without guarding against empty input. When `tagNames` and
|
|
`snapshots` are both empty (equal length, so the mismatch check at line 56 passes),
|
|
`Enumerable.Max` throws `InvalidOperationException` ("Sequence contains no elements").
|
|
A batch read that legitimately returns zero tags therefore crashes the formatter
|
|
instead of producing an empty (header-only) table.
|
|
|
|
**Recommendation:** Short-circuit on `rows.Length == 0` (return just the header +
|
|
separator, or an explicit "no rows" line), or use `DefaultIfEmpty(0).Max(...)` for the
|
|
width computations.
|
|
|
|
**Resolution:** Resolved 2026-05-23 — `FormatTable` guards each `rows.Max(...)` width
|
|
computation with a `rows.Length == 0 ? "<HEADER>".Length : Math.Max(...)` ternary, so
|
|
an empty batch read returns the header + separator rows (no data rows) instead of
|
|
throwing `InvalidOperationException`. The fix was landed in commit `1433a1c` alongside
|
|
the -002 work, and the regression test
|
|
`SnapshotFormatterTests.FormatTable_with_empty_input_returns_header_only` (added under
|
|
-005) exercises it.
|
|
|
|
### Driver.Cli.Common-005
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Medium |
|
|
| Category | Testing coverage |
|
|
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:27-37` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** The `FormatStatus_names_well_known_status_codes` `[Theory]` asserts
|
|
`0x80060000 => "BadTimeout"`, which encodes the wrong spec value (see
|
|
Driver.Cli.Common-001). The test passes because it validates the formatter against the
|
|
same incorrect table, so the bug is invisible to CI. Additionally there is no coverage
|
|
for: `DriverCommandBase` (`ConfigureLogging` verbose vs non-verbose level selection — no
|
|
test exercises the base at all), `FormatTable` with empty input (Driver.Cli.Common-004
|
|
would have been caught), `FormatValue` with array / enum / custom `object` values, and
|
|
`FormatTimestamp` with `DateTimeKind.Unspecified` (the docs imply Unspecified is
|
|
normalised but only `Local` is tested).
|
|
|
|
**Recommendation:** Fix the `[Theory]` expectations once Driver.Cli.Common-001 is
|
|
resolved, and add a test asserting each shortlist entry against the OPC Foundation
|
|
`Opc.Ua.StatusCodes` constants so the table cannot silently drift. Add `FormatTable`
|
|
empty-input and `DriverCommandBase` level-selection tests.
|
|
|
|
**Resolution:** Resolved 2026-05-22 — added `FormatTable_with_empty_input_returns_header_only` (exercises the -004 fix), `FormatStatus_with_sub_code_bits_resolves_to_named_class` / `FormatStatus_unknown_sub_code_falls_back_to_severity_class` Theories (cover -002 fix), and a new `DriverCommandBaseTests` class with four tests covering verbose/non-verbose level selection, idempotency of `ConfigureLogging`, and `FlushLogging`; stale `FormatStatus_unknown_codes_fall_back_to_hex_only` expectation corrected to match the -002 severity-class fallback.
|
|
|
|
### Driver.Cli.Common-006
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Documentation & comments |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71`
|
|
states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement
|
|
needed" — true only when every snapshot has a non-null `SourceTimestampUtc`.
|
|
`FormatTimestamp` returns `"-"` for a null timestamp, so a mixed table has a 1-char-wide
|
|
cell in an otherwise 24-char column; the column is unaligned. Harmless (right-most, no
|
|
padding consumer) but the stated invariant does not hold. (2) The `DriverCommandBase`
|
|
class summary enumerates "Modbus / AB CIP / AB Legacy / S7 / TwinCAT" as the driver CLIs
|
|
but omits FOCAS, which `docs/DriverClis.md` lists as the sixth CLI built on this shared
|
|
library. The XML doc is stale relative to the shipped driver-CLI set.
|
|
|
|
**Recommendation:** Reword the `SnapshotFormatter.cs:71` comment to note the column is
|
|
right-most and intentionally unpadded rather than claiming fixed width. Add FOCAS to the
|
|
`DriverCommandBase` class-summary driver list.
|
|
|
|
**Resolution:** Resolved 2026-05-23 — (1) `SnapshotFormatter.cs:71` comment reworded
|
|
to state the source-time column is the right-most one and intentionally not
|
|
measured/padded, calling out the null-timestamp `"-"` case explicitly. (2) FOCAS was
|
|
added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff356b`
|
|
(landed alongside the -003 work).
|
|
|
|
### Driver.Cli.Common-007
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | High |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:129` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** Commit `5a9c459` added `0x80550000u => "BadDeviceFailure"` to the
|
|
`FormatStatus` shortlist, but `0x80550000` is the canonical OPC UA spec value for
|
|
`BadSecurityPolicyRejected`, not `BadDeviceFailure`. The correct spec value for
|
|
`BadDeviceFailure` is `0x808B0000` (verified against the OPC Foundation
|
|
`Opc.Ua.StatusCodes` table via DeepWiki; corroborated locally by
|
|
`src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs:40`
|
|
(`BadDeviceFailure = 0x808B0000u`) and the two Wonderware historian quality mappers,
|
|
which all hand-pin the correct value).
|
|
|
|
This is the same shape of bug Driver.Cli.Common-001 closed: a wrong hex literal
|
|
in the shortlist that the test theory (`SnapshotFormatterTests.cs:42`) blindly
|
|
asserts against the same wrong value, so the bug is invisible to CI.
|
|
|
|
Practical impact, two-sided:
|
|
1. A driver that returns the real spec `BadDeviceFailure` (`0x808B0000`) — e.g.
|
|
the `Driver.Galaxy.StatusCodeMap` path on a deploy-time device fault — falls
|
|
through the named shortlist entirely. Since Driver.Cli.Common-002 added the
|
|
severity-class fallback, it now renders as `0x808B0000 (Bad)` instead of
|
|
`0x808B0000 (BadDeviceFailure)` — operators lose the specific class label
|
|
`docs/Driver.FOCAS.Cli.md:153` tells them to read off the output.
|
|
2. A driver that returns `0x80550000` (which `FocasStatusMapper`, `AbCipStatusMapper`,
|
|
`AbLegacyStatusMapper`, `TwinCATStatusMapper`, `S7Driver`, and `ModbusDriver` all
|
|
misuse as "BadDeviceFailure") now renders as `0x80550000 (BadDeviceFailure)` —
|
|
matching driver intent but contradicting the OPC UA spec, which says any client
|
|
that decodes the same payload using the OPC Foundation stack will see
|
|
`BadSecurityPolicyRejected`. A security-monitoring tool keying on
|
|
`BadSecurityPolicyRejected` will fire on a CPU fault, while real
|
|
`BadSecurityPolicyRejected` returns from the secure-channel layer would be
|
|
mislabelled as a device fault. Operator-facing CLI output and machine-readable
|
|
status semantics disagree.
|
|
|
|
The deeper bug is the wrong constant in the native-protocol mappers (out of scope
|
|
for this module), but the `SnapshotFormatter` shortlist is its own
|
|
spec-authoritative reference point — Driver.Cli.Common-001 explicitly framed the
|
|
shortlist as canonical, with the in-line "keep [these literals] in sync with [the
|
|
Opc.Ua.StatusCodes] table" comment at `SnapshotFormatter.cs:112-113`. That
|
|
contract is now broken.
|
|
|
|
**Recommendation:** Change line 129 to `0x808B0000u => "BadDeviceFailure"`. Update
|
|
the matching `[InlineData]` rows in `SnapshotFormatterTests.cs` (line 42 in the
|
|
well-known Theory; line 60 in the redundant Theory — see Driver.Cli.Common-008).
|
|
Also note in the resolution that the native-protocol mappers (FOCAS / AbCip /
|
|
AbLegacy / TwinCAT / S7 / Modbus) need the same fix recorded against their own
|
|
module reviews — the constant `0x80550000` should be replaced with `0x808B0000`
|
|
everywhere it claims to mean `BadDeviceFailure`. Consider Driver.Cli.Common-001's
|
|
original recommendation again: add a CI test that cross-checks every shortlist
|
|
entry against `Opc.Ua.StatusCodes` reflection so this class of bug stops
|
|
recurring.
|
|
|
|
**Resolution:** Resolved 2026-05-23 — corrected `SnapshotFormatter.FormatStatus`
|
|
to map `0x808B0000u => "BadDeviceFailure"` (was `0x80550000u`). Updated the
|
|
`InlineData` row in the well-known Theory accordingly; the redundant native-
|
|
emitted Theory was deleted entirely per Driver.Cli.Common-008. Added a regression
|
|
row to `FormatStatus_does_not_apply_pre_fix_wrong_names` pinning that
|
|
`0x80550000` no longer renders as `BadDeviceFailure` (mirroring the
|
|
Driver.Cli.Common-001 wrong-name guards). The underlying constant was also
|
|
corrected in all six native-protocol mappers as part of the same commit:
|
|
`FocasStatusMapper.BadDeviceFailure`, `AbCipStatusMapper.BadDeviceFailure`,
|
|
`AbLegacyStatusMapper.BadDeviceFailure`, `TwinCATStatusMapper.BadDeviceFailure`,
|
|
`ModbusDriver.StatusBadDeviceFailure`, `S7Driver.StatusBadDeviceFailure` — all
|
|
moved from `0x80550000u` to `0x808B0000u`. The three downstream Modbus tests
|
|
(`ModbusExceptionMapperTests` 3 InlineData rows + 1 ShouldBe assertion;
|
|
`ExceptionInjectionTests.StatusBadDeviceFailure` constant) updated to expect
|
|
the corrected code. **Behavior change:** OPC UA clients consuming the native
|
|
drivers now see the canonical `BadDeviceFailure` (0x808B0000) instead of the
|
|
misnamed `BadSecurityPolicyRejected` (0x80550000) on device-fault paths —
|
|
operator-facing CLI output and machine-readable status semantics now agree.
|
|
Suite totals after fix: Driver.Cli.Common.Tests 43 green (was 48 — minus 5
|
|
redundant rows); Modbus.Tests 263; AbCip.Tests 262; AbLegacy.Tests 157;
|
|
FOCAS.Tests 178; S7.Tests 112; TwinCAT.Tests 131; all green. The Opc.Ua.StatusCodes
|
|
cross-check the recommendation suggested is recorded as a follow-up worth
|
|
considering but is out of scope for this fix.
|
|
|
|
### Driver.Cli.Common-008
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Testing coverage |
|
|
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:50-64` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** Commit `5a9c459` adds a new
|
|
`FormatStatus_names_native_driver_emitted_codes` `[Theory]` whose five
|
|
`[InlineData]` rows are identical to five rows added to the existing
|
|
`FormatStatus_names_well_known_status_codes` `[Theory]` in the same commit
|
|
(lines 32, 39, 40, 41, 42). The new Theory therefore adds no coverage. It is
|
|
also weaker than the Theory it duplicates: it asserts
|
|
`output.ShouldContain($"({expectedName})")` (substring match) where the
|
|
well-known Theory asserts `output.ShouldBe($"0x{status:X8} ({expectedName})")`
|
|
(exact match including the hex prefix). The substring form would not catch a
|
|
regression where the hex literal renders wrong but the name is correct.
|
|
|
|
This is not a correctness problem — both Theories pass — but it's a
|
|
copy-paste inconsistency that costs maintainer attention every time someone
|
|
reads the test file and wonders which Theory is authoritative.
|
|
|
|
**Recommendation:** Either (a) delete the new Theory entirely — its five rows
|
|
are already covered by the well-known Theory in the same commit — or (b) keep
|
|
it but switch to `ShouldBe($"0x{status:X8} ({expectedName})")` so its
|
|
assertion strength matches the rest of the file. Option (a) is cleaner: the
|
|
commit's "operator workflow" intent is documented well enough in the
|
|
well-known Theory comment block; the redundant Theory is dead weight.
|
|
|
|
**Resolution:** Resolved 2026-05-23 — took option (a): deleted the
|
|
`FormatStatus_names_native_driver_emitted_codes` Theory entirely. Its five
|
|
`InlineData` rows are covered by the well-known Theory's `ShouldBe` (strict
|
|
exact-match assertion), which is the authoritative shortlist test. Landed
|
|
alongside the Driver.Cli.Common-007 fix in the same commit.
|