mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 23:11:12 +00:00
[AZ-597] Batch 77: replay_mode helpers + 13 scenario stub rewires
Add `runner/helpers/replay_mode.py` (NullFrameSink, NullFcInboundEmitter, default_frame_period_ms, load_replay_json, resolve_replay_subdir, imu_replay_noop) and rewire all 13 scenarios off their local `_resolve_*` / `_drive_*` / `_push_*` NotImplementedError stubs. Closes the offline FDR-replay execution path. `grep raise NotImplementedError` under `e2e/tests/` now returns zero matches. +17 unit tests (626 total, up from 608). Unit-test behaviour unchanged (scenarios still skip via b75 sitl_replay_ready gate when E2E_SITL_REPLAY_DIR is unset). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,152 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 77 — AZ-597 (replay_mode helpers + 13 scenario stub rewires)
|
||||
**Date**: 2026-05-17
|
||||
**Verdict**: PASS
|
||||
|
||||
## Findings
|
||||
|
||||
(none)
|
||||
|
||||
## Findings Sweep
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
Read the AZ-597 task spec, the `FrameSink` / `FcInboundEmitter`
|
||||
Protocol definitions in `frame_source_replay.py` and `imu_replay.py`
|
||||
(b74) to verify the new `Null*` implementations match the exact
|
||||
method signatures, the `outlier_tolerance_evaluator.GtPose` dataclass
|
||||
shape consumed by FT-N-01, and the surface used by FT-P-03/14's
|
||||
`_push_single_image_and_observe` return tuple. Re-read the b75
|
||||
`sitl_observer.replay_dir` env-var resolution pattern for symmetry
|
||||
(`E2E_SITL_REPLAY_DIR`, empty-string-as-None semantics).
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
|
||||
| AC | Coverage | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (`NullFrameSink.write_frame` / `NullFcInboundEmitter.emit` are pure counters) | `test_null_frame_sink_counts_writes`, `test_null_frame_sink_starts_at_zero`, `test_null_emitter_counts_emits`, `test_null_emitter_starts_at_zero` | Covered |
|
||||
| AC-2 (`load_replay_json` raises `FileNotFoundError` env-unset + file-missing; `ValueError` malformed; round-trips otherwise) | `test_load_replay_json_raises_when_env_unset`, `test_load_replay_json_raises_when_env_empty`, `test_load_replay_json_raises_when_file_missing`, `test_load_replay_json_raises_on_malformed_json`, `test_load_replay_json_round_trips_dict`, `test_load_replay_json_round_trips_list` | Covered |
|
||||
| AC-3 (`resolve_replay_subdir` raises `FileNotFoundError` env-unset + subdir-missing; returns Path otherwise) | `test_resolve_replay_subdir_raises_when_env_unset`, `test_resolve_replay_subdir_raises_when_subdir_missing`, `test_resolve_replay_subdir_returns_path_when_exists`, `test_resolve_replay_subdir_rejects_file_at_path` | Covered |
|
||||
| AC-4 (`default_frame_period_ms()` returns 33; documented) | `test_default_frame_period_ms_is_30_fps` (asserts both function + constant); module docstring documents 30 fps default | Covered |
|
||||
| AC-5 (13 scenarios have local `_resolve_*` / `_drive_*` / `_push_*` stubs deleted; import from `runner.helpers.replay_mode`) | Verified by `Grep raise NotImplementedError under e2e/tests` returning **no matches**. The 13 scenarios touch: FT-P-01/02/04/05/07/08/09-AP/09-iNav/10/11, FT-N-01/02/03/04, and FT-P-03/14. | Covered |
|
||||
| AC-6 (≥6 unit tests for `replay_mode.py`) | 17 tests total (2 null-sink, 2 null-emitter, 1 frame-period, 2 imu-replay-noop, 6 load_replay_json, 4 resolve_replay_subdir) | Covered (exceeds floor) |
|
||||
| AC-7 (full suite passes) | 626 passed (+18 from 608; +17 new replay_mode tests + 1 new directory-layout parametrize entry) | Covered |
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
* **Single responsibility**: each surface in `replay_mode.py` owns
|
||||
exactly one concern.
|
||||
* `NullFrameSink` / `NullFcInboundEmitter` — Protocol-compatible
|
||||
counter sinks. No I/O, no JSON, no env-var reads. Pure data.
|
||||
* `default_frame_period_ms` — constant lookup. Trivial; lives in
|
||||
the same module as the constant it wraps so callers see the
|
||||
rationale next to the value.
|
||||
* `imu_replay_noop` — explicit no-op with a comment explaining
|
||||
why the IMU CSV is ignored in replay mode. Signature mirrors
|
||||
`imu_replay.ImuReplayer.replay` so a future live-mode driver
|
||||
can be slotted in.
|
||||
* `load_replay_json` / `resolve_replay_subdir` — two file-system
|
||||
surfaces, distinct contracts ("file must exist + parse" vs
|
||||
"directory must exist"). Both go through one shared
|
||||
`_resolve_replay_root_or_raise` so env-var semantics are
|
||||
enforced exactly once.
|
||||
* **No suppressed errors**:
|
||||
* `load_replay_json` converts `json.JSONDecodeError` → `ValueError`
|
||||
with the offending file path AND `raise … from exc` preserves
|
||||
the original.
|
||||
* `_resolve_replay_root_or_raise` includes the calling surface in
|
||||
the error message (`"load_replay_json('foo.json'): ${ENV} not set"`)
|
||||
so a test author seeing the failure knows exactly which scenario
|
||||
fired which loader.
|
||||
* No bare `except`, no `2>/dev/null`, no empty `pass`.
|
||||
* **AAA comment discipline**: all 17 new tests use
|
||||
`# Arrange / # Act / # Assert`; sections omitted when not needed.
|
||||
* **Code comments**: only the module docstring narrates "why
|
||||
replay-mode no-ops are correct". Per-function docstrings document
|
||||
contracts and the live-mode follow-up. No line-narration.
|
||||
* **Public boundary**: `replay_mode.py` imports stdlib only
|
||||
(`json`, `os`, `pathlib`). Zero `from gps_denied_onboard ...` imports.
|
||||
|
||||
### Phase 4 — Security
|
||||
|
||||
* **No new credentials, secrets, or network surfaces**. All work is
|
||||
in-process counter state + file I/O over a controlled env-var-rooted
|
||||
path.
|
||||
* **`E2E_SITL_REPLAY_DIR`** read consistently with b75/b76 (set →
|
||||
use; unset / empty / whitespace → treated as absent). No
|
||||
shell-injection surface — the path is fed straight into `Path`
|
||||
arithmetic.
|
||||
* **No `eval`, `exec`, `pickle`, `subprocess`, or
|
||||
`yaml.load(unsafe=True)`** in the new module.
|
||||
* **JSON parse is pure stdlib** with explicit error wrapping. No
|
||||
schema validation — that's the caller's job (the scenarios that
|
||||
consume the parsed payload validate field types at the call site).
|
||||
|
||||
### Phase 5 — Performance
|
||||
|
||||
* All surfaces are O(1) or O(N) where N is the input JSON size
|
||||
(single `json.loads` call). No file I/O at module-import time.
|
||||
* `NullFrameSink` / `NullFcInboundEmitter` are constant-time per call
|
||||
with single integer increment + no allocations.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
* **Env-var pattern matches b75 (`sitl_observer.replay_dir`) and b76
|
||||
(`fc_proxy_runtime._resolve_replay_dir`)**: same env var, same
|
||||
"empty string → None" semantics, same lazy resolution per call.
|
||||
The three modules deliberately do not share a helper — each owns
|
||||
its own resolution so the import graph stays flat
|
||||
(`replay_mode` ↛ `sitl_observer`, `replay_mode` ↛ `fc_proxy_runtime`).
|
||||
The cost is ~12 lines of duplicate env-var code across three
|
||||
modules; the benefit is no cross-dependency surface.
|
||||
* **Skip-gate interaction**: the b75 `sitl_replay_ready` fixture
|
||||
still skips before any of these loaders fire in unit-test mode.
|
||||
When the SITL replay fixture builder lands and the env var is set,
|
||||
scenarios will reach the loaders — at which point the explicit
|
||||
`FileNotFoundError` messages ("replay fixture 'gt_per_frame.json'
|
||||
not found at …") provide a precise pointer to which fixture file
|
||||
is missing.
|
||||
* **`FileNotFoundError` / `ValueError` discipline matches the rest
|
||||
of `e2e/runner/helpers/`** (b73-b76 cumulative): missing inputs →
|
||||
`FileNotFoundError`, malformed inputs → `ValueError` with a file
|
||||
pointer.
|
||||
* **Scenario-side import convention**: every rewired stub imports
|
||||
inside the function body, not at module top-level. This matches
|
||||
the existing scenario convention (`from runner.helpers import …`
|
||||
is deferred so that `pytest --collect-only` doesn't pay the import
|
||||
cost). 13 scenarios, one pattern.
|
||||
* **`_push_single_image_and_observe` and `_resolve_gt_per_frame`
|
||||
field-name discipline**: both load JSON and project into a
|
||||
scenario-local dataclass / tuple. The JSON keys (`frame_idx`,
|
||||
`lat_deg`, `lon_deg`, `record`, `source_label`) match exactly what
|
||||
the evaluators / consumers downstream already expect — no schema
|
||||
translation layer required.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
* **Module placement**: `e2e/runner/helpers/replay_mode.py` (new)
|
||||
+ `e2e/_unit_tests/helpers/test_replay_mode.py` (new). Both
|
||||
registered in `e2e/_unit_tests/test_directory_layout.py`; the
|
||||
layout invariant test still passes.
|
||||
* **No `src/gps_denied_onboard` imports** anywhere. Confirmed.
|
||||
* **No new top-level dependencies** — stdlib only. `requirements.txt`
|
||||
untouched.
|
||||
* **Backwards-compatible scenario contract**: every `_resolve_*` /
|
||||
`_drive_*` / `_push_*` keeps its original name + signature + return
|
||||
type. The 13 rewires are body-only changes — no call site changes
|
||||
in the scenario test functions themselves.
|
||||
|
||||
## Test Results
|
||||
|
||||
* New unit tests: **17** (2 null-sink + 2 null-emitter + 1
|
||||
frame-period + 2 imu-replay-noop + 6 load_replay_json + 4
|
||||
resolve_replay_subdir).
|
||||
* Full `e2e/_unit_tests` suite: **626 passed in 127 s** (previous
|
||||
cumulative: 608 → +18 net = +17 new replay_mode tests + 1 new
|
||||
directory-layout parametrize entry).
|
||||
* No new linter errors (`ReadLints` clean on `replay_mode.py`,
|
||||
`test_replay_mode.py`, `test_directory_layout.py`, and all 13
|
||||
rewired scenario files).
|
||||
* `Grep raise NotImplementedError` under `e2e/tests/` returns **no
|
||||
matches** — confirming AC-5 (every scenario stub deleted).
|
||||
Reference in New Issue
Block a user