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

review(Analyzers): add trip-coverage for async guarded-interface methods

Browse Source 
Re-review at 7286d320. -008: 5 regression tests for Unsubscribe/UnsubscribeAlarms/
Acknowledge/ReadEvents trip + suppression paths (analyzer source already correct).
Surfaced cross-module: Runtime DriverInstanceActor.HandleWriteAsync calls WriteAsync
directly (tracked for Runtime).
This commit is contained in:
Joseph Doherty
2026-06-19 11:21:35 -04:00
parent 6b4210cb17
commit 13c1215811
2 changed files with 164 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Analyzers/findings.md
+41 -2
View File
@@ -4,8 +4,8 @@
|---|---|
| Module | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Review date | 2026-06-19 |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 0 |
@@ -137,3 +137,42 @@
**Recommendation:** Tighten the `<remarks>` to state precisely what `IsInsideWrapperLambda` checks today (textual containment within a lambda argument of a `CapabilityInvoker` / `AlarmSurfaceInvoker`-typed invocation), and note the known limitation that it does not bind the lambda to the `callSite` parameter. Keep the doc in sync if Analyzers-001 is fixed.
**Resolution:** Resolved 2026-05-23 — Rewrote the analyzer's `<remarks>` into five precise paragraphs: (1) guarded-call detection uses symbol identity, (2) DIM handling (covers Analyzers-005), (3) wrapper-lambda detection matches both containing-type symbol AND method name, with the lambda-not-bound-to-callSite-parameter limitation called out explicitly, (4) why `AlarmSurfaceInvoker` is not a wrapper home (covers Analyzers-002 narrative), (5) the existing capability-argument-not-enforced caveat. The doc is now in sync with the post-Analyzers-001/-002/-004 implementation.
## Re-review 2026-06-19 (commit 7286d320)
All seven prior findings (Analyzers-001 through Analyzers-007) were closed at the previous review commit and the fixes are present at HEAD. The 273-line source and 883-line test file were re-read in full. The diff from `76d35d1` to `7286d320` is entirely the collection of prior-round fixes plus a migration of the test project's Roslyn package references to central package management (removing explicit `5.3.0` pins that conflicted with the SDK-shipped `5.0.0` compiler on this machine — correct change).
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No new issues |
| 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 | No issues found |
| 7 | Design-document adherence | No issues found |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | Analyzers-008 |
| 10 | Documentation & comments | No issues found |
### Analyzers-008
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs` |
| Status | Resolved |
**Description:** The 26-test suite added in the previous round covers most guarded-interface methods but leaves three async methods on guarded interfaces with no test at all and one wrapped-path (pass-clean) test missing:
- `ISubscribable.UnsubscribeAsync` — no positive-trip test, no wrapped-pass test.
- `IAlarmSource.UnsubscribeAlarmsAsync` — no positive-trip test, no wrapped-pass test.
- `IAlarmSource.AcknowledgeAsync` — no positive-trip test, no wrapped-pass test.
- `IHistoryProvider.ReadEventsAsync` wrapped inside `CapabilityInvoker.ExecuteAsync` — DIM trip is tested (`Direct_ReadEventsAsync_OnConcreteDriverInheritingDIM_TripsDiagnostic`), but the corresponding pass-clean case (wrapped DIM call) is absent, leaving the suppression path for `ReadEventsAsync` unexercised.
The analyzer is interface-typed and applies the same logic uniformly to every guarded-interface member, so these gaps do not hide a latent bug in the current code. However, missing positive-trip tests for three async guarded methods means a future regression (e.g. accidentally removing a metadata name from `GuardedInterfaceMetadataNames`) would not be caught by the test suite for those methods.
**Recommendation:** Add one positive-trip test and one pass-clean test for each of the three untested async methods, and one wrapped pass-clean test for `ReadEventsAsync`. All four methods already appear in `StubSources` so no stub changes are required.
**Resolution:** Resolved 2026-06-19 (SHA blank) — Added five regression tests: `Direct_UnsubscribeAsync_Call_TripsDiagnostic`, `Direct_UnsubscribeAlarmsAsync_Call_TripsDiagnostic`, `Direct_AcknowledgeAsync_Call_TripsDiagnostic`, `Wrapped_UnsubscribeAsync_InsideCapabilityInvokerLambda_PassesCleanly`, and `Wrapped_ReadEventsAsync_DIM_InsideCapabilityInvokerLambda_PassesCleanly`. All five pass against the existing analyzer with no source changes.