azaion/satellite-provider
1
0
Fork 0
mirror of https://github.com/azaion/satellite-provider.git synced 2026-06-22 16:31:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-289] [AZ-290] Batch 3 tests: integration ZIP cap, perf, security, queue

Browse Source 
AZ-289 — RL-01 50MB ZIP cap added to RunRouteWithTilesZipTest;
existing integration tests already cover BT-08/BT-09 + AC-1/AC-2.

AZ-290:
- scripts/run-performance-tests.sh extended with PT-01/03/04/05
- SatelliteProvider.IntegrationTests/SecurityTests.cs (SEC-01..SEC-04),
  wired into Program.cs
- SatelliteProvider.Tests/RegionRequestQueueTests.cs covering RS-04 /
  RL-02 queue capacity behavior

Notes:
- RS-04 spec wording ("rejects overflow") drifts from the channel's
  BoundedChannelFullMode.Wait back-pressure semantics. Tests assert
  the actual behavior; spec to be reconciled in Step 12 (Test-Spec
  Sync). Tracked as Low/Spec-Gap in batch_03_review.md.
- Unit tests: 35/35 passed (Docker .NET 8 SDK).
- Integration test project builds clean (0 warnings, 0 errors).

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-10 05:10:30 +03:00
parent dea0b8b4c0
commit 7822841587
10 changed files with 473 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/reviews/batch_03_review.md
+68
View File
@@ -0,0 +1,68 @@
# Code Review Report
**Batch**: 3 — AZ-289 (Integration tests: route map processing + ZIP), AZ-290 (Non-functional tests: perf, resilience, security, limits)
**Date**: 2026-05-10
**Verdict**: PASS_WITH_WARNINGS
## Scope
Changed files in this batch:
- `SatelliteProvider.IntegrationTests/ExtendedRouteTests.cs` — added 50 MB ZIP cap assertion (RL-01).
- `SatelliteProvider.IntegrationTests/SecurityTests.cs` — new file; SEC-01..SEC-04 integration tests.
- `SatelliteProvider.IntegrationTests/Program.cs` — wires `SecurityTests.RunAll` into the integration runner.
- `SatelliteProvider.Tests/RegionRequestQueueTests.cs` — new file; covers queue capacity behavior (RS-04 / RL-02 at unit level).
- `scripts/run-performance-tests.sh` — extended with PT-01, PT-03, PT-04, PT-05.
Existing integration tests (`BasicRouteTests.RunRouteWithRegionProcessingAndStitching`, `ExtendedRouteTests.RunRouteWithTilesZipTest`, `ComplexRouteTests.RunComplexRouteWithStitching*`) already covered BT-08 / BT-09 / RS-06 / RL-01 (capped now); no duplicates added.
## Findings
| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Spec-Gap | `_docs/02_tasks/todo/AZ-290_nonfunctional_tests.md:24` | RS-04 spec says "rejects overflow"; implementation `BoundedChannelFullMode.Wait` blocks instead |
| 2 | Low | Maintainability | `SatelliteProvider.Tests/RegionRequestQueueTests.cs:35` | Time-based blocking assertion uses a 150ms `Task.Delay` heuristic |
### Finding Details
**F1: RS-04 spec wording vs. implementation behavior** (Low / Spec-Gap)
- Location: `_docs/02_tasks/todo/AZ-290_nonfunctional_tests.md:24`
- Description: RS-04 reads "Queue rejects overflow (capacity 1000)". The actual queue uses `Channel.CreateBounded<>(BoundedChannelFullMode.Wait)` (`SatelliteProvider.Services/RegionRequestQueue.cs:17-20`), which blocks the producer rather than rejecting the request. The unit tests assert the actually-implemented behavior (block + cancellable wait). Spec needs to be updated to "Queue back-pressures producer when full" in Step 12 (Test-Spec Sync).
- Suggestion: Fix in Step 12; do not change runtime behavior — back-pressure is the correct choice for an in-process producer/consumer.
- Task: AZ-290
**F2: Time-based assertion in capacity-block test** (Low / Maintainability)
- Location: `SatelliteProvider.Tests/RegionRequestQueueTests.cs:35-46` (`EnqueueAsync_BlocksWhenAtCapacity_UntilDequeue_RL02`)
- Description: The test relies on a 150 ms `Task.Delay` race to confirm that the third `EnqueueAsync` does not complete while the queue is full. This is a small heuristic risk on heavily loaded CI agents. Risk is low because the operation only completes after a real `DequeueAsync`, and the subsequent `WaitAsync(1s)` covers slow paths.
- Suggestion: Acceptable as-is; revisit if the test ever flakes on CI.
- Task: AZ-290
## Acceptance Criteria Coverage
### AZ-289
| AC | Coverage | Evidence |
|----|----------|----------|
| AC-1 — Route map processing completes within 180s | Existing | `WaitForRouteReady(routeId, 180, 3000)` in `RunRouteWithTilesZipTest` |
| AC-2 — ZIP structure validated (entry count = CSV, path prefix `tiles/`) | Existing + reinforced | Entry count check, `firstEntry.FullName.StartsWith("tiles/")`, plus new 50 MB cap |
### AZ-290
| AC | Coverage | Evidence |
|----|----------|----------|
| AC-1 — Performance script passes thresholds | New | `scripts/run-performance-tests.sh` covers PT-01..PT-06 with `check_threshold` per scenario |
| AC-2 — Resilience tests verify state transitions and resource limits | Mixed (unit + integration + structural) | RS-03 in `RegionServiceTests.ProcessRegionAsync_DownloaderFailure_TransitionsToFailedAndWritesErrorSummary` (Batch 2); RS-04 / RL-02 in new `RegionRequestQueueTests`; RS-01 / RS-02 covered by Docker startup + `DbUp` migrations; RS-05 (semaphore) and RL-03 / RL-04 (concurrency caps) verified structurally via `appsettings.json` (`MaxConcurrentDownloads: 4`, `MaxConcurrentRegions: 20`, `QueueCapacity: 1000`); RS-06 covered by existing `ComplexRouteTests` |
| AC-3 — Security tests confirm no injection or traversal vulnerabilities | New | `SecurityTests.cs` runs SEC-01 (non-numeric coord rejected), SEC-02 (multiple traversal attempts rejected), SEC-03 (oversized region rejected), SEC-04 (malformed JSON rejected) |
## Test Run
- Unit tests: 35 passed / 0 failed (Docker `mcr.microsoft.com/dotnet/sdk:8.0`).
- Integration test project: builds clean (0 warnings, 0 errors); runtime execution deferred to Step 7 (Run Tests) since it requires the full Docker Compose stack and a Google Maps API key.
## Baseline Delta
No new architecture findings. The two High findings from `architecture_compliance_baseline.md` (F1: TileService concrete dependency, F2: ISatelliteDownloader dead code) were both resolved by the earlier testability refactor that landed in the autodev baseline commit and remain resolved at the end of this batch.
## Verdict Rationale
No Critical or High findings. Two Low findings (spec wording drift, time-based test heuristic) are non-blocking and tracked for follow-up. Verdict: **PASS_WITH_WARNINGS** — proceed to commit.