From 7d66967122059f58d06e53a5525976584f392f9c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 24 May 2026 08:28:26 -0400 Subject: [PATCH] code-reviews: re-review IntegrationTests at 42b0037 Append 1 new finding (IntegrationTests-025): the ResolveRepositoryRoot_NoMarkers test walks up from Path.GetTempPath() through unisolated ancestors; a redirected TMP or co-located checkout at C:\src silently breaks the assertion. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/IntegrationTests/findings.md | 49 +++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/code-reviews/IntegrationTests/findings.md b/code-reviews/IntegrationTests/findings.md index 8dd24b7..9e6695f 100644 --- a/code-reviews/IntegrationTests/findings.md +++ b/code-reviews/IntegrationTests/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.MxGateway.IntegrationTests` | | Reviewer | Claude Code | | Review date | 2026-05-24 | -| Commit reviewed | `d692232` | -| Status | Reviewed | -| Open findings | 0 | +| Commit reviewed | `42b0037` | +| Status | Re-reviewed | +| Open findings | 1 | ## Checklist coverage @@ -59,6 +59,31 @@ a category produced nothing rather than leaving it blank. | 9 | Testing coverage | Issues found: IntegrationTests-005 (thin MXAccess parity coverage), IntegrationTests-006 (thin LDAP failure-path coverage). | | 10 | Documentation & comments | Issue found: IntegrationTests-009 (`TestServerCallContext` mislabelled "Mock"). | +### 2026-05-24 re-review (commit `42b0037`) + +Re-review pass at `42b0037` scoped to commit `865c22a` (the only commit +touching `src/ZB.MOM.WW.MxGateway.IntegrationTests` between `d692232` and +`42b0037` — all later commits in the range affect dashboard/clients only). +`865c22a` resolved IntegrationTests-022..024: the +`ResolveRepositoryRoot` throw replaces the silent +`Directory.GetCurrentDirectory()` fallback and ships a regression test; +`DashboardLdapLiveTests` adds the `Role: Admin` claim assertion; and +`NullDashboardEventBroadcaster` is extracted into +`TestSupport/NullDashboardEventBroadcaster.cs`. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Issue found: IntegrationTests-025 (the new `ResolveRepositoryRoot_NoMarkers_…` test seeds an "isolated" directory under `Path.GetTempPath()` and walks parents, implicitly assuming no ancestor of the temp dir contains `src/` plus `.git`/`*.sln`/`*.slnx`; the assumption is environment-dependent and can flap on machines whose `TMP` is redirected). | +| 2 | mxaccessgw conventions | No issues found — file-scoped namespaces, `sealed` types, accurate XML docs, env-var names preserved. | +| 3 | Concurrency & thread safety | No issues found in this diff. | +| 4 | Error handling & resilience | No issues found. The new `InvalidOperationException` only fires from the live-MXAccess worker-exe resolution path, which is gated by `MXGATEWAY_RUN_LIVE_MXACCESS_TESTS`; non-live tests are unaffected. | +| 5 | Security | No issues found — the new live-LDAP claim assertion adds no new credential surface. | +| 6 | Performance & resource management | No issues found. | +| 7 | Design-document adherence | No issues found. The diff is internal behavior hardening + a private-claim assertion; no public API / env-var / opt-in surface changes, so the CLAUDE.md "update docs in the same change" rule does not require a `docs/GatewayTesting.md` edit. | +| 8 | Code organization & conventions | No issues found. `NullDashboardEventBroadcaster`'s placement under `TestSupport/` matches the unit-test project layout; the deliberate duplication is documented in IntegrationTests-024's resolution. | +| 9 | Testing coverage | No issues found — the new role-claim assertion in `DashboardLdapLiveTests` and the new exception-path test in `IntegrationTestEnvironmentTests` both add coverage; no further gaps surface from this diff. | +| 10 | Documentation & comments | No issues found. XML docs on `ResolveRepositoryRoot` accurately describe the new throw, and the inline IntegrationTests-023 comment correctly explains *why* the role claim is asserted. | + ### 2026-05-24 review (commit d692232) Re-review pass at `d692232` scoped to: the rename (`dc9c0c9`) of the @@ -461,3 +486,21 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass **Recommendation:** When Tests-025 lands the shared `TestSupport/NullDashboardEventBroadcaster.cs`, either reference the same shared helper from this project (a project reference if practical) or accept the duplication as a deliberate isolation between the unit-test and integration-test trees. Either choice is fine; the current state is the only one that should not persist. **Resolution:** Resolved 2026-05-24 — Chose option (b) (extract within IntegrationTests) so the unit-test and integration-test projects stay independently buildable without a shared test-helpers project. Moved the inline private class out of `WorkerLiveMxAccessSmokeTests.cs` into a new public type at `src/ZB.MOM.WW.MxGateway.IntegrationTests/TestSupport/NullDashboardEventBroadcaster.cs` (namespace `ZB.MOM.WW.MxGateway.IntegrationTests.TestSupport`), mirroring the layout the unit-test Tests project established for Tests-007/Tests-021. The fixture wires through the same `NullDashboardEventBroadcaster.Instance` singleton, but the constructor is now private so future integration tests can rely on a single shared no-op. A new `using ZB.MOM.WW.MxGateway.IntegrationTests.TestSupport;` in `WorkerLiveMxAccessSmokeTests.cs` is the only change to its `EventStreamService` wiring. Build is green (0 warnings, 0 errors); no regression in the existing tests (4 passed / 15 skipped, identical to the pre-change baseline). + +### IntegrationTests-025 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs:57-84` (`ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers`) | +| Status | Open | + +**Description:** The new regression test for IntegrationTests-022 builds an "isolated" start directory under `Path.GetTempPath()` (e.g. `C:\Users\\AppData\Local\Temp\\nested` on Windows) and calls `ResolveRepositoryRoot(isolatedStart)`, asserting an `InvalidOperationException` is thrown. The walker walks every parent — ``, `Temp`, `Local`, `AppData`, ``, `Users`, `C:\` — and stops only when it either finds a repository root marker or runs out of parents. The test silently assumes none of those ancestor directories satisfies `IsRepositoryRoot` (a `src/` subdirectory next to `.git` / `*.sln` / `*.slnx`). The assumption is environment-dependent: + +1. `Path.GetTempPath()` honors the `TMP` / `TEMP` environment variable. A developer or CI runner that redirects `TMP` to a path under a checkout (e.g. `D:\work\repo\tmp`) — or simply has another git checkout at `C:\` (`C:\src` with a `.git`, common on Windows build agents) — will see the walker stop early at the real repo root and return that path instead of throwing. The `Assert.Throws` then fails with a misleading "No exception was thrown" message that does not name the path the walker actually returned. +2. The sibling test `ResolveRepositoryRoot_AcceptsGitWorktreeFile` (line 25) is unaffected because it walks from a directory it controls upward into an enclosing directory it also controls — the worktree file marker is encountered before any ancestor of `Path.GetTempPath()` is reached. The new test has the opposite shape: it walks past every directory the test owns into directories it does not control. + +The current dev box layout (`C:\Users\dohertj2\Desktop\mxaccessgw`) is safe because Temp is at `C:\Users\dohertj2\AppData\Local\Temp` and the walker exits at `C:\` without ever encountering `src/`. The fragility is invisible on this machine and only surfaces if the test ever runs in CI / on a contributor box with a less hermetic file-system layout. + +**Recommendation:** Isolate the walker from any ambient ancestor by either (a) constructing an `isolatedRoot` directly under a drive root and pointing the walker at a chain entirely under it (e.g. create `\level1\level2\level3` and start the walk at `level3`, then assert the throw — the walker stops at the drive root regardless of what is on it), (b) refactoring `ResolveRepositoryRoot` to accept an injectable `stopBoundary` parameter for tests and pass `isolatedRoot` as the boundary, or (c) replacing the `Assert.Throws` shape with an explicit upward-walk check that the test owns. Option (a) is the smallest change: prepend a sentinel — e.g. create a dummy `\sentinel-no-markers` and assert nothing about Temp ancestors — and pass the test only when the walker reaches that sentinel without finding a marker. The current shape is acceptable on the documented dev box but should not be the sole regression coverage for IntegrationTests-022.