mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 12:41:14 +00:00
[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 <cursoragent@cursor.com>
This commit is contained in:
@@ -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).
|
||||
Reference in New Issue
Block a user