mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 14:11:15 +00:00
[AZ-491] Cycle 3 batch 2: consolidate JWT test-mint helpers into TestSupport
AZ-491 (3 SP): eliminate the cycle-2 duplicate of JWT-minting logic that existed in both SatelliteProvider.Tests/TestUtilities/ JwtTokenFactory.cs (unit-side) and SatelliteProvider.IntegrationTests/ JwtTestHelpers.cs (integration-side), where the same Expires < NotBefore bug needed parallel fixes in commitsf64d0d7+11b7074. Option A chosen: new SatelliteProvider.TestSupport class library (no test framework) holds the canonical JwtTokenFactory.Create / CreateExpired / TamperSignature. Both Tests and IntegrationTests consume it via ProjectReference; production projects (Api, Common, DataAccess, Services.*) cannot depend on it. The notBefore-shift workaround is preserved with an inline regression-prevention comment back-referencing the cycle-2 fix commits. SatelliteProvider.IntegrationTests/JwtTestHelpers.cs is stripped to runner-only concerns: ResolveSecretOrThrow, AttachDefaultAuthorization, and the DefaultSubject = "integration-tests" constant. Call sites in Program.cs, JwtIntegrationTests.cs, and UavUploadTests.cs (10 sites) switched to JwtTokenFactory.* with JwtTestHelpers.DefaultSubject explicitly passed for the runner subject - behavior parity preserved. Dockerfile for IntegrationTests gets the new TestSupport csproj in its pre-restore COPY layer. Api Dockerfile unchanged (TestSupport is NOT a production dependency). A new code-review SKILL.md Phase 6 checklist row flags near-identical helper logic across test projects as a Medium / Maintainability finding with explicit cycle-2 retro back-reference, so this whole pattern stops at one occurrence. module-layout.md adds a TestSupport Shared/Cross-Cutting entry documenting the production-isolation invariant. tests_unit.md + tests_integration.md updated to describe the consolidated layout. sln updated. Test-suite gate (AC-2 + AC-3) deferred to Step 16 Final Test Run per implement-skill convention. Per-batch review verdict: PASS_WITH_WARNINGS with 1 Low (pre-existing 7.0.3 version pin preserved verbatim from cycle-2 IntegrationTests csproj for parity; not blocking; deferred bump). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -1,119 +0,0 @@
|
||||
# Consolidate JWT test-mint helpers
|
||||
|
||||
**Task**: AZ-491_consolidate_jwt_test_helpers
|
||||
**Name**: Consolidate JWT test-mint helpers
|
||||
**Description**: Eliminate the duplicate JWT-minting logic that exists in both `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` by moving the canonical implementation to a single shared location consumed by both test projects.
|
||||
**Complexity**: 3 points
|
||||
**Dependencies**: None (no in-repo task dependency; logically follows AZ-487 which introduced both copies)
|
||||
**Component**: Test infrastructure (`SatelliteProvider.Tests` + `SatelliteProvider.IntegrationTests`)
|
||||
**Tracker**: AZ-491
|
||||
**Epic**: none (cycle-3 test-infrastructure hardening)
|
||||
|
||||
## Problem
|
||||
|
||||
During cycle 2 (AZ-487), JWT token-minting logic was written twice — once in `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` (unit-side) and once in `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken` (integration-side). The two implementations are near-identical: same signing algorithm, same claim shape, same negative-lifetime workaround. Both contained the same `Expires < NotBefore` bug and both required independent fixes in separate commits (`f64d0d7` for unit-side, `11b7074` for integration-side).
|
||||
|
||||
The cycle-2 retrospective identified this as Improvement Action 1: "Consolidate JWT token-mint helpers into one shared utility" (`_docs/06_metrics/retro_2026-05-11_cycle2.md` § 7). LESSONS.md ring buffer carries the lesson under `[testing]`. Until this is resolved, every future JWT-side change pays a dual-fix tax, and the two implementations will continue to drift on shape, claim handling, or signing parameters — a real security-relevance risk for the surface that mints test credentials.
|
||||
|
||||
## Outcome
|
||||
|
||||
- A single canonical JWT-minting implementation exists, consumable by both `SatelliteProvider.Tests` and `SatelliteProvider.IntegrationTests`.
|
||||
- `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken` and `MintExpiredToken` and `TamperSignature` are deleted; existing call sites (`Program.cs`, `JwtIntegrationTests`, `UavUploadTests`) consume the consolidated factory.
|
||||
- Runner-side concerns (`JwtTestHelpers.cs::ResolveSecretOrThrow`, `AttachDefaultAuthorization`) remain in `SatelliteProvider.IntegrationTests` — only the mint logic moves.
|
||||
- All 253 unit tests + the full integration test suite pass end-to-end with the consolidated factory.
|
||||
- A code-review checklist item is added to `.cursor/skills/code-review/SKILL.md` Phase 6 so that future duplicate-helper situations are flagged at review time, not at retrospective time.
|
||||
|
||||
## Scope
|
||||
|
||||
### Included
|
||||
|
||||
- Move (or re-locate) the canonical `JwtTokenFactory` class so both test projects can reference it. Implementer chooses between two viable shapes (see § Risks & Mitigation Risk 1):
|
||||
- **Option A**: new test-support class library `SatelliteProvider.TestSupport` (no test framework), referenced by both `SatelliteProvider.Tests` and `SatelliteProvider.IntegrationTests`. Larger upfront cost; cleaner long-term layering.
|
||||
- **Option B**: `SatelliteProvider.IntegrationTests` gains a `ProjectReference` to `SatelliteProvider.Tests`, consuming the existing `TestUtilities/JwtTokenFactory.cs` directly. Smaller cost; integration tests acquire a dependency on unit tests (slightly unusual but precedented in some .NET test layouts).
|
||||
- Delete `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs::MintValidToken`, `MintExpiredToken`, and `TamperSignature`. Their integration-test call sites switch to the canonical factory.
|
||||
- Update all call sites: `SatelliteProvider.IntegrationTests/Program.cs`, `JwtIntegrationTests.cs`, `UavUploadTests.cs`.
|
||||
- Add a code-review checklist row in `.cursor/skills/code-review/SKILL.md` Phase 6 (Cross-Task Consistency): "If two test projects contain near-identical helper logic with no shared source-of-truth, flag as a Medium finding for consolidation."
|
||||
- Update `_docs/02_document/modules/tests_unit.md` and `tests_integration.md` to point at the new consolidated location (one-line each).
|
||||
|
||||
### Excluded
|
||||
|
||||
- Any change to production code in `SatelliteProvider.Api`, `SatelliteProvider.Common`, or service projects.
|
||||
- Any change to `UavTileImageFactory` (also a candidate for consolidation but out of scope here — keep this PBI surgical).
|
||||
- Any change to `ResolveSecretOrThrow` / `AttachDefaultAuthorization` — these are runner-side concerns and stay in `SatelliteProvider.IntegrationTests`.
|
||||
- Changes to the JWT algorithm, claim shape, or `TokenValidationParameters` — this PBI is a pure refactor, not a behavior change.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
**AC-1: Single source of truth**
|
||||
Given the repository after this PBI lands
|
||||
When a developer searches for `JwtSecurityToken(` constructor calls in test code
|
||||
Then exactly one location implements JWT minting; no duplicate implementation exists in another test project.
|
||||
|
||||
**AC-2: Existing integration tests pass unchanged in behavior**
|
||||
Given the full integration test suite (`scripts/run-tests.sh --full`)
|
||||
When the suite executes against the consolidated factory
|
||||
Then all integration test scenarios produce the same pass/fail outcomes as they did prior to this PBI.
|
||||
|
||||
**AC-3: Existing unit tests pass unchanged in behavior**
|
||||
Given the full unit test suite (`dotnet test SatelliteProvider.Tests`)
|
||||
When the suite executes against the consolidated factory
|
||||
Then all 253 unit test scenarios produce the same pass/fail outcomes as they did prior to this PBI.
|
||||
|
||||
**AC-4: Runner-side concerns preserved**
|
||||
Given the integration test runner
|
||||
When the runner starts up
|
||||
Then `ResolveSecretOrThrow` still reads `JWT_SECRET` from the environment with the same minimum-length validation, AND `AttachDefaultAuthorization` still sets the `Authorization: Bearer …` header on the shared `HttpClient`.
|
||||
|
||||
**AC-5: Cycle-2 fixes remain effective**
|
||||
Given a fixture that requests an expired-lifetime token (negative `TimeSpan`)
|
||||
When the consolidated factory mints the token
|
||||
Then `Expires` is greater than `NotBefore` (the cycle-2 `IDX12401` fix is preserved), AND the token fails `ValidateLifetime` downstream (the lifetime check still fires).
|
||||
|
||||
**AC-6: Code-review rule lands**
|
||||
Given a future PR that adds a near-identical helper to two test projects
|
||||
When the code-review skill runs Phase 6 (Cross-Task Consistency)
|
||||
Then the checklist item from this PBI fires and the finding is recorded as a Medium-severity duplicate-helper item.
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
**Compatibility**
|
||||
- Must compile against `net8.0` target framework (same as all existing projects).
|
||||
- Must not introduce new external NuGet dependencies — the consolidated factory uses only `System.IdentityModel.Tokens.Jwt` and `Microsoft.IdentityModel.Tokens`, already referenced by both test projects.
|
||||
|
||||
**Reliability**
|
||||
- The negative-lifetime workaround documented in the existing factory (cycle-2 `f64d0d7` / `11b7074` commits) must be preserved exactly — `Expires > NotBefore` is invariant.
|
||||
|
||||
## Unit Tests
|
||||
|
||||
| AC Ref | What to Test | Required Outcome |
|
||||
|--------|-------------|-----------------|
|
||||
| AC-1 | The consolidated factory is the sole `JwtSecurityToken` constructor call site in test code | A grep of `new JwtSecurityToken(` across all `**/*.cs` under both test projects returns exactly one match (in the consolidated factory) |
|
||||
| AC-3 | All existing `JwtTokenFactoryTests` cases | Pass against the consolidated factory at its new location |
|
||||
| AC-5 | Expired-token fixture path | The factory's `Create` (or new equivalent) with `lifetime: TimeSpan.FromMinutes(-5)` produces `Expires > NotBefore` AND the resulting token fails `ValidateLifetime` |
|
||||
|
||||
## Blackbox Tests
|
||||
|
||||
| AC Ref | Initial Data/Conditions | What to Test | Expected Behavior | NFR References |
|
||||
|--------|------------------------|-------------|-------------------|----------------|
|
||||
| AC-2 | Integration test runner started with a valid `JWT_SECRET` | `JwtIntegrationTests` + `UavUploadTests` run end-to-end through the consolidated factory | Same pass/fail outcomes as the pre-PBI baseline | Compatibility |
|
||||
| AC-4 | Integration test runner started with `JWT_SECRET` set to a 32-byte value | `ResolveSecretOrThrow` returns the secret; `AttachDefaultAuthorization` sets the `Authorization` header on the shared `HttpClient` | Runner-side behavior unchanged | — |
|
||||
|
||||
## Constraints
|
||||
|
||||
- Stay within the existing project layout — the chosen Option A or B must not require renaming or restructuring `SatelliteProvider.Tests` / `SatelliteProvider.IntegrationTests`.
|
||||
- The consolidated factory must remain a `public static class` (current shape) — do not introduce DI, abstractions, or factories around it. This is test infrastructure, not production code.
|
||||
- Comment in the consolidated factory must retain the explanation of the `Expires <= NotBefore` workaround so future readers don't "fix" the apparent oddity.
|
||||
|
||||
## Risks & Mitigation
|
||||
|
||||
**Risk 1: Option A vs Option B trade-off**
|
||||
- *Risk*: Option A (new `SatelliteProvider.TestSupport` library) is the cleaner long-term shape but costs ~1 extra csproj, a new ProjectReference graph entry, and a new module in `module-layout.md`. Option B (integration tests reference unit tests) is cheaper but creates a slightly unusual dependency direction.
|
||||
- *Mitigation*: The implementer chooses based on whether other shared test utilities (e.g., `UavTileImageFactory`) are likely to be consolidated in the next 1–2 cycles. If yes, Option A pays for itself quickly. If no, Option B is fine. Document the choice in the batch report so future agents understand the precedent.
|
||||
|
||||
**Risk 2: Drift in the comment block that documents the `Expires <= NotBefore` workaround**
|
||||
- *Risk*: Moving the file may invite a "cleanup" of the comment, removing the rationale and re-introducing the `IDX12401` bug on a future edit.
|
||||
- *Mitigation*: AC-5 + the unit test that exercises the expired-token path catch any regression. The comment block survives the move verbatim.
|
||||
|
||||
**Risk 3: Removal of duplicate file exposes hidden coupling**
|
||||
- *Risk*: Some other test file may currently `using SatelliteProvider.IntegrationTests` with intent to consume `JwtTestHelpers` symbols beyond just `MintValidToken` (e.g., `DefaultSubject` constant).
|
||||
- *Mitigation*: Before deletion, grep for all references to `JwtTestHelpers.` symbols in the codebase and replace each one with the consolidated equivalent. The `DefaultSubject` constant moves with the mint logic.
|
||||
Reference in New Issue
Block a user