diff --git a/code-reviews/IntegrationTests/findings.md b/code-reviews/IntegrationTests/findings.md index 3cd1efc..57acf30 100644 --- a/code-reviews/IntegrationTests/findings.md +++ b/code-reviews/IntegrationTests/findings.md @@ -424,13 +424,13 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass | Severity | Low | | Category | Code organization & conventions | | Location | `src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs:103-138` (`ResolveRepositoryRoot` / `IsRepositoryRoot`) | -| Status | Open | +| Status | Resolved | **Description:** The walker introduced in `dc9c0c9` searches parents for a directory containing `src/` plus either `.git` or a `*.slnx`/`*.sln` file, falling back to `Directory.GetCurrentDirectory()` when nothing matches. The fallback masks misconfiguration: a test run from a subdirectory of an unpacked tree without a `.git` or `.slnx` marker silently uses the wrong root and then the live `MXGATEWAY_LIVE_MXACCESS_WORKER_EXE` lookup produces a misleading "worker exe not found" error pointing at a fabricated path. The current `bin/` layout makes this unlikely in practice (the test host sets a stable working directory), but the failure mode would be confusing if it triggered. **Recommendation:** Throw a clear `InvalidOperationException` from `ResolveRepositoryRoot` when the walk exhausts without finding a root, naming the directories searched and the markers expected. The opt-in env var (`MXGATEWAY_LIVE_MXACCESS_WORKER_EXE`) remains the escape hatch for unusual deployments. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-05-24 — Replaced the silent `Directory.GetCurrentDirectory()` fallback in `IntegrationTestEnvironment.ResolveRepositoryRoot` with an `InvalidOperationException` whose message names the start directory, the expected markers (`src/`, `.git`, `*.sln`, `*.slnx`), and the `MXGATEWAY_LIVE_MXACCESS_WORKER_EXE` escape hatch. Added regression test `IntegrationTestEnvironmentTests.ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers` that creates an isolated temp directory under `Path.GetTempPath()` containing no markers, asserts the throw, and asserts the message carries the start directory, `.git`, `.sln`, and the worker-exe env-var name so a future refactor that dropped any of these from the diagnostic would fail loudly. TDD red/green confirmed: the new test fails against the pre-fix walker (`Assert.Throws() Failure: No exception was thrown`) and passes after the throw is added. The existing `ResolveRepositoryRoot_AcceptsGitWorktreeFile` test still passes, so the happy path is unaffected. ### IntegrationTests-023 @@ -439,13 +439,13 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass | Severity | Low | | Category | Testing coverage | | Location | `src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs:14-29` | -| Status | Open | +| Status | Resolved | **Description:** `AuthenticateAsync_AdminInGwAdminGroup_Succeeds` asserts the principal carries an `LdapGroupClaimType` claim containing `GwAdmin` (line 26-28). After the `27ed651` refactor, `DashboardAuthenticator.CreatePrincipal` also emits a `ClaimTypes.Role` claim with the mapped role (`Admin` for the seeded `GwAdmin` → `Admin` mapping). The test asserts the LDAP-group claim but not the role claim, so a regression that stopped emitting `Role: Admin` (e.g. a future refactor of `MapGroupsToRoles` that returned an empty list) would silently pass. **Recommendation:** Add an `Assert.Contains` asserting the principal holds a `ClaimTypes.Role` claim with value `DashboardRoles.Admin` (or `"Admin"`). Mirror the style of the existing `LdapGroupClaimType` assertion. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-05-24 — Added an `Assert.Contains` to `AuthenticateAsync_AdminInGwAdminGroup_Succeeds` that mirrors the existing `LdapGroupClaimType` assertion style, asserting the principal carries a `ClaimTypes.Role` claim with value `DashboardRoles.Admin`. Couples the live test to the `GwAdmin` → `Admin` mapping wired through `CreateAuthenticator`'s `GroupToRole` dictionary, so a future regression to `MapGroupsToRoles` (returning an empty list, dropping the RDN-fallback lookup, breaking the case-insensitive comparer) would now surface as a failed live test rather than a silent pass. The test is gated by `MXGATEWAY_RUN_LIVE_LDAP_TESTS`; build is green and the test continues to skip cleanly when the env var is unset. ### IntegrationTests-024 @@ -454,10 +454,10 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass | Severity | Low | | Category | Code organization & conventions | | Location | `src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs` (`NullDashboardEventBroadcaster` private class at end of file) | -| Status | Open | +| Status | Resolved | **Description:** The inline `NullDashboardEventBroadcaster` private class is the third copy in the repository (the other two live in `EventStreamServiceTests` and `GatewayEndToEndFakeWorkerSmokeTests` under the Tests module — see Tests-025). Each carries a singleton `Instance` field and a no-op `Publish`. While the integration-tests project doesn't directly share test-support code with the Tests project, a duplicate-everywhere pattern reads as a code smell. **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:** _(empty until closed)_ +**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). diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs index 207c1c4..166390e 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs @@ -26,6 +26,16 @@ public sealed class DashboardLdapLiveTests Assert.Contains(result.Principal.Claims, claim => claim.Type == DashboardAuthenticationDefaults.LdapGroupClaimType && claim.Value.Contains("GwAdmin", StringComparison.OrdinalIgnoreCase)); + + // IntegrationTests-023: DashboardAuthenticator.CreatePrincipal emits a + // ClaimTypes.Role claim derived from MapGroupsToRoles. The seeded + // GroupToRole map (GwAdmin -> Admin) means the admin principal must + // carry Role=Admin alongside the raw LDAP-group claim. A regression in + // MapGroupsToRoles (returning an empty list, missing the RDN fallback) + // would silently pass without this assertion. + Assert.Contains(result.Principal.Claims, claim => + claim.Type == ClaimTypes.Role + && claim.Value == DashboardRoles.Admin); } [LiveLdapFact] diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs index 5ab53fd..15f67bd 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs @@ -97,9 +97,22 @@ public static class IntegrationTestEnvironment return defaultValue; } - /// Resolves the root directory of the repository by walking parents for a src/ directory next to either a .git marker or a .sln/.slnx file. + /// + /// Resolves the root directory of the repository by walking parents for a + /// src/ directory next to either a .git marker or a + /// .sln/.slnx file. Throws + /// when no root is found so a misconfigured run fails fast with an actionable + /// message rather than silently falling back to the current working directory + /// (which previously produced a misleading "worker exe not found" pointing at + /// a fabricated path — see IntegrationTests-022). The + /// environment variable + /// remains the escape hatch for unusual deployments. + /// /// Starting directory to search from. - /// The repository root path, or the start directory if not found. + /// The repository root path. + /// + /// Thrown when the parent walk exhausts without finding a repository root. + /// internal static string ResolveRepositoryRoot(string startDirectory) { DirectoryInfo? directory = new(startDirectory); @@ -113,7 +126,11 @@ public static class IntegrationTestEnvironment directory = directory.Parent; } - return Directory.GetCurrentDirectory(); + throw new InvalidOperationException( + $"Could not resolve repository root by walking parents of '{startDirectory}'. " + + "Expected to find a directory containing a 'src' subdirectory next to either a '.git' marker " + + "or a '*.sln' / '*.slnx' file in 'src'. " + + $"Set the '{LiveMxAccessWorkerExecutableVariableName}' environment variable to bypass repository-root resolution."); } private static bool IsRepositoryRoot(DirectoryInfo directory) diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs index de66d4e..5cae1dd 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs @@ -45,4 +45,41 @@ public sealed class IntegrationTestEnvironmentTests } } } + + /// + /// Verifies that + /// throws with a diagnostic message when + /// the walk exhausts without finding a repository root. The previous silent + /// fallback to Directory.GetCurrentDirectory() masked misconfiguration + /// (IntegrationTests-022); operators get a clear, actionable failure instead. + /// + [Fact] + public void ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers() + { + string isolatedRoot = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + string isolatedStart = Path.Combine(isolatedRoot, "nested"); + + try + { + Directory.CreateDirectory(isolatedStart); + + InvalidOperationException ex = Assert.Throws( + () => IntegrationTestEnvironment.ResolveRepositoryRoot(isolatedStart)); + + Assert.Contains(isolatedStart, ex.Message, StringComparison.Ordinal); + Assert.Contains(".git", ex.Message, StringComparison.Ordinal); + Assert.Contains(".sln", ex.Message, StringComparison.Ordinal); + Assert.Contains( + IntegrationTestEnvironment.LiveMxAccessWorkerExecutableVariableName, + ex.Message, + StringComparison.Ordinal); + } + finally + { + if (Directory.Exists(isolatedRoot)) + { + Directory.Delete(isolatedRoot, recursive: true); + } + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/TestSupport/NullDashboardEventBroadcaster.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/TestSupport/NullDashboardEventBroadcaster.cs new file mode 100644 index 0000000..f47f773 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/TestSupport/NullDashboardEventBroadcaster.cs @@ -0,0 +1,31 @@ +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; + +namespace ZB.MOM.WW.MxGateway.IntegrationTests.TestSupport; + +/// +/// No-op for integration tests that +/// construct an +/// without exercising the dashboard SignalR fan-out. Singleton because the +/// type holds no state — every consumer can share . +/// The unit-test project owns a parallel copy under +/// ZB.MOM.WW.MxGateway.Tests/TestSupport/; IntegrationTests keeps its +/// own copy here so the two test projects stay independently buildable +/// without a shared test-helpers project (IntegrationTests-024). +/// +public sealed class NullDashboardEventBroadcaster : IDashboardEventBroadcaster +{ + /// Shared no-op instance. + public static readonly NullDashboardEventBroadcaster Instance = new(); + + private NullDashboardEventBroadcaster() + { + } + + /// + public void Publish(string sessionId, MxEvent mxEvent) + { + // Intentionally empty — integration tests assert directly on the gRPC + // stream writer, not on dashboard fan-out. + } +} diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs index 43506aa..01cbc82 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs @@ -15,6 +15,7 @@ using ZB.MOM.WW.MxGateway.Server.Security.Authentication; using ZB.MOM.WW.MxGateway.Server.Security.Authorization; using ZB.MOM.WW.MxGateway.Server.Sessions; using ZB.MOM.WW.MxGateway.Server.Workers; +using ZB.MOM.WW.MxGateway.IntegrationTests.TestSupport; using Xunit.Abstractions; namespace ZB.MOM.WW.MxGateway.IntegrationTests; @@ -1595,9 +1596,4 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) CancellationToken cancellationToken) => Task.CompletedTask; } - private sealed class NullDashboardEventBroadcaster : ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs.IDashboardEventBroadcaster - { - public static readonly NullDashboardEventBroadcaster Instance = new(); - public void Publish(string sessionId, ZB.MOM.WW.MxGateway.Contracts.Proto.MxEvent mxEvent) { } - } }