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

code-reviews: re-review IntegrationTests at 42b0037

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-24 08:28:26 -04:00
parent 2f8404d2ef
commit 7d66967122
1 changed files with 46 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/IntegrationTests/findings.md
+46 -3
View File
@@ -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\<user>\AppData\Local\Temp\<random>\nested` on Windows) and calls `ResolveRepositoryRoot(isolatedStart)`, asserting an `InvalidOperationException` is thrown. The walker walks every parent — `<random>`, `Temp`, `Local`, `AppData`, `<user>`, `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<InvalidOperationException>` 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 `<isolatedRoot>\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 `<isolatedRoot>\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.