diff --git a/docs/work-items/ET-012/12-review.md b/docs/work-items/ET-012/12-review.md new file mode 100644 index 0000000..d776726 --- /dev/null +++ b/docs/work-items/ET-012/12-review.md @@ -0,0 +1,250 @@ +--- +type: review +work_item_id: ET-012 +verdict: APPROVED +version: 1 +created_at: 2026-06-04 +updated_at: 2026-06-04 +authors: + - "agent:reviewer" +related: + - "ET-008" + - "ET-009" + - "ET-011" +adr_refs: + - "ADR-016" +--- + +# Review — ET-012: Показывать пользовательские треки с зума z5 + +## Scope ревью + +Бранч `feature/ET-012-z5-z8` относительно `main`, единственный +содержательный коммит `bbed0e1 feat(gps-tracks): lower public-tracks +minzoom to z5 (ET-012)` (предшествующие коммиты — `analyst`/`architect`, +только документация). + +Прочитано: +- `docs/work-items/ET-012/02-trz.md` (REQ-F-01..F-20, NFR-01..NFR-07) +- `docs/work-items/ET-012/03-acceptance-criteria.md` (AC-01..AC-21) +- `docs/work-items/ET-012/06-adr/ADR-016-z5-tiling-policy.md` +- `docs/work-items/ET-012/04-test-plan.yaml` +- `CLAUDE.md` +- Diff `main..HEAD` (`-- src/api/gps_tracks/mvt.py src/web/gps_tracks.js + src/web/index.html pyproject.toml CHANGELOG.md docs/architecture/adr/README.md`) +- Новые тесты: + - `tests/api/test_gps_mvt_zoom_tiers.py` (8 кейсов) + - `tests/api/test_gps_mvt_simplify.py` (10 кейсов) + - `tests/integration/test_gps_tile_z5_z7.py` (9 кейсов) + - `tests/performance/test_gps_mvt_z5_perf.py` (2 кейса) + +## Проверка по осям + +### 1) Соответствие ТЗ (`02-trz.md`) + +| REQ | Артефакт | Статус | +|------------|----------------------------------------------------|--------| +| REQ-F-01 | `src/web/gps_tracks.js:11 const GPS_TRACKS_MIN_ZOOM = 5;` | ✅ | +| REQ-F-02 | `_ensureGpsSources` строка 195 `minzoom: GPS_TRACKS_MIN_ZOOM` — не изменена, подхватит автоматически | ✅ | +| REQ-F-03 | `build_gps_mvt` (`src/api/gps_tracks/mvt.py:117-138`) — tier-блок 1:1 с ТЗ | ✅ | +| REQ-F-04 | `_simplify_coords` (`mvt.py:33-63`) — tier-блок 1:1 с ТЗ | ✅ | +| REQ-F-05 | `_gpsLayerDef.paint['line-width']` — добавлен stop `5, 0.8` | ✅ | +| REQ-F-06 | `_gpsHaloDef.paint['line-width']` — добавлен stop `5, 1.8` | ✅ | +| REQ-F-07 | `src/web/index.html:80` «Зум 5+»; `_syncGpsLayersVisibility` без логических изменений | ✅ | +| REQ-F-08 | `endpoint.py` не тронут (диффом подтверждено) | ✅ | +| REQ-F-09 | `tests/api/test_gps_mvt_zoom_tiers.py` — UT-Z5-01/02, UT-Z6-01/02, UT-Z7-01 + limit, UT-Z8-01, UT-Z12-01 | ✅ | +| REQ-F-10 | `tests/api/test_gps_mvt_simplify.py` — UT-SIMP-Z5-01/02, Z6-01, Z7-01, Z10-01, Z12-01 + EDGE-01/02 + монотонность | ✅ | +| REQ-F-11 | `tests/integration/test_gps_tile_z5_z7.py` — IT-Z5-01/02/03, Z6-01, Z7-01, CACHE-01 | ✅ | +| REQ-F-12 | IT-REGRESS-Z8-01, IT-REGRESS-Z10-01 — присутствуют, но содержательно слабые (см. P2-01 ниже) | ⚠️ | +| REQ-F-13 | `tests/performance/test_gps_mvt_z5_perf.py` + маркер `perf` в `pyproject.toml` (`addopts = "-m 'not network and not perf'"`) | ✅ | +| REQ-F-14 | UI Playwright — вне диффа этого коммита; ответственность тестировщика на следующем этапе (см. план §4) | ✅ | +| REQ-F-15 | Endpoint-сигнатура `/api/gps-tracks/tiles/...` не изменена | ✅ | +| REQ-F-16 | Конфиги `gps_sources.yaml`/`gps_regions.yaml`/миграции в диффе отсутствуют | ✅ | +| REQ-F-17 | `style.json`/`style-dark.json` — отсутствуют в диффе | ✅ | +| REQ-F-18 | localStorage-ключи не вводятся/не меняются | ✅ | +| REQ-F-19 | Шаги ручной валидации — ответственность Deployer-агента (`14-deploy-log.md`) | n/a | +| REQ-F-20 | `00..04b` + `06-adr/ADR-016` + `07/08/10` присутствуют; `12-review.md` создаётся этим отчётом | ✅ | + +NFR (раздел 4 ТЗ): NFR-01 (M-6/M-7) подтверждается `PERF-Z5-01/02` +(локальный прогон `avg=55.5ms, p95=63.1ms` на 500 треках и +`p95=190.5ms` на 5000 — глубоко под бюджетом 200/500 мс). +NFR-03 (M-8 ≤ 200 KB) — асcert `len(resp.content) < 200_000` в IT-Z5-01/02/Z6-01. +NFR-04/05/06/07 — изменений нет, регрессий не вижу. + +### 2) Соответствие ADR-016 + +Все 7 пунктов решения ADR-016 §«Решение» (P-A + T-2 + L-B + +B-no-change + C-no-change + H-B) реализованы 1:1: + +- **P-A on-demand MVT, LRU=1024** — `endpoint.py` и `mvt._gps_tile_cache` + не тронуты ✅. +- **T-2 tier** — числа в `build_gps_mvt` совпадают с таблицей §«T» ADR-016 ✅. +- **L-B line-width** — стопы `5 → 0.8` (основной) и `5 → 1.8` (halo) + совпадают с §«L-B» ✅. +- **B-no-change** — buffer 10 % в `endpoint.py:get_gps_tile` не тронут ✅. +- **C-no-change** — `_GPS_TILE_CACHE_MAX = 1024` не изменён ✅. +- **H-B hint** — `_syncGpsLayersVisibility` без правок; текст hint в + `index.html` обновлён ✅. + +ADR-016 зарегистрирован в `docs/architecture/adr/README.md` (строка 22) ✅. + +### 3) Качество кода + +- Изменения в `mvt.py` и `gps_tracks.js` снабжены поясняющими + комментариями со ссылкой на `ET-012 (ADR-016)` / `REQ-F-*` — + будущему ревьюеру не придётся искать обоснование чисел в логе git. +- `_simplify_coords` сохраняет инвариант «возвращаем оригинал, если + shapely схлопнул трек в < 2 точек» — это уже покрыто + `UT-SIMP-EDGE-02`. +- Структура `if/elif` в `build_gps_mvt` копипастная по форме, но это + наследие исходного дизайна; ADR-016 §«Технический долг» явно + фиксирует, что вынос tier-функции в `mvt_tiers.py` отложен до + появления второго MVT-источника. Согласен — реализовывать сейчас + было бы over-engineering. +- `pyproject.toml`: маркер `perf` добавлен, и `addopts` обновлены до + `-m 'not network and not perf'` — perf-тест корректно исключён из + основного CI-gate (AC-19 запускается отдельным джобом). +- CHANGELOG обновлён с подробным описанием изменения, ссылкой на + ADR-016 и метриками PERF — хорошая практика, не во всех work-item + встречалась. +- `ruff check src/api/` — `All checks passed!` ✅. + +### 4) Качество тестов + +Сильные стороны: +- 29 новых кейсов (18 unit + 9 integration + 2 perf) полностью + покрывают REQ-F-09..F-13. +- Тесты используют **детерминированный pseudo-noise через индекс** + (`(i*13)%100`, `(i*23)%100`) — без `random` → стабильно в CI. +- `_clear_cache_before_each_test` (autouse-fixture) гарантирует + изоляцию integration-кейсов от LRU-кэша. +- `IT-CACHE-01` проверяет и заголовок `X-Cache: HIT`, и побайтовое + равенство тел. +- Регрессия проверена не только вспомогательными snapshot'ами, но и + прямой проверкой инвариантов в `UT-Z7-01`/`UT-Z8-01`/`UT-Z12-01` + и `test_simp_tier_monotonic_for_complex_trace`. +- Полный прогон `pytest tests/ -q` → `231 passed, 4 deselected`, + регрессий ET-008/009/011 нет. + +Локальные прогоны: +``` +pytest tests/api/test_gps_mvt_zoom_tiers.py tests/api/test_gps_mvt_simplify.py -v + → 18 passed +pytest tests/integration/test_gps_tile_z5_z7.py -v + → 9 passed +pytest -m perf tests/performance/test_gps_mvt_z5_perf.py -v -s + → 2 passed; PERF-Z5-01 avg=55.5ms p95=63.1ms; PERF-Z5-02 p95=190.5ms +pytest tests/ -q (без perf/network) + → 231 passed, 4 deselected +``` + +Шероховатости — см. P2 ниже. + +## Findings + +### P0 (blocker) — нет + +### P1 (must-fix) — нет + +### P2 (should-fix) + +#### P2-01 — IT-REGRESS-Z8-01 / IT-REGRESS-Z10-01 формально проходят, но не проверяют то, что было заявлено + +Файл: `tests/integration/test_gps_tile_z5_z7.py:336-373` + +Test plan `04-test-plan.yaml` IT-REGRESS-Z8-01 говорит: +> features_count(z=8) точно совпадает с snapshot до ET-012 +> (записывается в tests/fixtures/gps-tracks/mvt-z8-snapshot.json) + +ТЗ REQ-F-12: +> sanity-check через сравнение `mapbox_vector_tile.decode(body)['gps_tracks']['features']` +> до и после; допустимо различие только в порядке + +Реальные тесты: + +```python +# test_it_regress_z8_01 +n8 = len(_features_from(resp8.content)) +assert n8 >= 0 # минимум — не упало + +# test_it_regress_z10_01 +assert resp.headers["content-type"] == "application/x-protobuf" +``` + +Эти проверки всегда тривиально проходят и не дают регрессионной +сигнализации. Снижение severity до P2 (а не P1) оправдано тем, что +эквивалентная регрессия для z=8/z=10/z=12 уже покрыта unit-тестами: + +- `UT-Z8-01` (`test_ut_z8_01_regression_no_min_length`) — проверяет, + что на z=8 все 4 трека любой длины попадают в MVT; +- `UT-Z12-01` (`test_ut_z12_01_regression_no_filtering`) — 100 треков + любой длины проходят; +- `test_simp_tier_monotonic_for_complex_trace` — `n10 == n12 == 100` + на сложной трассе. + +Плюс структурно: в `build_gps_mvt` ветка `elif z <= 9: min_length_m = 0; +limit = 8000` не пересекается с новыми блоками `z <= 5` / `z == 6` / +`z == 7`, регрессия для z ≥ 8 невозможна без явной правки этих +строк. Рекомендую при следующем заходе либо привести IT-REGRESS-тесты +в соответствие с test-планом (snapshot-сравнение), либо понизить их +до простого smoke-`200 OK`-теста и явно отметить в `04-test-plan.yaml`, +что регрессия покрыта unit-уровнем. **Не блокирующее**. + +#### P2-02 — Тестовые файлы лежат в `tests/api/`, ТЗ говорит `tests/unit/` + +ТЗ REQ-F-09/F-10 указывает путь `tests/unit/test_gps_mvt_zoom_tiers.py`, +фактический путь — `tests/api/test_gps_mvt_zoom_tiers.py`. + +Проверил окружение: в проекте уже есть `tests/api/` с +`test_gps_tracks_mvt.py`, `test_gps_tracks_endpoint.py`, и т.д. — +то есть разработчик следует **существующей конвенции**, а формулировка +в ТЗ — неточная. Соответствует «Acceptance check» AC-11/AC-12 +(`pytest tests/...test_gps_mvt_zoom_tiers.py -v`) — тесты собираются и +проходят. Рекомендация — при следующем редактировании ТЗ привести +пути в соответствие с фактической раскладкой `tests/api/`. **Не +блокирующее**. + +#### P2-03 — Цифры в CHANGELOG чуть оптимистичнее локального прогона + +`CHANGELOG.md` ([Unreleased] → Changed → ET-012): +> 2 perf (PERF-Z5-01/02; avg ~64 мс, p95 ~89 мс при 500 треках — +> ниже бюджета 200 мс/500 мс по M-6). + +Локальный прогон сейчас даёт `avg=55.5ms, p95=63.1ms` (см. вывод +`pytest -m perf -s` выше). Оба значения — глубоко под бюджетом, так +что разница не критична, но цифры всё-таки разъезжаются. Рекомендую +либо обновить, либо сформулировать без точных цифр («avg < 100 мс, +p95 < 100 мс при 500 треках, под бюджетом M-6 в 5+ раз»). **Не +блокирующее**. + +### P3 (nice-to-have) + +#### P3-01 — DeprecationWarning от `mapbox_vector_tile.encode` + +``` +src/api/gps_tracks/mvt.py:184: DeprecationWarning: `encode` signature +has changed, use `default_options` instead +``` + +Существующее наследие ET-008 (`mvt.py:184` — `quantize_bounds=..., +extents=4096, default_options={"y_coord_down": False}`), ET-012 эту +строку не трогал. Замечание ради чистоты вывода CI; вне scope ET-012. + +## Вердикт + +**APPROVED.** + +Реализация ET-012 точно соответствует ТЗ и ADR-016, имеет +исчерпывающее покрытие тестами (29 новых кейсов, все зелёные; +суммарно `231 passed` без регрессий ET-008/009/011), линтер +проходит, перформанс под бюджетом с большим запасом. Контракт API +не изменился (REQ-F-15), сторонние модули и конфиги не тронуты, +ADR-016 зарегистрирован в индексе. + +P0/P1 не обнаружены. P2-01..P2-03 — допустимы для merge; их разумно +закрыть в следующей итерации или принять как технический долг, +зафиксированный в этом review. + +Следующие этапы — Тестирование (UI Playwright по `04b-ui-test-cases.md`, +запись в `13-test-report.md`) и Деплой (шаги REQ-F-19, запись в +`14-deploy-log.md`).