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

Resolve Worker.Tests-008..015 code-review findings

Browse Source 
Worker.Tests-008: moved the misplaced WorkerLogRedactor test out of
VariantConverterTests into Bootstrap/WorkerLogRedactorTests.

Worker.Tests-009: renamed 46 snake_case alarm-test methods to PascalCase
Method_Scenario_Expectation.

Worker.Tests-010: replaced a weak Assert.Contains with an exact assertion
against the real diagnostic message and corrected the XML doc.

Worker.Tests-011: renamed and re-documented a cancellation test that
overstated what it proved.

Worker.Tests-012: added an oversized-frame (MessageTooLarge) test; renamed
the mislabeled zero-length-payload test.

Worker.Tests-013: removed the fixed-100ms ThrowIfCompletedAsync helper; the
caller now races runTask deterministically.

Worker.Tests-014: consolidated duplicated test fakes/helpers
(FakeRuntimeSession, NoopComApartmentInitializer, NoopEventSink, frame
helpers) into a shared TestSupport namespace.

Worker.Tests-015: added MxAccessEventQueue coverage for drain-all (maxEvents
0), empty-queue drain, and enqueue-after-fault.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-18 22:59:07 -04:00
parent 9582de077b
commit 371bcb3f91
19 changed files with 507 additions and 512 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Worker.Tests/findings.md
+17 -17
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-18 |
| Commit reviewed | `6c64030` |
| Status | Reviewed |
| Open findings | 8 |
| Open findings | 0 |
## Checklist coverage
@@ -138,13 +138,13 @@
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/MxGateway.Worker.Tests/Conversion/VariantConverterTests.cs:175-182` |
| Status | Open |
| Status | Resolved |
**Description:** `Redactor_WithCredentialBearingValueFields_RedactsBeforeLogging` lives in `VariantConverterTests` but asserts on `WorkerLogRedactor.RedactValue`, which has nothing to do with `VariantConverter`. It is also a near-duplicate of coverage in `WorkerLogRedactorTests`. Placing redaction coverage inside the variant-converter class is misleading.
**Recommendation:** Move this test into `Bootstrap/WorkerLogRedactorTests.cs` (which already exists and tests `RedactFields`).
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — The misplaced redaction test was removed from `VariantConverterTests.cs` and re-added to `Bootstrap/WorkerLogRedactorTests.cs` as `RedactValue_WithCredentialBearingFieldNames_ReturnsRedactedValue` — alongside the existing `RedactFields` coverage, where redaction tests belong. Confirmed root cause: the old test asserted only on `WorkerLogRedactor.RedactValue` and never touched `VariantConverter`. The now-orphaned `using MxGateway.Worker.Bootstrap;` was removed from `VariantConverterTests.cs` (`TreatWarningsAsErrors`). The new home is `RedactValue` per-field coverage; `WorkerLogRedactorTests.RedactFields_...` already covers the dictionary path, so the two are complementary rather than duplicates.
### Worker.Tests-009
@@ -153,13 +153,13 @@
| Severity | Low |
| Category | Code organization & conventions |
| Location | `src/MxGateway.Worker.Tests/MxAccess/AlarmCommandHandlerTests.cs`, `AlarmDispatcherTests.cs`, `AlarmCommandExecutorTests.cs`, `AlarmRecordTransitionMapperTests.cs`, `WnWrapAlarmConsumerXmlTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The alarm-related test files use `snake_case` method names while the rest of the project uses the `Method_State_Result` PascalCase convention. `docs/style-guides/CSharpStyleGuide.md` and the surrounding code establish PascalCase as the project convention; the alarm files diverge.
**Recommendation:** Rename alarm-test methods to the `Method_Scenario_Expectation` PascalCase form for one consistent convention.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — Renamed every `[Fact]`/`[Theory]` method in the five alarm test files from `snake_case` to the project's `Method_Scenario_Expectation` PascalCase form (46 test methods total: 10 in `AlarmCommandHandlerTests`, 8 in `AlarmDispatcherTests`, 12 in `AlarmCommandExecutorTests`, 8 in `AlarmRecordTransitionMapperTests`, 9 in `WnWrapAlarmConsumerXmlTests` minus the existing PascalCase probe methods). Only test methods were renamed — `snake_case` is not present; the method names that *look* like helpers (`Subscribe`, `PollOnce`, `Dispose` on the fake doubles) are interface implementations of `IAlarmCommandHandler`/`IAlarmTransitionConsumer`/`IDisposable` and were correctly left unchanged. The suite stays green; xUnit discovers tests by attribute, not name, so the renames are behaviour-neutral.
### Worker.Tests-010
@@ -168,13 +168,13 @@
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/MxGateway.Worker.Tests/MxAccess/MxAccessStaSessionTests.cs:230-258` |
| Status | Open |
| Status | Resolved |
**Description:** `StartAsync_WithoutAlarmCommandHandlerFactory_SubscribeAlarmsReturnsInvalidRequest` asserts `Assert.Contains("alarm", reply.DiagnosticMessage, StringComparison.OrdinalIgnoreCase)`. The XML doc claims it verifies the diagnostic says "alarm consumer not configured", but the assertion only checks the substring "alarm" — which would also match an unrelated message like "invalid alarm GUID". The assertion is weaker than the documented intent.
**Recommendation:** Assert the full diagnostic phrase so the test fails if the diagnostic regresses to a misleading message.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — The weak `Assert.Contains("alarm", ...)` was replaced with an exact `Assert.Equal` against the diagnostic the executor actually emits. Re-triage: the test's XML doc claimed the phrase was "alarm consumer not configured", but `MxAccessCommandExecutor.ExecuteSubscribeAlarms` (verified in `src/MxGateway.Worker/MxAccess/MxAccessCommandExecutor.cs:310-315`) produces "SubscribeAlarms requires an alarm command handler; the worker was constructed without one." — the doc was wrong, so both the assertion and the XML doc were corrected to the real phrase. The test now fails if the diagnostic regresses to any other message.
### Worker.Tests-011
@@ -183,13 +183,13 @@
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/MxGateway.Worker.Tests/Sta/StaCommandDispatcherTests.cs:92-112` |
| Status | Open |
| Status | Resolved |
**Description:** `DispatchAsync_WhenCanceledAfterExecutionStarts_StillReturnsLateReply` is named and documented as if it proves cancellation arrived after execution began. The test does `Started.Wait(...)` then `cancellation.Cancel()`, which proves execution started, but because the executor is already running on the STA the cancellation is inherently a no-op — the test cannot distinguish "cancel was observed and ignored" from "cancel was never checked". The name overstates what is proven.
**Recommendation:** Either tighten the test (assert the dispatcher's cancel path was reached and declined) or rename/comment it to "cancellation cannot abort an in-flight STA command", matching `gateway.md`'s stated behavior.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — Took the rename/re-document option. The test is renamed `DispatchAsync_WhenCanceledWhileExecuting_DoesNotAbortInFlightCommand` and its XML doc rewritten to state exactly what it proves — an in-flight STA command is *not* aborted by cancellation — and to state explicitly that the test cannot and does not distinguish "cancel observed and ignored" from "cancel never checked". The doc now cites `gateway.md`'s wording ("cannot safely abort an in-flight COM call on the STA"). The test body is unchanged: it already asserts the command runs to completion and returns its normal `Ok` reply, which is the genuine behaviour. No runtime behaviour changed.
### Worker.Tests-012
@@ -198,13 +198,13 @@
| Severity | Low |
| Category | Testing coverage |
| Location | `src/MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** `docs/WorkerFrameProtocol.md` states the reader "rejects zero-length payloads and payloads larger than the configured maximum (default 16 MiB) before allocating the payload buffer." `WorkerFrameProtocolTests` covers malformed-length, wrong protocol version, wrong session, and malformed payload, but has no test for the zero-length-payload rejection or the oversized-frame rejection — both explicit security-relevant input-validation paths.
**Recommendation:** Add tests feeding a frame with `payload_length == 0` and one with `payload_length` above the configured maximum, asserting the corresponding `WorkerFrameProtocolErrorCode`.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — Re-triage of the zero-length half: the finding's "no test for the zero-length-payload rejection" is partly inaccurate. The pre-existing `ReadAsync_WithMalformedLength_ThrowsMalformedLength` fed a four-zero-byte stream — which is exactly a frame declaring `payload_length == 0` — so the zero-length path *was* already covered, just under a misleading name (the length prefix itself is well-formed; only the declared length is zero). That test was renamed `ReadAsync_WithZeroLengthPayload_ThrowsMalformedLength` with an XML doc explaining the four-zero-byte construction, rather than adding a duplicate. The oversized half was a genuine gap: a new `ReadAsync_WithPayloadAboveConfiguredMaximum_ThrowsMessageTooLarge` constructs `WorkerFrameProtocolOptions` with a 64-byte maximum, feeds a length prefix of 65, and asserts `WorkerFrameProtocolErrorCode.MessageTooLarge` — verified against `WorkerFrameReader.ReadAsync`, both checks fire before the payload buffer is rented. The small configured maximum keeps the test from allocating a multi-megabyte buffer.
### Worker.Tests-013
@@ -213,13 +213,13 @@
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `src/MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs:539-546` |
| Status | Open |
| Status | Resolved |
**Description:** `ThrowIfCompletedAsync` does an unconditional `await Task.Delay(TimeSpan.FromMilliseconds(100))` then checks `task.IsCompleted`. This adds a fixed 100 ms to the test and only catches a `RunAsync` that fails within that arbitrary window; a session that faults after 100 ms slips past undetected.
**Recommendation:** Replace with a deterministic race: `await Task.WhenAny(runTask, <first-expected-frame-read>)` and assert the run task did not win.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — `ThrowIfCompletedAsync` was deleted (it had a single call site, in `RunAsync_SendsHeartbeatPayloadFromRuntimeSnapshot`). That test now races `runTask` against the first-heartbeat `ReadUntilAsync` with `Task.WhenAny`; if `runTask` wins it is awaited to surface the underlying fault and the test fails via `Assert.Fail`. The fixed 100 ms delay is gone — the check is now deterministic: a `RunAsync` faulting at *any* time before the first heartbeat is caught, and a healthy run completes as soon as the heartbeat arrives instead of always paying 100 ms.
### Worker.Tests-014
@@ -228,13 +228,13 @@
| Severity | Low |
| Category | Code organization & conventions |
| Location | `src/MxGateway.Worker.Tests/Ipc/WorkerPipeClientTests.cs:194`, `WorkerPipeSessionTests.cs:622`, `Sta/StaCommandDispatcherTests.cs:348`, `MxAccess/MxAccessStaSessionTests.cs:334`, `MxAccess/MxAccessCommandExecutorTests.cs:1124` |
| Status | Open |
| Status | Resolved |
**Description:** `FakeRuntimeSession`, `NoopComApartmentInitializer`, `NoopEventSink`/`NullEventSink`, and the `CreateFrame`/`WriteUInt32LittleEndian` helpers are re-implemented independently in multiple test files. The two `FakeRuntimeSession` implementations have already diverged (one supports `BlockDispatch`/event enqueue, one does not), and `NoopComApartmentInitializer` is defined four times.
**Recommendation:** Extract shared test doubles (`NoopComApartmentInitializer`, frame helpers, a single configurable `FakeRuntimeSession`) into a `TestSupport` folder/namespace consumed by all test classes.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — Added a `src/MxGateway.Worker.Tests/TestSupport/` folder (namespace `MxGateway.Worker.Tests.TestSupport`) with four shared doubles: `NoopComApartmentInitializer`, `NoopEventSink`, `WorkerFrameTestHelpers` (`CreateFrame`/`WriteUInt32LittleEndian`), and a single configurable `FakeRuntimeSession`. The consolidated `FakeRuntimeSession` is the richer of the two divergent copies (it supports `BlockDispatch`, event enqueue, shutdown-timeout, and throw-after-release); the minimal `WorkerPipeClientTests` caller simply leaves the options unset. The per-file copies were deleted from `WorkerPipeClientTests`, `WorkerPipeSessionTests`, `StaCommandDispatcherTests`, `MxAccessStaSessionTests`, `MxAccessCommandExecutorTests`, and `WorkerFrameProtocolTests`, and the orphaned `NullEventSink` in `AlarmCommandExecutorTests` was replaced with the shared `NoopEventSink`. Re-triage: the finding says `NoopComApartmentInitializer` "is defined four times" — it was defined **three** times (`StaCommandDispatcherTests`, `MxAccessStaSessionTests`, `MxAccessCommandExecutorTests`); the fourth alarm-area `IStaComApartmentInitializer` implementation is `StaRuntimeTests.RecordingComApartmentInitializer`, which is a *recording* double (asserts init/uninit ordering), not a no-op, so it was deliberately left in place rather than folded into the shared no-op. Unused `using` directives left behind by the removals were stripped (`TreatWarningsAsErrors`).
### Worker.Tests-015
@@ -243,10 +243,10 @@
| Severity | Low |
| Category | Testing coverage |
| Location | `src/MxGateway.Worker.Tests/MxAccess/MxAccessEventQueueTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** `MxAccessEventQueueTests` covers monotonic sequencing, drain, capacity overflow, and first-fault-wins, but does not cover `Drain` with `maxEvents: 0` (drain-all) — a branch `FakeRuntimeSession.DrainEvents` even special-cases — nor draining an empty queue, nor enqueue after a manual `RecordFault`. These are minor branches but the overflow/fault interaction is the worker's backpressure contract.
**Recommendation:** Add a `Drain(0)` drain-all test and an empty-queue drain test.
**Resolution:** _(open)_
**Resolution:** 2026-05-18 — Added three tests to `MxAccessEventQueueTests`. `Drain_WithZeroMaxEvents_DrainsAllEvents` covers the `maxEvents == 0` drain-all branch in `MxAccessEventQueue.Drain` (verified at `src/MxGateway.Worker/MxAccess/MxAccessEventQueue.cs:174`) — three events enqueued, `Drain(0)` returns all three in order and empties the queue. `Drain_WhenQueueIsEmpty_ReturnsEmptyList` covers the `drainCount == 0` early-return branch for both `Drain(0)` and `Drain(5)` on an empty queue. `Enqueue_AfterRecordFault_ThrowsInvalidOperationException` covers the backpressure contract gap the finding flagged — after a manual `RecordFault`, `Enqueue` throws `InvalidOperationException` ("outbound event queue is faulted") and the event is not queued.