From 745f4840e6ca53b0b38d1cc51f17012ad734411f Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Tue, 12 May 2026 01:38:42 +0300 Subject: [PATCH] [AZ-493] Cycle 3 batch 3: integration test DB-reset hook AZ-493 (2 SP): replace the cycle-2 wallclock-seeded _coordinateCounter workaround with a proper Postgres state-reset hook that runs at integration test runner startup, eliminating the per-source-unique-index collision risk that the persistent docker-compose Postgres volume introduced post-AZ-484. The reset is split into two surfaces: * SatelliteProvider.TestSupport.IntegrationTestResetGuard - pure static class, I/O-free, unit-tested. Two independent guards: (a) ASPNETCORE_ENVIRONMENT must equal "Testing", (b) DB_CONNECTION_STRING Host must be in the allowed-host list (postgres, localhost, 127.0.0.1). Failure of either guard surfaces a structured operator-friendly InvalidOperationException. * SatelliteProvider.IntegrationTests.IntegrationTestDatabaseReset - instance class owning the Npgsql side effects. Calls the guard then runs TRUNCATE TABLE route_regions, route_points, routes, regions, tiles RESTART IDENTITY CASCADE inside a single Npgsql transaction. Spec-vs-reality: the task spec prescribed "DB name contains _test" as Guard 2; the actual compose file uses Database=satelliteprovider and DB rename is gated on user confirmation per coderule.mdc. Substituted a Host allowlist as the equivalent guard (intent identical: reject remote / production hosts). Recorded as Low/Spec-Gap in the review. Program.cs adds --keep-state CLI flag and INTEGRATION_KEEP_STATE env var (1/true) opt-outs so a developer can inspect leftover state when debugging. Startup banner shows which path executed. docker-compose.tests.yml gets ASPNETCORE_ENVIRONMENT=Testing + passthrough for INTEGRATION_KEEP_STATE. scripts/run-tests.sh wires the --keep-state flag through to compose. UavUploadTests._coordinateCounter wallclock seed is retained as defense-in-depth (per the task spec's implementer choice). The reset is the primary isolation path; the seed is the belt-and-suspenders fallback for --keep-state runs. 8 new unit tests in SatelliteProvider.Tests/TestSupport/ IntegrationTestResetGuardTests.cs cover Production/Staging/missing-env throw, allowed-host case-insensitivity, disallowed-host rejection with representative prod hostnames, and the AllowedHosts contract. tests_integration.md gains a Reliability section that documents the hook, the two guards, the truncate order, and the three opt-out forms. module-layout.md TestSupport entry extended with the new pure guard and the explicit "Npgsql stays in IntegrationTests" boundary. Test-suite gate (AC-6) deferred to Step 16 Final Test Run per implement skill convention. Per-batch review verdict: PASS_WITH_WARNINGS with 1 Low (spec-vs-reality on Guard 2, non-blocking). Co-authored-by: Cursor --- .../IntegrationTestDatabaseReset.cs | 42 ++++++++ SatelliteProvider.IntegrationTests/Program.cs | 28 ++++++ .../UavUploadTests.cs | 5 + .../IntegrationTestResetGuard.cs | 37 +++++++ .../IntegrationTestResetGuardTests.cs | 97 +++++++++++++++++++ _docs/02_document/module-layout.md | 12 ++- .../02_document/modules/tests_integration.md | 40 +++++++- .../AZ-493_integration_test_db_reset_hook.md | 0 .../batch_03_cycle3_report.md | 65 +++++++++++++ .../reviews/batch_03_cycle3_review.md | 54 +++++++++++ docker-compose.tests.yml | 2 + scripts/run-tests.sh | 16 ++- 12 files changed, 385 insertions(+), 13 deletions(-) create mode 100644 SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs create mode 100644 SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs create mode 100644 SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs rename _docs/02_tasks/{todo => done}/AZ-493_integration_test_db_reset_hook.md (100%) create mode 100644 _docs/03_implementation/batch_03_cycle3_report.md create mode 100644 _docs/03_implementation/reviews/batch_03_cycle3_review.md diff --git a/SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs b/SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs new file mode 100644 index 0000000..bf877c3 --- /dev/null +++ b/SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs @@ -0,0 +1,42 @@ +using Npgsql; +using SatelliteProvider.TestSupport; + +namespace SatelliteProvider.IntegrationTests; + +public sealed class IntegrationTestDatabaseReset +{ + public static readonly IReadOnlyList TruncateOrder = + new[] { "route_regions", "route_points", "routes", "regions", "tiles" }; + + private readonly string _connectionString; + + public IntegrationTestDatabaseReset(string connectionString) + { + if (string.IsNullOrWhiteSpace(connectionString)) + { + throw new ArgumentException("Connection string must not be empty.", nameof(connectionString)); + } + _connectionString = connectionString; + } + + public async Task EnsureCleanStateAsync() + { + var environment = Environment.GetEnvironmentVariable(IntegrationTestResetGuard.EnvironmentEnvVar); + var builder = new NpgsqlConnectionStringBuilder(_connectionString); + IntegrationTestResetGuard.EnsureGuardPassesOrThrow(environment, builder.Host); + + await using var connection = new NpgsqlConnection(_connectionString); + await connection.OpenAsync(); + + await using var transaction = await connection.BeginTransactionAsync(); + var truncateSql = $"TRUNCATE TABLE {string.Join(", ", TruncateOrder)} RESTART IDENTITY CASCADE"; + await using (var cmd = new NpgsqlCommand(truncateSql, connection, transaction)) + { + await cmd.ExecuteNonQueryAsync(); + } + await transaction.CommitAsync(); + + Console.WriteLine( + $"✓ Integration test DB reset complete: truncated [{string.Join(", ", TruncateOrder)}] on {builder.Host}/{builder.Database}"); + } +} diff --git a/SatelliteProvider.IntegrationTests/Program.cs b/SatelliteProvider.IntegrationTests/Program.cs index e366d03..688454f 100644 --- a/SatelliteProvider.IntegrationTests/Program.cs +++ b/SatelliteProvider.IntegrationTests/Program.cs @@ -19,6 +19,15 @@ class Program TestRunMode.Smoke = modeEnv == "smoke"; } + // AZ-493: opt-out of the startup DB reset so a developer can inspect leftover state. + var keepStateEnv = Environment.GetEnvironmentVariable("INTEGRATION_KEEP_STATE")?.Trim(); + var keepState = args.Any(a => a.Equals("--keep-state", StringComparison.OrdinalIgnoreCase)) + || string.Equals(keepStateEnv, "1", StringComparison.Ordinal) + || string.Equals(keepStateEnv, "true", StringComparison.OrdinalIgnoreCase); + + var connectionString = Environment.GetEnvironmentVariable("DB_CONNECTION_STRING") + ?? "Host=postgres;Port=5432;Database=satelliteprovider;Username=postgres;Password=postgres"; + string jwtSecret; try { @@ -35,6 +44,7 @@ class Program Console.WriteLine("========================="); Console.WriteLine($"API URL : {apiUrl}"); Console.WriteLine($"Mode : {(TestRunMode.Smoke ? "smoke (fast subset, tightened timeouts)" : "full")}"); + Console.WriteLine($"State : {(keepState ? "keep (DB reset skipped)" : "reset (clean DB at startup, AZ-493)")}"); Console.WriteLine($"Auth : JWT_SECRET resolved ({System.Text.Encoding.UTF8.GetByteCount(jwtSecret)} bytes)"); Console.WriteLine(); @@ -52,6 +62,24 @@ class Program Console.WriteLine("Waiting for API to be ready..."); await WaitForApiReady(httpClient); Console.WriteLine("✓ API is ready"); + + if (keepState) + { + Console.WriteLine("⚠ --keep-state / INTEGRATION_KEEP_STATE set; skipping DB reset (AZ-493)"); + } + else + { + try + { + await new IntegrationTestDatabaseReset(connectionString).EnsureCleanStateAsync(); + } + catch (InvalidOperationException ex) + { + Console.WriteLine($"❌ DB reset refused: {ex.Message}"); + return 1; + } + } + Console.WriteLine(); await JwtIntegrationTests.RunAll(apiUrl, jwtSecret); diff --git a/SatelliteProvider.IntegrationTests/UavUploadTests.cs b/SatelliteProvider.IntegrationTests/UavUploadTests.cs index bcc98c1..e69bbec 100644 --- a/SatelliteProvider.IntegrationTests/UavUploadTests.cs +++ b/SatelliteProvider.IntegrationTests/UavUploadTests.cs @@ -356,6 +356,11 @@ public static class UavUploadTests // (named volume); a monotonic counter from 0 would collide with prior runs on // the per-source unique index, especially for tests that seed rows via raw // INSERT rather than the API's UPSERT path. + // Kept as defense-in-depth after AZ-493 introduced a Program.cs startup DB + // reset. If the reset is skipped via --keep-state OR fails silently, the + // wallclock seed still spreads coordinates across runs so the per-source + // unique index does not collide. Safe to remove if/when the DB-reset path + // becomes load-bearing for every run. private static int _coordinateCounter = (int)((DateTime.UtcNow.Ticks / TimeSpan.TicksPerSecond) % 1_000_000); private static (double Latitude, double Longitude) NextTestCoordinate() diff --git a/SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs b/SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs new file mode 100644 index 0000000..f2b82f0 --- /dev/null +++ b/SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs @@ -0,0 +1,37 @@ +namespace SatelliteProvider.TestSupport; + +public static class IntegrationTestResetGuard +{ + public const string EnvironmentEnvVar = "ASPNETCORE_ENVIRONMENT"; + public const string TestingEnvironment = "Testing"; + + public static readonly IReadOnlyList AllowedHosts = + new[] { "postgres", "localhost", "127.0.0.1" }; + + public static void EnsureGuardPassesOrThrow(string? environment, string? host) + { + if (!string.Equals(environment, TestingEnvironment, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException( + $"Integration test DB reset refused: {EnvironmentEnvVar} is '{environment ?? "(not set)"}', " + + $"expected '{TestingEnvironment}'. This guard prevents accidental truncate against a non-test " + + "database (production, staging, or developer-loop DB)."); + } + + if (string.IsNullOrWhiteSpace(host)) + { + throw new InvalidOperationException( + "Integration test DB reset refused: connection string has no Host. " + + "Ensure DB_CONNECTION_STRING is set with Host=postgres (docker-compose) or Host=localhost (developer machine)."); + } + + var hostMatch = AllowedHosts.Any(allowed => string.Equals(allowed, host, StringComparison.OrdinalIgnoreCase)); + if (!hostMatch) + { + throw new InvalidOperationException( + $"Integration test DB reset refused: Host '{host}' is not in the allowed-host list " + + $"({string.Join(", ", AllowedHosts)}). This guard prevents accidental truncate against a remote " + + "database whose hostname suggests production or staging."); + } + } +} diff --git a/SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs b/SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs new file mode 100644 index 0000000..83dc2ee --- /dev/null +++ b/SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs @@ -0,0 +1,97 @@ +using FluentAssertions; +using SatelliteProvider.TestSupport; + +namespace SatelliteProvider.Tests.TestSupport; + +public class IntegrationTestResetGuardTests +{ + [Fact] + public void EnsureGuardPassesOrThrow_ProductionEnvironment_Throws() + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Production", "postgres"); + + // Assert + act.Should().Throw() + .WithMessage("*ASPNETCORE_ENVIRONMENT*Production*expected*Testing*"); + } + + [Fact] + public void EnsureGuardPassesOrThrow_StagingEnvironment_Throws() + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Staging", "postgres"); + + // Assert + act.Should().Throw() + .WithMessage("*Staging*expected*Testing*"); + } + + [Fact] + public void EnsureGuardPassesOrThrow_MissingEnvironment_Throws() + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow(null, "postgres"); + + // Assert + act.Should().Throw() + .WithMessage("*not set*expected*Testing*"); + } + + [Fact] + public void EnsureGuardPassesOrThrow_TestingEnvironment_AllowedHost_DoesNotThrow() + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Testing", "postgres"); + + // Assert + act.Should().NotThrow(); + } + + [Theory] + [InlineData("postgres")] + [InlineData("Postgres")] + [InlineData("POSTGRES")] + [InlineData("localhost")] + [InlineData("127.0.0.1")] + public void EnsureGuardPassesOrThrow_TestingEnvironment_AcceptsAllowedHosts(string host) + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Testing", host); + + // Assert + act.Should().NotThrow(); + } + + [Theory] + [InlineData("prod.example.com")] + [InlineData("rds.amazonaws.com")] + [InlineData("db.staging.internal")] + public void EnsureGuardPassesOrThrow_TestingEnvironment_RejectsDisallowedHosts(string host) + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Testing", host); + + // Assert + act.Should().Throw() + .WithMessage($"*Host '{host}' is not in the allowed-host list*"); + } + + [Fact] + public void EnsureGuardPassesOrThrow_TestingEnvironment_MissingHost_Throws() + { + // Act + var act = () => IntegrationTestResetGuard.EnsureGuardPassesOrThrow("Testing", null); + + // Assert + act.Should().Throw() + .WithMessage("*connection string has no Host*"); + } + + [Fact] + public void AllowedHosts_IsImmutableContract() + { + // Assert + IntegrationTestResetGuard.AllowedHosts.Should().BeEquivalentTo(new[] { "postgres", "localhost", "127.0.0.1" }); + } +} diff --git a/_docs/02_document/module-layout.md b/_docs/02_document/module-layout.md index 8719145..77916a3 100644 --- a/_docs/02_document/module-layout.md +++ b/_docs/02_document/module-layout.md @@ -164,16 +164,18 @@ The cycle-1 (AZ-487) and cycle-2 (AZ-488) code reviews each surfaced an F1 (Low - **Consumed by**: DataAccess (entity defaults, type handler registration), TileDownloader (sets `TileEntity.Source` via `TileSourceConverter.ToWireValue`), Tests - **Important constraint**: Dapper's `SqlMapper.TypeHandler` is bypassed for enum reads (Dapper issue #259 — see `_docs/LESSONS.md` L-001). For any new enum that must round-trip through a database column, prefer the `string`-on-entity + `Enum`-at-API-boundary pattern with a converter class in this folder. Do NOT register a `TypeHandler` and assume it will be honored on reads. -### TestSupport (added by AZ-491) +### TestSupport (added by AZ-491; extended by AZ-493) - **Directory**: `SatelliteProvider.TestSupport/` - **csproj**: `SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj` (class library, no test framework) -- **Purpose**: Canonical home for cross-project test utilities. Currently holds `JwtTokenFactory` (HS256 token minting + signature tampering). Replaces the cycle-2 duplicate that lived in both `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` and required parallel fixes. Future additions: shared image-fixture factories, shared deterministic clocks / test-data builders that need to be visible to both unit and integration projects. -- **Public API**: `SatelliteProvider.TestSupport/JwtTokenFactory.cs` (`Create`, `CreateExpired`, `TamperSignature`) -- **PackageReferences**: `Microsoft.IdentityModel.Tokens` 7.0.3, `System.IdentityModel.Tokens.Jwt` 7.0.3 (matches the integration tests' pre-AZ-491 explicit reference). +- **Purpose**: Canonical home for cross-project test utilities. Currently holds `JwtTokenFactory` (HS256 token minting + signature tampering) and `IntegrationTestResetGuard` (pure-string guard for the integration-test DB-reset hook). Replaces the cycle-2 duplicate that lived in both `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` and required parallel fixes. Future additions: shared image-fixture factories, shared deterministic clocks / test-data builders that need to be visible to both unit and integration projects. +- **Public API**: + - `SatelliteProvider.TestSupport/JwtTokenFactory.cs` (`Create`, `CreateExpired`, `TamperSignature`) — added by AZ-491. + - `SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs` (`EnsureGuardPassesOrThrow`, `AllowedHosts`, `EnvironmentEnvVar`, `TestingEnvironment`) — added by AZ-493. Pure static class — no I/O, no DB calls. Consumed by `SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs` (instance class that owns the Npgsql side effects) and unit-tested in `SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs`. +- **PackageReferences**: `Microsoft.IdentityModel.Tokens` 7.0.3, `System.IdentityModel.Tokens.Jwt` 7.0.3 (matches the integration tests' pre-AZ-491 explicit reference). The AZ-493 guard introduced no new package dependencies — it is pure string comparison over the BCL. - **Consumed by**: `SatelliteProvider.Tests`, `SatelliteProvider.IntegrationTests` (both via `ProjectReference`). - **Not consumed by**: production projects (`Api`, `Common`, `DataAccess`, `Services.*`). The TestSupport library is test-only by design; production code must NOT depend on it. -- **Runner-side concerns NOT in TestSupport**: `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` retains `ResolveSecretOrThrow`, `AttachDefaultAuthorization`, and the `DefaultSubject = "integration-tests"` constant — these are runner-specific (env-var reads, `HttpClient` mutation, runner-identity subject) and intentionally not consolidated. +- **Runner-side concerns NOT in TestSupport**: `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` retains `ResolveSecretOrThrow`, `AttachDefaultAuthorization`, and the `DefaultSubject = "integration-tests"` constant — these are runner-specific (env-var reads, `HttpClient` mutation, runner-identity subject) and intentionally not consolidated. `SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs` (AZ-493) holds the Npgsql side effects of the reset — it sits in the integration-tests project (not TestSupport) so the Npgsql dependency doesn't leak into unit tests. ## Allowed Dependencies (layering) diff --git a/_docs/02_document/modules/tests_integration.md b/_docs/02_document/modules/tests_integration.md index f37e28e..4244ea7 100644 --- a/_docs/02_document/modules/tests_integration.md +++ b/_docs/02_document/modules/tests_integration.md @@ -13,13 +13,15 @@ Console application that runs end-to-end integration tests against a live API in - `ExtendedRouteTests` — routes with `requestMaps: true` and tile ZIP creation - `MigrationTests` — direct PostgreSQL schema/index validation (no HTTP). AZ-484 cycle added: `NewUniqueConstraintIncludesSourceColumn_AZ484_AC1`, `BackfillUpdateAssignsGoogleMapsAndCapturedAt_AZ484_AC4`, `MultiSourceInsertCoexistsUnderNewIndex_AZ484_AC1`, `MostRecentAcrossSourcesSelection_AZ484_AC2`, `SameSourceUpsertReplacesPreviousRow_AZ484_AC3` (latter four use temp tables to keep production data untouched). - `JwtIntegrationTests` (added by AZ-487, cycle 2; helpers consolidated by AZ-491 cycle 3) — `AnonymousRequest_To_AnyEndpoint_Returns401`, `ExpiredToken_Returns401`, `InvalidSignature_Returns401`, `ValidToken_Returns200_OnHealthyEndpoint`, `SwaggerDocument_AdvertisesBearerSecurityScheme`. HS256 token minting lives in the shared `SatelliteProvider.TestSupport.JwtTokenFactory` (consumed via `ProjectReference`); runner-specific concerns (`JwtTestHelpers.ResolveSecretOrThrow`, `AttachDefaultAuthorization`, `DefaultSubject = "integration-tests"`) remain in this project. The test runner sets `JWT_SECRET` on the API container and attaches a Bearer token to every existing test's HTTP requests so the pre-cycle-2 suite continues to pass. -- `UavUploadTests` (added by AZ-488, cycle 2) — `HappyPathSingleItem_PersistsRow`, `MixedBatch_ReturnsPerItemResults`, `MultiSourceCoexistence_AZ484_Cycle2`, `SameSourceUpsert_AZ484_Cycle2`, `NoToken_Returns401`, `ValidTokenWithoutGpsPermission_Returns403`, `OversizedBatch_Returns400`. Uses a wall-clock-seeded coordinate counter (`_coordinateCounter` initialized from `DateTime.UtcNow`) so each docker-compose run picks a fresh coordinate band — the postgres named volume persists across runs and a naïve `int = 0` counter collided with prior runs on the per-source unique index (fixed mid-Step-11). +- `UavUploadTests` (added by AZ-488, cycle 2; coordinate-counter promoted to defense-in-depth by AZ-493 cycle 3) — `HappyPathSingleItem_PersistsRow`, `MixedBatch_ReturnsPerItemResults`, `MultiSourceCoexistence_AZ484_Cycle2`, `SameSourceUpsert_AZ484_Cycle2`, `NoToken_Returns401`, `ValidTokenWithoutGpsPermission_Returns403`, `OversizedBatch_Returns400`. The wall-clock-seeded `_coordinateCounter` is retained as a belt-and-suspenders safeguard alongside the AZ-493 startup DB-reset (below) — if a developer runs with `--keep-state`, or the DB-reset path is skipped for any reason, the wall-clock seed still spreads coordinates across runs so the per-source unique index does not collide. - `StubAndErrorContractTests` (existing) — updated in cycle 2 to drop the legacy `StubUpload_Returns501` expectation since AZ-488 implemented the endpoint. ### Supporting Classes - `Models.cs` — HTTP response DTOs for deserialization - `RouteTestHelpers.cs` — shared utilities (wait-for-completion polling, geofence polygon builders, test data) -- `Program.cs` — test runner entry point +- `Program.cs` — test runner entry point (handles `--smoke` / `--full` mode selection, `--keep-state` opt-out flag, default-token issuance via `JwtTokenFactory`, and the AZ-493 DB-reset hook) +- `JwtTestHelpers.cs` — runner-side JWT concerns (`ResolveSecretOrThrow` reads the `JWT_SECRET` env var with size validation; `AttachDefaultAuthorization` puts a Bearer token on the shared `HttpClient`; `DefaultSubject = "integration-tests"` is the canonical runner subject value). Token *minting* lives in the shared `SatelliteProvider.TestSupport.JwtTokenFactory` (AZ-491) — runner-side concerns deliberately stay here. +- `IntegrationTestDatabaseReset.cs` (AZ-493) — instance class with a single `EnsureCleanStateAsync()` method that truncates the integration-test target tables in FK-safe order. Guarded via `SatelliteProvider.TestSupport.IntegrationTestResetGuard` (env + Host allowlist) so it cannot run against a non-test database. ## Internal Logic - Makes HTTP calls to the API at `API_URL` environment variable (default: `http://api:8080`) @@ -28,15 +30,43 @@ Console application that runs end-to-end integration tests against a live API in - Validates response structure, status transitions, file creation ## Dependencies -- No project references (standalone console app) -- Communicates with the API exclusively via HTTP -- NuGet: implicit .NET 8 runtime +- `ProjectReference` to `SatelliteProvider.TestSupport` (added by AZ-491; provides `JwtTokenFactory`. Added by AZ-493; provides `IntegrationTestResetGuard`). +- Communicates with the API exclusively via HTTP for end-to-end tests; communicates with PostgreSQL directly only via the dedicated DB-reset hook + the existing `MigrationTests` schema assertions. +- NuGet: `Npgsql` 9.0.2 (Postgres client for DB-reset + MigrationTests), `SixLabors.ImageSharp` 3.1.11 (UAV fixture image generation). ## Consumers - `docker-compose.tests.yml` — runs as a container that depends on the API service ## Configuration - `API_URL` environment variable (set in docker-compose.tests.yml to `http://api:8080`) +- `INTEGRATION_TESTS_MODE` — `smoke` or `full` (default `full`). Drives `TestRunMode.Smoke`. +- `INTEGRATION_KEEP_STATE` — set to `1` or `true` (or pass `--keep-state` to `Program.cs` / `scripts/run-tests.sh`) to skip the AZ-493 DB-reset hook. Useful for debugging a failed run. +- `ASPNETCORE_ENVIRONMENT=Testing` — guard for the DB-reset hook. The reset refuses to run unless this is set (see Reliability § Test isolation below). +- `JWT_SECRET` — shared HMAC secret with the API container; must be ≥ 32 bytes (UTF-8). +- `DB_CONNECTION_STRING` — Npgsql connection string; the reset hook additionally requires the Host to be in the allowed-host list (`postgres`, `localhost`, `127.0.0.1`). + +## Reliability + +### Test isolation (AZ-493) + +`Program.cs` runs `IntegrationTestDatabaseReset.EnsureCleanStateAsync()` at startup, before any test class executes. The hook truncates `route_regions`, `route_points`, `routes`, `regions`, `tiles` (in that FK-safe order, with `RESTART IDENTITY CASCADE`) so each run starts from a known empty state. The Postgres named volume in `docker-compose.yml` is intentionally persisted across `docker-compose down` cycles for fast iteration; the AZ-493 reset hook is what gives back per-run isolation in spite of that. + +Two guards protect against accidental truncate against a non-test database: + +1. `ASPNETCORE_ENVIRONMENT` MUST equal `Testing` (case-insensitive). Set by `docker-compose.tests.yml`; absent in production / dev. +2. `DB_CONNECTION_STRING` Host MUST be one of `postgres`, `localhost`, `127.0.0.1`. Set by `docker-compose.tests.yml` and developer machines; a remote-host connection string is rejected even with the env guard satisfied. + +Both guards are pure-string checks in `SatelliteProvider.TestSupport.IntegrationTestResetGuard` — unit-tested in `SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs`. Failure of either guard surfaces a clear `InvalidOperationException` and exits the runner with code 1. + +To debug leftover state from a failed run, opt out of the reset: + +- CLI: `./scripts/run-tests.sh --full --keep-state` +- Direct: `INTEGRATION_KEEP_STATE=1 docker compose ... up` +- In the runner Main: `dotnet run --project SatelliteProvider.IntegrationTests -- --keep-state` + +### Adding new tables + +If a new task adds a table that integration tests insert into AND that table participates in foreign-key relationships with `tiles` / `regions` / `routes`, update `IntegrationTestDatabaseReset.TruncateOrder` to include the new table in FK-safe order. The current order assumes the AZ-484 + AZ-488 schema; future migrations that introduce new FK chains need a corresponding order revision. The `CASCADE` clause is a safety net but is not a substitute for an explicit order — the order is the audit trail for "what does an integration-test runner see at startup". ## External Integrations - HTTP to the SatelliteProvider API diff --git a/_docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md b/_docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md similarity index 100% rename from _docs/02_tasks/todo/AZ-493_integration_test_db_reset_hook.md rename to _docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md diff --git a/_docs/03_implementation/batch_03_cycle3_report.md b/_docs/03_implementation/batch_03_cycle3_report.md new file mode 100644 index 0000000..5dcc7ba --- /dev/null +++ b/_docs/03_implementation/batch_03_cycle3_report.md @@ -0,0 +1,65 @@ +# Batch Report — Batch 03 cycle 3 + +**Batch**: 03 (cycle 3) +**Tasks**: AZ-493 (integration test DB-reset hook) +**Date**: 2026-05-12 + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|---------------|-------|-------------|--------| +| AZ-493_integration_test_db_reset_hook | Done | 3 added + 5 modified | 8 new unit tests for the guard (in `SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs`); reset itself exercised by the existing integration suite at Step 16 | 6/6 ACs covered | 0 blockers; 1 spec-vs-reality note (see below) | + +## AC Test Coverage: All covered (6 of 6) +## Code Review Verdict: pending (this batch report precedes per-batch review) +## Auto-Fix Attempts: 0 +## Stuck Agents: None + +## What was implemented + +The reset is split into two parts so the guard logic is pure-string and unit-testable while the actual DB side effects live in the integration-tests project (which already depends on Npgsql). + +### Added + +- `SatelliteProvider.TestSupport/IntegrationTestResetGuard.cs` (pure static class). Validates `(environment, host)` against two rules: (a) `ASPNETCORE_ENVIRONMENT` MUST equal `"Testing"` (case-insensitive); (b) `Host` MUST be one of `postgres`, `localhost`, `127.0.0.1`. Throws `InvalidOperationException` with structured operator-friendly messages on either failure. The class is intentionally I/O-free so unit tests don't need Postgres. +- `SatelliteProvider.IntegrationTests/IntegrationTestDatabaseReset.cs` (instance class). Constructor takes a connection string; `EnsureCleanStateAsync()` calls the guard then runs `TRUNCATE TABLE route_regions, route_points, routes, regions, tiles RESTART IDENTITY CASCADE` inside an Npgsql transaction. FK-safe order is preserved as both the SQL argument list AND the public `TruncateOrder` `IReadOnlyList`. Structured success log includes which tables were truncated and the Host/Database tuple so operators have an audit trail. +- `SatelliteProvider.Tests/TestSupport/IntegrationTestResetGuardTests.cs` (8 unit tests). Covers: Production / Staging / missing-environment all throw (AC-4); allowed hosts (`postgres`, `Postgres`, `POSTGRES`, `localhost`, `127.0.0.1`) all pass with case-insensitivity; disallowed hosts (`prod.example.com`, `rds.amazonaws.com`, `db.staging.internal`) all throw with the specific host name in the message; missing host throws; the `AllowedHosts` contract is asserted as an immutable expectation. + +### Modified + +- `SatelliteProvider.IntegrationTests/Program.cs`: + - Argument parsing now recognizes `--keep-state` (CLI) and `INTEGRATION_KEEP_STATE=1` / `INTEGRATION_KEEP_STATE=true` (env var). + - Reads `DB_CONNECTION_STRING` up front (alongside the existing `API_URL` read) so the reset and the test classes share the same connection string. + - After the API readiness probe and before any test class runs, calls `await new IntegrationTestDatabaseReset(connectionString).EnsureCleanStateAsync()`. Exits with code 1 if the guard throws (with the structured error message visible to the operator). + - Startup banner gains a `State : reset (clean DB at startup, AZ-493)` or `State : keep (DB reset skipped)` line so the run log makes it unambiguous which path executed. +- `SatelliteProvider.IntegrationTests/UavUploadTests.cs` — `_coordinateCounter` wallclock seed retained as **defense-in-depth** (per the AZ-493 task spec's implementer choice). An inline comment back-references AZ-493 and explains that the reset hook is the primary isolation path; the wallclock seed is the belt to the suspenders so `--keep-state` runs don't immediately collide on the per-source unique index. +- `docker-compose.tests.yml` — `integration-tests` service gains `ASPNETCORE_ENVIRONMENT=Testing` (the AZ-493 Guard 1) and `INTEGRATION_KEEP_STATE=${INTEGRATION_KEEP_STATE:-}` (developer-overridable env var). API service is unchanged. +- `scripts/run-tests.sh` — `--keep-state` flag added with usage docs; passes `INTEGRATION_KEEP_STATE=1` through to the compose run when set. +- `_docs/02_document/modules/tests_integration.md` — new `## Reliability` section documents the AZ-493 hook, the two guards, the truncate order, and the three opt-out forms (`./scripts/run-tests.sh --keep-state`, `INTEGRATION_KEEP_STATE=1`, `dotnet run ... -- --keep-state`). The UavUploadTests entry now flags the coordinate-counter wallclock seed as "promoted to defense-in-depth by AZ-493 cycle 3". The `## Dependencies` section now lists the ProjectReference to `SatelliteProvider.TestSupport` and the actual NuGet references (Npgsql 9.0.2, ImageSharp 3.1.11) instead of the previous "No project references" line. +- `_docs/02_document/module-layout.md` § TestSupport — extended with the `IntegrationTestResetGuard` entry, the "no new packages" note, and an explicit boundary: AZ-493's Npgsql-bearing reset class lives in `SatelliteProvider.IntegrationTests` (not TestSupport) so the Npgsql dependency doesn't leak into unit tests. + +## AC Verification + +| AC | Status | Evidence | +|----|--------|----------| +| AC-1: Empty-state on startup | ✓ | `TRUNCATE ... RESTART IDENTITY CASCADE` against `route_regions, route_points, routes, regions, tiles` runs in a single Npgsql transaction before any test class is reached | +| AC-2: Wallclock workaround no longer needed for back-to-back runs | ✓ (with belt-and-suspenders) | Reset hook ensures empty start. Wallclock seed retained as defense-in-depth per implementer choice — does not weaken AC-2 since the primary isolation mechanism is the reset, not the seed | +| AC-3: Opt-out preserves state | ✓ | `--keep-state` (CLI) and `INTEGRATION_KEEP_STATE` (env) both wired through `scripts/run-tests.sh` → docker-compose → Program.cs; startup banner shows which path executed | +| AC-4: Reset only fires in test environment | ✓ | 8 unit tests in `IntegrationTestResetGuardTests` cover Production/Staging/missing-env → throw; 3 disallowed-host examples → throw; allowed-host case-insensitivity → pass | +| AC-5: Documentation reflects new convention | ✓ | tests_integration.md `## Reliability` section is canonical; module-layout.md cross-references the guard's pure-vs-side-effectful split | +| AC-6: Existing tests pass unchanged | Deferred to Step 16 | All pre-AZ-493 test logic untouched; only Program.cs startup wiring + the new reset class were added | + +## Spec-vs-reality notes + +- **Guard 2 deviation from task spec**: The task spec § Safety prescribed "DB name contains `_test`" as the second guard. However, the existing test compose file uses `Database=satelliteprovider` — *not* `satelliteprovider_test`. Renaming the database is gated on user confirmation per `coderule.mdc`. The "or equivalent" qualifier in the spec was used to substitute a **Host allowlist** (`postgres`, `localhost`, `127.0.0.1`) as the second guard. The intent is identical (refuse to truncate against a remote / production host) and the implementation is unit-tested with a representative production-shape host list (`prod.example.com`, `rds.amazonaws.com`, `db.staging.internal`) all hitting the rejection path. +- This is being recorded as a Low / Spec-Gap finding in the code-review report, mirroring the AZ-496 pattern (spec inaccuracy detected during implementation, documented and worked around without renaming production assets). + +## Open follow-ups (non-blocking) + +- **Test-DB rename decision** (optional future PBI): if the team wants to align with the spec's preferred two-guard model (`ASPNETCORE_ENVIRONMENT=Testing` + DB name contains `_test`), the integration-tests compose `Database=satelliteprovider` would become `Database=satelliteprovider_test`. This requires a DB rename + migration-runner verification + at least one round of integration-suite re-verification. Out of scope for AZ-493. +- **Reset performance under load**: AZ-493 NFR sets a < 1 s budget for `TRUNCATE` against an O(10K)-row DB. The current cycle's tile-table row count is far below that threshold; no measurement is in scope. A future cycle that exercises higher-volume scenarios should validate the NFR. +- **Truncate-order audit on schema changes**: noted in tests_integration.md § Reliability — new tables with FK relationships to `tiles`/`regions`/`routes` need a `TruncateOrder` revision. Add to the decompose-skill review checklist if the pattern recurs (suggested but not in scope for AZ-493). + +## Next Batch: AZ-492 (Perf harness PT-07 + PT-08 + JWT-attach) + +AZ-492 is 3 SP. Promotes the two deferred performance NFRs (PT-07: P95 latency-vs-load; PT-08: error-rate-under-pressure) into runnable scenarios under `scripts/run-performance-tests.sh`, and fixes the cycle-2-regressed JWT-attach so every perf scenario sends a valid Bearer token (post-AZ-487 the endpoints are protected, so every perf request currently 401s). With AZ-491 done, AZ-492 can consume `SatelliteProvider.TestSupport.JwtTokenFactory` indirectly via a small shell-side `python3 -c` minter that mirrors its parameters (or by shelling out to a tiny dotnet entry point — implementer's choice at batch start). diff --git a/_docs/03_implementation/reviews/batch_03_cycle3_review.md b/_docs/03_implementation/reviews/batch_03_cycle3_review.md new file mode 100644 index 0000000..694dfb9 --- /dev/null +++ b/_docs/03_implementation/reviews/batch_03_cycle3_review.md @@ -0,0 +1,54 @@ +# Code Review Report — Batch 03 cycle 3 + +**Batch**: 03 (cycle 3) — AZ-493 (integration test DB-reset hook) +**Date**: 2026-05-12 +**Verdict**: PASS_WITH_WARNINGS + +## Findings + +| # | Severity | Category | File:Line | Title | +|---|----------|----------|-----------|-------| +| 1 | Low | Spec-Gap | `_docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md` § Safety + § Risk 1 | Task spec prescribed "DB name contains `_test`" as Guard 2; production reality uses `Database=satelliteprovider` and rename requires user confirmation | + +### Finding Details + +**F1: Task spec prescribed "DB name contains `_test`" as Guard 2; production reality uses `Database=satelliteprovider`** (Low / Spec-Gap) + +- Location: `_docs/02_tasks/done/AZ-493_integration_test_db_reset_hook.md` § Safety (line ~98) and § Risk 1 (line ~125) +- Description: The AZ-493 spec asserted "the integration test compose file already uses a separate DB" and prescribed the second guard as "the DB name contains `_test`". Reading `docker-compose.tests.yml` showed the integration tests use the same `Database=satelliteprovider` as the API service. Renaming the database is gated on user confirmation per `coderule.mdc` ("Do not rename any databases or tables or table columns without confirmation."). The spec's "or equivalent" qualifier was used to substitute a Host allowlist (`postgres`, `localhost`, `127.0.0.1`) as Guard 2. +- Suggestion: Capture this in the batch report (already done — Spec-vs-reality notes section) and consider a future PBI to align on the spec's preferred guard pattern *if* the team wants `_test` in the DB name. The current implementation provides identical safety (remote / production hostnames are rejected) and is unit-tested. No code action required. Pattern matches cycle-3 batch-1 F1 (AZ-496 Tests.csproj non-existence) — a recurring symptom of task specs encoding assumptions that aren't verified against the actual codebase before authoring. +- Task: AZ-493 + +## Phase-by-Phase Summary + +| Phase | Result | Notes | +|-------|--------|-------| +| 1. Context Loading | OK | AZ-493 spec read; 6 ACs identified; defense-in-depth choice on `_coordinateCounter` documented before implementation | +| 2. Spec Compliance | OK | 6/6 ACs verified at code level. AC-6 (existing tests pass unchanged) waits on Step 16 final test gate; the structural prerequisite (Program.cs startup path is the only diff) is satisfied | +| 3. Code Quality | OK | Clean separation of pure guard logic (`IntegrationTestResetGuard` — static, I/O-free, easy to test) from side-effectful reset (`IntegrationTestDatabaseReset` — instance, owns the Npgsql connection). Single responsibility per class. Errors surface with structured operator-friendly messages. No silent suppression. Test class has 8 well-isolated `[Fact]` / `[Theory]` cases following Arrange/Act/Assert. | +| 4. Security Quick-Scan | OK | The guard's express purpose is preventing accidental truncate against non-test databases. Two independent checks (env sentinel + Host allowlist) — either alone would have prevented the cycle-2 scenario; together they reduce the false-positive risk to near zero. Unit tests confirm the rejection paths with representative production-shape hostnames | +| 5. Performance Scan | OK | Reset is O(table-count) Npgsql round-trips wrapped in a single transaction; AZ-493 NFR budget (< 1 s on O(10K) rows) is comfortably within Postgres TRUNCATE behavior | +| 6. Cross-Task Consistency | OK | The new `IntegrationTestResetGuard` lives in TestSupport alongside the AZ-491 `JwtTokenFactory` — both are pure utility surfaces consumed by both test projects. The boundary is enforced: `IntegrationTestDatabaseReset` (Npgsql-bearing) stays in IntegrationTests so the Npgsql dependency does not leak into unit tests. This explicit separation is the architectural improvement the AZ-491 batch teed up | +| 7. Architecture Compliance | OK | No production-component change. TestSupport's role as a shared/cross-cutting test library is reinforced (now serves two cycle-3 PBIs). No new ProjectReferences in production code. No cycles. Layering invariant preserved: TestSupport is *below* both test projects; production projects do NOT reference TestSupport | + +## Cross-cutting observations (info, no finding) + +- The guard's `AllowedHosts` is a constant list, not a configurable env var. This is intentional — the safety guarantee weakens if operators can override it. A future PBI that needs to support a non-listed Host (e.g., a Kubernetes pod hostname for ephemeral test environments) would extend the list in source, not via runtime config. +- The decision to retain the wallclock-seeded `_coordinateCounter` in `UavUploadTests` is recorded as defense-in-depth. AC-2 explicitly allowed either approach; the batch chose retention because the additional code surface is minimal and the safety benefit (against `--keep-state` collisions) is non-trivial. The retention comment back-references AZ-493 so future agents understand why the cycle-2 workaround is still there. + +## Baseline Delta + +| Class | Count | Notes | +|-------|-------|-------| +| Carried over | 0 | No Architecture findings; nothing recurring | +| Resolved | 1 (informal) | The cycle-2 Pattern 5 (Integration test state leakage from persistent Postgres volume) is structurally closed by this batch. Tracked in `retro_2026-05-11_cycle2.md` § 6 Pattern 5; not an Architecture-class baseline entry, so it does not appear in the cycle-1 baseline | +| Newly introduced | 0 | — | + +## Verdict Logic + +- 0 Critical, 0 High, 0 Medium, 1 Low → **PASS_WITH_WARNINGS** +- The Low finding is the spec-vs-reality observation about Guard 2 — documented, mitigated, and unit-tested. Does not block commit per the implement-skill auto-fix gate. + +## Recommendation to /implement + +Proceed to commit + push + tracker transition (Steps 11-13). diff --git a/docker-compose.tests.yml b/docker-compose.tests.yml index 7b54e09..711ee99 100644 --- a/docker-compose.tests.yml +++ b/docker-compose.tests.yml @@ -17,6 +17,8 @@ services: environment: - API_URL=http://api:8080 - INTEGRATION_TESTS_MODE=${INTEGRATION_TESTS_MODE:-full} + - INTEGRATION_KEEP_STATE=${INTEGRATION_KEEP_STATE:-} + - ASPNETCORE_ENVIRONMENT=Testing - DB_CONNECTION_STRING=Host=postgres;Port=5432;Database=satelliteprovider;Username=postgres;Password=postgres - JWT_SECRET=${JWT_SECRET} volumes: diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh index 1ce2fcc..eabe090 100755 --- a/scripts/run-tests.sh +++ b/scripts/run-tests.sh @@ -11,7 +11,7 @@ trap cleanup EXIT usage() { cat <