diff --git a/code-reviews/Driver.Historian.Gateway/findings.md b/code-reviews/Driver.Historian.Gateway/findings.md new file mode 100644 index 00000000..76ce846a --- /dev/null +++ b/code-reviews/Driver.Historian.Gateway/findings.md @@ -0,0 +1,239 @@ +# Code Review — Driver.Historian.Gateway + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway` | +| Reviewer | Claude Code | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 0 | + +## Scope + +The sole historian backend: a thin gRPC seam (`IHistorianGatewayClient` / +`HistorianGatewayClientAdapter`) over the `ZB.MOM.WW.HistorianGateway.Client` package, plus +`GatewayHistorianDataSource` (HistoryRead), `GatewayAlarmHistorianWriter` (alarm `SendEvent`), +`GatewayTagProvisioner` (`EnsureTags` upsert), the `Mapping/` pure mappers, and the +`Recorder/` continuous-historization write path (`FasterLogHistorizationOutbox`, +`GatewayHistorianValueWriter`, `HistorizationOutboxEntrySerializer`). 17 source files, ~1,850 LOC +(excluding generated/obj); test suite `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests` +(130 tests: 124 offline unit + 6 env-gated `Category=LiveIntegration`, skip cleanly offline). + +Read against `CLAUDE.md`'s "Historian / HistoryRead" section in full before reviewing — several +things that look like defects on first read are deliberate, documented decisions (Boolean→Int2, +the dead `Quality` field on `WriteLiveValues`, the per-path-own-channel design, the two `EnsureTags` +gotchas, the #491/#537/#538 live-gate history). None of those were re-litigated here. + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | 1 finding (Driver.Historian.Gateway-001, High) — Resolved | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | 1 finding (Driver.Historian.Gateway-002, Low) — Resolved | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | 1 finding (Driver.Historian.Gateway-003, Medium) — Resolved | +| 10 | Documentation & comments | No issues found | + +### Category notes + +**1. Correctness & logic bugs.** See Driver.Historian.Gateway-001 (Severity write/read mismatch). +Everything else checked out: `SampleMapper`/`GatewayQualityMapper` quality-byte bucketing is exact +and tested per-byte; `AggregateModeMapper`/`HistorianTypeMapper` are exhaustive-matrix-guarded +(`Every_..._member_is_classified` tests fail on a future unmapped enum member rather than silently +defaulting); `GatewayReadEventsTests` confirms the truncation/continuation-point boundary +(`collected.Count == maxEvents` checked *before* `Add`, so an exact-cap read has no continuation but +one more matching event sets it); `AlignAtTimeSnapshots` correctly keeps the first of a duplicate +timestamp and Bad-fills gaps at the *requested* time, not the nearest returned one. + +**2. OtOpcUa conventions.** Driver-level code contains no OPC UA SDK reference (`uint` status codes +via the named `HistorianStatusCodes`, not the SDK's `Opc.Ua.StatusCodes` — matches the repo's stated +pattern of keeping the driver layer SDK-free so `StatusCodeParityTests` elsewhere can reflect-guard +it). No authorization decisions are made in this driver — HistoryRead access-level gating happens at +materialization in the server layer per CLAUDE.md, and this module is a pure translation seam. +Serilog-style `ILogger` used throughout via constructor injection. + +**3. Concurrency & thread safety.** `GatewayHistorianDataSource`'s health counters +(`_totalQueries`/`_totalSuccesses`/`_totalFailures`/etc.) are all mutated and read under one +`_healthLock`, preserving the `TotalQueries == TotalSuccesses + TotalFailures` invariant at every +observed snapshot (per its own XML remarks and `GatewayHealthSnapshotTests`). +`FasterLogHistorizationOutbox` documents an explicit single-writer assumption (the recorder actor +processes messages sequentially) and its `_state` lock protects only the in-memory FIFO index, never +held across an `await` — correct, since FasterLog itself tolerates concurrent enqueue/scan. No shared +mutable state was found that lacked a lock or a documented ownership model. + +**4. Error handling & resilience.** `GatewayAlarmHistorianWriter` and `GatewayHistorianValueWriter` +both honor their "never throws" contracts (verified by dedicated exception-classification tests +covering the typed client hierarchy, a defensive raw `RpcException` path, and an unclassifiable +exception default). Cancellation is handled as a first-class, non-poisoning case in both the alarm +writer (mid-drain cancellation yields `RetryPlease`, never `PermanentFail`) and the tag provisioner +(a shutdown `OperationCanceledException` is logged quietly and counted `Failed`, never thrown). The +fire-and-forget `EnsureTagsAsync` hook is genuinely non-blocking end to end — no `await` in its +caller's synchronous path can escape as an exception (verified by tracing `AddressSpaceApplier`'s +fire-and-forget dispatch, outside this module's scope but confirmed non-blocking here). + +**5. Security.** No API key, tag value, hostname, or credential is ever passed to a logger call +anywhere in the module (audited every `_logger.Log*` call site) — failures are logged by exception +*type name* only. `HistorianGatewayClientAdapter.Create` echoes the `Endpoint` in its +`InvalidOperationException` (deliberately, documented as non-secret) but never the `ApiKey`. TLS / +certificate-validation options (`UseTls`, `AllowUntrustedServerCertificate`, `CaCertificatePath`) map +onto the package client with the documented inverted `RequireCertificateValidation` semantics and are +correctly *not* forwarded on a plaintext h2c connection (pinned by +`Adapter_ignores_tls_only_settings_on_a_plaintext_endpoint`). No SQL or other injectable surface in +this module. + +**6. Performance & resource management.** See Driver.Historian.Gateway-002 (missing dispose on +`GatewayTagProvisioner`). Everything else: `GatewayHistorianDataSource` and +`GatewayAlarmHistorianWriter` both correctly dispose their owned `IHistorianGatewayClient` (verified +by dedicated tests); the three `GatewayHistorian.Create*` factories deliberately open independent +gRPC channels per path rather than sharing one, a documented and reasonable trade (each channel's +lifetime stays simple; the gateway pools/amortizes historian sessions server-side). Channel +construction is lazy (no network I/O at construction, confirmed by the adapter's own tests) so a +misconfigured/disabled path never dials. Per-call deadlines exist via +`ServerHistorianOptions.CallTimeout` → `HistorianGatewayClientOptions.DefaultCallTimeout`, and every +seam method threads a `CancellationToken` through to the package client — no unbounded call was found. + +**7. Design-document adherence.** Matches `CLAUDE.md`'s "Historian / HistoryRead" section point for +point: gateway is the sole backend; Boolean→Int2 is implemented exactly as documented (with an +explicit-presence pin test guarding the proto3-optional contract); the continuous-historization +outbox/writer/serializer shape matches the FasterLog-outbox design; the `Quality` field is +deliberately populated despite being a documented-dead input on the SQL write path, with the +reasoning captured in the class's own remarks (proto3 default-0 would be a worse lie than the +correct-but-ignored 192). No stale documentation was found in this module. + +**8. Code organization & conventions.** Clean separation: `Mapping/` holds pure, stateless, +internal-visibility mappers (each independently unit-tested); `Recorder/` holds the +continuous-historization write path; the top-level files are the three public seams +(`IHistorianGatewayClient`, the adapter, and the `GatewayHistorian` factory facade the Host consumes). +`InternalsVisibleTo` is scoped to the test project only. The `ServerHistorianOptions` type living in +`Runtime` rather than this driver (creating the documented Host→{Runtime, Gateway}, Gateway→Runtime +diamond) is explained inline in the `.csproj` and is a reasonable ownership call, not a layering +violation. + +**9. Testing coverage.** See Driver.Historian.Gateway-003 (two offline-coverage gaps, now closed). +Otherwise the suite is thorough: every mapper has a matrix-guard test for unmapped enum members, +every writer's outcome-classification switch is covered by every named branch (including the +FIFO-truncation semantics of the FasterLog outbox — append/peek/remove/restart-durability/capacity +drop-oldest/periodic-commit-recovery all have dedicated tests), and the six env-gated +`Category=LiveIntegration` tests (skip cleanly without `HISTGW_*` env vars, confirmed offline) cover +the write/read/alarm/EnsureTags round trips against a real gateway per `docs/Historian.md`'s +re-run recipe. + +**10. Documentation & comments.** XML docs are extensive, accurate against the code they describe, +and several carry load-bearing historical context (e.g. why each `GatewayHistorian.Create*` factory +opens its own channel, why `Quality` is populated despite being server-ignored, why +`HistorianEvent.Id` is deliberately left unset). No misleading or stale comment was found. + +## Findings + +### Driver.Historian.Gateway-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs:39` (write side) and `Mapping/EventMapper.cs:62` (read side) | +| Status | Resolved | + +**Description:** `AlarmEventMapper.ToHistorianEvent` wrote the alarm's `Severity` wire property as +`alarm.Severity.ToString()` — the .NET enum's *name* (`"Low"`/`"Medium"`/`"High"`/`"Critical"`). +`AlarmHistorianEvent.Severity`'s own XML doc states the contract: "Mapped to Historian's numeric +priority on the sink side." The read side, `EventMapper.ParseSeverity`, `int.TryParse`s that exact +property and falls back to the minimum severity (1) on any unparseable value — which an enum name +always is. So round-tripping any alarm event's severity through this module's own write mapper then +its own read mapper silently collapsed every severity to the minimum, regardless of the real +severity: a `Critical` alarm and a `Low` alarm would read back identically as severity 1. This is a +self-contained defect (both mappers live in this module) independent of whether the gateway's own +storage happens to echo arbitrary custom event properties back on `ReadEvents` — the write and read +mappers disagreed with each other and with their own documented contract, and the existing test +(`AlarmEventMapperTests.Maps_source_time_type_and_rich_properties`) asserted the *wrong* value +(`"High"`) as correct, so no test caught it. + +**Recommendation:** Map `AlarmSeverity` to a numeric OPC UA Part 9 priority string before writing it +to the wire property, using the same severity convention already established elsewhere in the +codebase for this exact enum (`ZB.MOM.WW.OtOpcUa.Runtime.Drivers.NativeAlarmProjector.MapSeverity`: +Low=200, Medium=500, High=700, Critical=900). + +**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added a private +`AlarmEventMapper.ToNumericSeverity` mirroring `NativeAlarmProjector.MapSeverity`'s convention and +routed the `Severity` property through it instead of `alarm.Severity.ToString()`. TDD: added +`AlarmEventMapperTests.Severity_round_trips_through_EventMapper_as_numeric_priority` (a +`[Theory]` over all four `AlarmSeverity` values, round-tripping through both mappers) and corrected +the existing assertion to `"700"`; both were RED against the pre-fix code (confirmed by running the +suite before the source change) and are GREEN after it. All 130 module tests pass +(124 run + 6 skipped `LiveIntegration`). + +### Driver.Historian.Gateway-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs` | +| Status | Resolved | + +**Description:** Unlike its two siblings — `GatewayHistorianDataSource` (`IAsyncDisposable`, +disposes its `IHistorianGatewayClient`) and `GatewayAlarmHistorianWriter` (`IDisposable`, same) — +`GatewayTagProvisioner` implemented neither `IDisposable` nor `IAsyncDisposable`, so the gRPC channel +`GatewayHistorian.CreateProvisioner` opens for it was never closed. `IHistorianProvisioning` (the +interface it's registered as) does not declare disposal either, so nothing in the codebase had any +way to close that channel. Impact is bounded — `AddHistorianProvisioning` registers it as a single +process-lifetime DI singleton (built once at startup, not per-deploy), so this was not an +accumulating leak — but it is a real gap in graceful-shutdown hygiene and an inconsistency with the +established pattern in this same module. + +**Recommendation:** Implement `IAsyncDisposable` on `GatewayTagProvisioner`, disposing the +underlying `IHistorianGatewayClient` — the built-in `Microsoft.Extensions.DependencyInjection` +container tracks and disposes any singleton instance it constructs that implements +`IDisposable`/`IAsyncDisposable`, regardless of the declared service-interface type, so no other +wiring change is required. + +**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added +`GatewayTagProvisioner : IHistorianProvisioning, IAsyncDisposable` with a `DisposeAsync()` delegating +to `_client.DisposeAsync()`. TDD: added +`GatewayTagProvisionerTests.DisposeAsync_disposes_underlying_client`, confirmed RED (compile error — +`GatewayTagProvisioner` did not implement `IAsyncDisposable`) before the fix, GREEN after. + +### Driver.Historian.Gateway-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Testing coverage | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Recorder/GatewayHistorianValueWriter.cs`, `Recorder/HistorizationOutboxEntrySerializer.cs` | +| Status | Resolved | + +**Description:** Two production-critical pieces of the continuous-historization write path had no +offline unit coverage: + +- `GatewayHistorianValueWriter` — the recorder's `WriteLiveValues` adapter, and per CLAUDE.md's + "KNOWN LIMITATION 2" history the exact class where the #491 `ConfigureAwait(false)`-destroys-`Self` + defect and the #537/#538 gate findings were discovered live — was exercised only by the env-gated + `Category=LiveIntegration` suite (`tests/.../Live/GatewayLiveIntegrationTests.cs`), which skips + cleanly offline. Its ack-to-bool mapping (`Success || Queued`), timestamp-presence handling (a null + timestamp must leave the proto field unset, not send a default), and non-throwing exception + contract had zero offline verification. +- `HistorizationOutboxEntrySerializer` — a hand-rolled fixed-layout little-endian binary + (de)serializer with manual span-offset arithmetic — was only exercised *indirectly* through + `FasterLogHistorizationOutboxTests`, and only ever with single-character ASCII tag names ("A"/"B"/ + "C"), never testing the one part of the layout arithmetic (the UTF-8 *byte*-length prefix vs. the + tag's *char* length) that a multi-byte tag name would exercise. + +**Recommendation:** Add direct offline unit tests for `GatewayHistorianValueWriter` (reusing the +existing `FakeHistorianGatewayClient` test double already used by every other seam consumer in this +module) and for `HistorizationOutboxEntrySerializer` (direct Serialize/Deserialize round trips, +including a multi-byte UTF-8 tag name, an empty tag, and boundary `ushort`/`double` values). + +**Resolution:** `a86a0976` (branch `review/historian-gateway`), 2026-08-10. Added +`tests/.../Recorder/GatewayHistorianValueWriterTests.cs` (10 tests: success/queued/soft-failure ack +mapping, exception swallowing, empty-batch short-circuit, timestamp presence/absence, quality/value +forwarding, null-argument guards, dispose delegation) and +`tests/.../Recorder/HistorizationOutboxEntrySerializerTests.cs` (4 tests: full round trip incl. +`DateTimeKind` preservation, a multi-byte UTF-8 tag `"Pumpe_Ω_日本語_Temp"`, an empty tag, and +`double.MinValue`/`ushort.MaxValue` boundaries). All 14 new tests pass; full module suite is +124 passed / 6 skipped (LiveIntegration) / 0 failed. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs index 986f82ab..b160f04f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs @@ -17,7 +17,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway; /// as and returns. The method never throws and never /// logs tag values, hostnames, or credentials. /// -public sealed class GatewayTagProvisioner : IHistorianProvisioning +public sealed class GatewayTagProvisioner : IHistorianProvisioning, IAsyncDisposable { /// /// Storage rate (ms) stamped on every provisioned analog tag. The gateway's EnsureTags path @@ -104,4 +104,13 @@ public sealed class GatewayTagProvisioner : IHistorianProvisioning return new HistorianProvisionResult(requests.Count, Ensured: 0, Skipped: skipped, Failed: definitions.Count); } } + + /// + /// Disposes the underlying gateway client (and its gRPC channel). The DI container owns this + /// provisioner as a singleton (AddHistorianProvisioning), so this fires once at host + /// shutdown — closing the channel gracefully, matching the read () + /// and alarm-write () paths' dispose ownership. + /// + /// A task that represents the asynchronous operation. + public ValueTask DisposeAsync() => _client.DisposeAsync(); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs index a0112dcd..bdda3fc5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs @@ -1,5 +1,6 @@ using Google.Protobuf.WellKnownTypes; using ZB.MOM.WW.HistorianGateway.Contracts.Grpc; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian; namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Mapping; @@ -10,6 +11,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Mapping; /// internal static class AlarmEventMapper { + /// + /// Maps to the numeric OPC UA Part 9 priority (1-1000) carried in the + /// wire event's Severity property, per the + /// contract ("Mapped to Historian's numeric priority on the sink side"). The read side + /// () int.TryParses this same property, so it + /// must stay a numeric string — the enum's .ToString() name (e.g. "High") is NOT + /// parseable and used to silently collapse every read-back severity to the minimum (1). The + /// values mirror the existing native-alarm convention + /// (ZB.MOM.WW.OtOpcUa.Runtime.Drivers.NativeAlarmProjector.MapSeverity). + /// + private static string ToNumericSeverity(AlarmSeverity severity) => severity switch + { + AlarmSeverity.Low => "200", + AlarmSeverity.Medium => "500", + AlarmSeverity.High => "700", + AlarmSeverity.Critical => "900", + _ => "500", + }; + /// Maps an alarm historian event to a gateway event. /// The driver-agnostic alarm event. /// The gateway wire event ready for SendEvent. @@ -36,7 +56,7 @@ internal static class AlarmEventMapper historianEvent.Properties["AlarmId"] = alarm.AlarmId; historianEvent.Properties["AlarmName"] = alarm.AlarmName; historianEvent.Properties["EventKind"] = alarm.EventKind; - historianEvent.Properties["Severity"] = alarm.Severity.ToString(); + historianEvent.Properties["Severity"] = ToNumericSeverity(alarm.Severity); historianEvent.Properties["User"] = alarm.User; historianEvent.Properties["Message"] = alarm.Message; if (alarm.Comment is not null) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs index b03a9af9..798a0dca 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/GatewayTagProvisionerTests.cs @@ -149,6 +149,27 @@ public sealed class GatewayTagProvisionerTests Assert.Equal(1, result.Failed); // non-blocking: no throw } + /// + /// Driver.Historian.Gateway-002: unlike its siblings (GatewayHistorianDataSource, + /// GatewayAlarmHistorianWriter), had no dispose path at + /// all — the gRPC channel GatewayHistorian.CreateProvisioner opens was never closed. The + /// provisioner is registered as a process-lifetime DI singleton + /// (AddHistorianProvisioning), and the built-in container disposes any singleton instance + /// it creates that implements / — regardless + /// of the registered service-interface type — so implementing it here is sufficient for a clean + /// shutdown with no other wiring change. + /// + [Fact] + public async Task DisposeAsync_disposes_underlying_client() + { + var fake = new FakeHistorianGatewayClient(); + var p = Provisioner(fake); + + await ((IAsyncDisposable)p).DisposeAsync(); + + Assert.Equal(1, fake.DisposeCallCount); + } + [Fact] public async Task Ensured_count_reflects_successful_results() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs index f26784ec..e0b489b1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Mapping/AlarmEventMapperTests.cs @@ -23,12 +23,41 @@ public sealed class AlarmEventMapperTests Assert.Equal("A1", e.Properties["AlarmId"]); Assert.Equal("HiHi", e.Properties["AlarmName"]); Assert.Equal("Activated", e.Properties["EventKind"]); - Assert.Equal("High", e.Properties["Severity"]); + // Severity is the AlarmHistorianEvent.Severity XML doc contract: "Mapped to Historian's + // numeric priority on the sink side" — NOT the .NET enum name. EventMapper.ParseSeverity on + // the read side int.TryParse()s this same property, so a non-numeric string (the old bug: + // alarm.Severity.ToString() == "High") silently always read back as the minimum severity (1). + Assert.Equal("700", e.Properties["Severity"]); Assert.Equal("operator1", e.Properties["User"]); Assert.Equal("ack note", e.Properties["Comment"]); Assert.Equal("Temp high", e.Properties["Message"]); } + /// + /// Driver.Historian.Gateway-001 regression: the write side () and + /// the read side () must agree on the wire representation of severity — + /// a numeric OPC UA Part 9 priority string, matching the convention already used elsewhere in the + /// codebase for the same enum (NativeAlarmProjector.MapSeverity: Low=200/Medium=500/ + /// High=700/Critical=900). Round-tripping through both mappers must reproduce the numeric severity, + /// not collapse to the minimum every time. + /// + [Theory] + [InlineData(AlarmSeverity.Low, (ushort)200)] + [InlineData(AlarmSeverity.Medium, (ushort)500)] + [InlineData(AlarmSeverity.High, (ushort)700)] + [InlineData(AlarmSeverity.Critical, (ushort)900)] + public void Severity_round_trips_through_EventMapper_as_numeric_priority(AlarmSeverity severity, ushort expected) + { + var a = new AlarmHistorianEvent("A1", "Area/Line/Pump1", "HiHi", "LimitAlarm", + severity, "Activated", "m", "operator1", null, + new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + + var wire = AlarmEventMapper.ToHistorianEvent(a); + var readBack = EventMapper.ToHistoricalEvent(wire); + + Assert.Equal(expected, readBack.Severity); + } + [Fact] public void Null_comment_is_omitted_not_null() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs new file mode 100644 index 00000000..3367cfa2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/GatewayHistorianValueWriterTests.cs @@ -0,0 +1,149 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; +using ZB.MOM.WW.HistorianGateway.Contracts.Grpc; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Recorder; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests.Recorder; + +/// +/// Driver.Historian.Gateway-003 (offline unit coverage for the continuous-historization write path): +/// before this file, — the recorder's WriteLiveValues +/// adapter, in production the write path CLAUDE.md's #491/#537/#538 live-gate history bit hard — was +/// exercised only by the env-gated Category=LiveIntegration suite, which skips cleanly offline. +/// These tests use the same the rest of the module's offline +/// tests share. +/// +public sealed class GatewayHistorianValueWriterTests +{ + private static GatewayHistorianValueWriter Writer(FakeHistorianGatewayClient fake) => + new(fake, NullLogger.Instance); + + [Fact] + public async Task Success_ack_returns_true() + { + var fake = new FakeHistorianGatewayClient { WriteLiveValuesResult = new WriteAck { Success = true } }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 42.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.True(ok); + Assert.Equal(1, fake.WriteLiveValuesCallCount); + Assert.Equal("T", fake.LastWriteLiveTag); + } + + [Fact] + public async Task Queued_ack_is_treated_as_success() + { + // A value the gateway durably store-forward-queued must not be re-drained by the recorder. + var fake = new FakeHistorianGatewayClient + { + WriteLiveValuesResult = new WriteAck { Success = false, Queued = true }, + }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.True(ok); + } + + [Fact] + public async Task Soft_failure_ack_returns_false_for_retry() + { + var fake = new FakeHistorianGatewayClient + { + WriteLiveValuesResult = new WriteAck { Success = false, Queued = false }, + }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.False(ok); + } + + [Fact] + public async Task Gateway_exception_is_swallowed_and_returns_false() + { + // NEVER throw out of the writer — the recorder's drain loop expects a plain bool. + var fake = new FakeHistorianGatewayClient { WriteLiveValuesThrows = new InvalidOperationException("boom") }; + var values = new[] { new HistorizationValue(DateTime.UtcNow, 1.0, 192) }; + + var ok = await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + Assert.False(ok); + } + + [Fact] + public async Task Empty_batch_is_trivially_acked_without_calling_the_gateway() + { + var fake = new FakeHistorianGatewayClient(); + + var ok = await Writer(fake).WriteLiveValuesAsync( + "T", Array.Empty(), TestContext.Current.CancellationToken); + + Assert.True(ok); + Assert.Equal(0, fake.WriteLiveValuesCallCount); + } + + [Fact] + public async Task Present_timestamp_is_sent_and_absent_timestamp_leaves_the_proto_field_unset() + { + var fake = new FakeHistorianGatewayClient(); + var stamped = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + var values = new[] + { + new HistorizationValue(stamped, 1.0, 192), + new HistorizationValue(null, 2.0, 192), + }; + + await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + var sent = fake.LastWriteLiveValues!; + Assert.Equal(2, sent.Count); + Assert.Equal(stamped, sent[0].Timestamp!.ToDateTime()); + Assert.Null(sent[1].Timestamp); // absent -> unset proto field -> gateway server-stamps now + } + + [Fact] + public async Task Quality_and_numeric_value_are_forwarded_unchanged() + { + var fake = new FakeHistorianGatewayClient(); + var values = new[] { new HistorizationValue(null, 123.5, 192) }; + + await Writer(fake).WriteLiveValuesAsync("T", values, TestContext.Current.CancellationToken); + + var sent = fake.LastWriteLiveValues!; + Assert.Equal(123.5, sent[0].NumericValue); + Assert.Equal(192u, sent[0].Quality); + } + + [Fact] + public async Task Null_tag_throws() + { + var fake = new FakeHistorianGatewayClient(); + var values = new[] { new HistorizationValue(null, 1.0, 192) }; + + await Assert.ThrowsAsync(() => + Writer(fake).WriteLiveValuesAsync(null!, values, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task Null_values_throws() + { + var fake = new FakeHistorianGatewayClient(); + + await Assert.ThrowsAsync(() => + Writer(fake).WriteLiveValuesAsync("T", null!, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task DisposeAsync_disposes_underlying_client() + { + var fake = new FakeHistorianGatewayClient(); + var writer = Writer(fake); + + await writer.DisposeAsync(); + + Assert.Equal(1, fake.DisposeCallCount); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs new file mode 100644 index 00000000..0efdc6ae --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/HistorizationOutboxEntrySerializerTests.cs @@ -0,0 +1,73 @@ +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Recorder; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests.Recorder; + +/// +/// Driver.Historian.Gateway-003 (offline unit coverage): +/// exercises only indirectly, and only with short +/// ASCII tag names ("A"/"B"/"C"). The hand-rolled little-endian layout computes its tag byte-length +/// from Encoding.UTF8.GetByteCount before writing, so a multi-byte tag exercises the one part +/// of the layout arithmetic the indirect coverage never reaches. This asserts the (de)serializer +/// directly, in isolation from FasterLog. +/// +public sealed class HistorizationOutboxEntrySerializerTests +{ + [Fact] + public void Round_trips_all_fields() + { + var entry = new HistorizationOutboxEntry( + Guid.NewGuid(), "Pump1.Temp", 123.456, 192, + new DateTime(2026, 1, 1, 12, 34, 56, DateTimeKind.Utc)); + + var bytes = HistorizationOutboxEntrySerializer.Serialize(entry); + var round = HistorizationOutboxEntrySerializer.Deserialize(bytes); + + Assert.Equal(entry.Id, round.Id); + Assert.Equal(entry.Tag, round.Tag); + Assert.Equal(entry.NumericValue, round.NumericValue); + Assert.Equal(entry.Quality, round.Quality); + Assert.Equal(entry.TimestampUtc, round.TimestampUtc); + Assert.Equal(entry.TimestampUtc.Kind, round.TimestampUtc.Kind); // ToBinary/FromBinary round-trips Kind + } + + [Fact] + public void Round_trips_a_multi_byte_utf8_tag() + { + // The tag length prefix is a UTF-8 BYTE count, not a char count — a tag whose char count and + // byte count diverge (e.g. Ω is 2 bytes, 日 is 3) is the one case that would desync the layout + // if GetByteCount/GetBytes ever disagreed with what Deserialize slices out. + var entry = new HistorizationOutboxEntry( + Guid.NewGuid(), "Pumpe_Ω_日本語_Temp", -1.5, 0, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(entry.Tag, round.Tag); + Assert.Equal(entry.NumericValue, round.NumericValue); + } + + [Fact] + public void Round_trips_an_empty_tag() + { + var entry = new HistorizationOutboxEntry(Guid.NewGuid(), string.Empty, 0.0, 0, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(string.Empty, round.Tag); + } + + [Fact] + public void Round_trips_boundary_quality_and_negative_value() + { + var entry = new HistorizationOutboxEntry(Guid.NewGuid(), "T", double.MinValue, ushort.MaxValue, DateTime.UtcNow); + + var round = HistorizationOutboxEntrySerializer.Deserialize( + HistorizationOutboxEntrySerializer.Serialize(entry)); + + Assert.Equal(ushort.MaxValue, round.Quality); + Assert.Equal(double.MinValue, round.NumericValue); + } +}