feat(web): worker route integration-test lane for #94 - #177
feat(web): worker route integration-test lane for #94#177LyuboslavLyubenov wants to merge 11 commits into
Conversation
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Прегледах интеграционния тестов стек (с няколко агента, всеки стъпил на реалния код). Посоката е добра — реален SSR Worker през Miniflare, покрити са правилните маршрути от #94. Но има няколко неща за оправяне преди merge, едно от които блокира CI.
🔴 Блокер — hardcode-нати абсолютни пътища към машината на автора (/Users/lyuboslavlyubenov/Desktop/...) в vitest.integration.config.ts (редове 16 и 32), test/integration/setup.ts:44 и test/integration/global-setup.ts:6. Интеграционната lane не тръгва на никоя друга машина или в CI — вероятно затова няма докладвани checks по PR-а.
🔴 Scope — pnpm-lock.yaml въвежда чужд ralph: workspace с @opencode-ai/sdk и wrangler, несвързан с този PR.
🟠 Dead code — 666 реда архивирани R2 spike тестове, изключени от самата конфигурация.
🟠 Тестово качество — няколко asserts дават фалшива увереност (детайли по редовете).
🟡 Документация — test/README.md твърди „46 теста / 9 файла“, а реалната lane пуска 34 теста / 7 файла (броят включва изключения архив).
Подробностите са в коментарите по редовете.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Проверих срещу head 5f6c40225 (не срещу по-ранния коммит) — двата 🔴 блокера на Неда са затворени:
- Hardcode-натите пътища ги няма:
repoRootсега се извежда отpath.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..')— портативно, lane-ът тръгва извън машината на автора. pnpm-lock.yamlизобщо не е пипнат на head — чуждиятralph:workspace /@opencode-ai/sdkго няма.
Посоката е добра: реален SSR Worker през Miniflare (getPlatformProxy, in-memory D1 + binding-и), покрити маршрути от #94, плюс реални регресии за CSV rate-limit (429 + Retry-After) и keyset пейджинг (#87). Security scan на харнеса е чист.
Една residual бележка (не блокер): ralph/ директорията не съществува в репото, но добавените docs (test/README.md, docs/spec/integration-testing.md) и няколко съобщения за провал на тестове още сочат към ralph/criteria-revisions.md, ralph/assumptions.md, ralph/evidence.md. Тоест човек, който дебъгва паднал тест, ще подгони файл, който го няма. Махнатият ralph workspace е оставил dangling препратки — изчистете ги (или върнете файловете под docs/). Плюс 4 inline .skip блока, които може да отпаднат.
След почистване на висящите препратки — от моя страна готово.
done |
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Благодаря — проверих срещу head 9d3e8c35a: висящите ralph/*.md препратки ги няма (чист scan по docs + тестовете), а двата 🔴 блокера на Неда (hardcode-нати пътища, чужд ralph: workspace в lockfile) бяха затворени още по-рано. От моя страна по кода е чисто.
Lane-ът е стойностен: реален SSR Worker през Miniflare, покрити маршрути от #94, регресии за CSV rate-limit (429 + Retry-After) и keyset пейджинг (#87). Одобрявам — при условие че CI мине зелено (advisory; финалният merge е на maintainer).
Бележка: в момента няма пуснат CI на този PR — workflow-ите на fork PR изчакват maintainer с write достъп да цъкне „Approve and run workflows". Щом се пусне и е зелено, одобрението важи.
Бележките на Неда за полиране (архивните R2 тестове / броя в README) остават на твоя преценка — не са блокер от моя страна.
|
Благодаря — наистина качествен принос. Прегледах целия diff на HEAD Сигурност и цялост на данните: чисто. Промяната е само тестове + документация, без да се пипа продукционен runtime, query или миграционен код. Fixture SQL-ът е изцяло статичен — Предишните блокери: всички затворени на HEAD — портативна резолюция на пътищата през Acceptance критерии (#94): покрити — всеки динамичен route има поне един интеграционен тест срещу miniflare D1, а security/cache заглавките са под регресионен тест. Няколко не-блокиращи бележки за полиране, на твоя преценка:
Една оперативна бележка: CI още не е пускан (fork PR-ите изискват maintainer да одобри workflow-ите), така че одобрението ми важи при зелен пробег. Вердикт: Одобрявам — при условие че CI мине зелено. Няма съображения за сигурност, SQL injection, верига на доставки или цялост на данните; остатъчните бележки са само полиране. |
PR midt-bg#177 (ralph/web-route-integration, for midt-bg#94) already owns the worker-route integration-test lane and is further along; shipping a second integration architecture here would collide on apps/web/package.json (both rewrite the `test` script) and leave the repo with two conventions and two runtimes. Remove the pool-workers dependency, vitest.workers.config.ts, wrangler.test.jsonc, and rate-limit.workerd.test.ts; restore package.json / vitest.config.ts / tsconfig.cloudflare.json / pnpm-lock.yaml to their prior state. The fix plus the unit + classifier + node integration tests already prove the .data bypass is closed. The .data rate-limit regression will land in midt-bg#177's lane instead. Backs midt-bg#184.
|
нещо чакаме ли за да се мърджне пр-а? |
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Прегледах последните промени — трите блокера (hardcode-нати пътища, ralph/@opencode-ai/sdk, архивните R2 spike тестове) и всички по-дребни бележки са адресирани чисто, а rate-limit helper-ът е дори по-строг, отколкото предложих. От моя страна е готово за merge (CI-то настрана — червеното е заварен prettier дълг на main, оправя се в #174). Оставям само една дребна, неблокираща бележка по-долу.
|
Всё проверено. Writing the final review. Прегледах PR #177 изцяло срещу HEAD Обхват и естество на промяната Сигурност и цялост на данните — чисто
Предходни блокери — всички затворени на HEAD (проверено локално)
Спрямо issue #94: покрито — всеки динамичен маршрут има поне един интеграционен тест срещу miniflare D1, плюс реални регресии за CSV rate-limit и keyset пейджинг (#87). Отклоненията от буквалния текст на тикета ( Остатъчни, неблокиращи бележки (на преценка на автора):
Вердикт: Одобрявам — при условие че CI мине зелено. Няма съображения за сигурност, SQL injection, верига на доставки или цялост на данните; остатъчните бележки са само полиране. |
|
Прегледах PR #177 изцяло срещу текущия HEAD Обхват и естество на промяната Сигурност и цялост на данните — чисто
Предходни блокери — всички затворени на HEAD (проверено локално)
Спрямо issue #94: покрито — всеки динамичен маршрут има поне един интеграционен тест срещу miniflare D1, плюс реални регресии за CSV rate-limit и keyset пейджинг (#87). Отклоненията от буквалния текст на тикета ( Остатъчни, неблокиращи бележки (на преценка на автора):
Вердикт: Одобрявам — при условие че CI мине зелено. Няма съображения за сигурност, SQL injection, верига на доставки или цялост на данните; остатъчните бележки са само полиране. |
setup.ts and global-setup.ts each inlined ~60 lines of identical SQL helpers and fixture constants. The duplication was a drift hazard — any change to the fixture seed had to be made in lockstep in both files. Move stripSqlCommentsAndCollapse, buildContractsInsert, and the seven FIXTURE_* constants (plus a canonical FIXTURE_STATEMENTS array) into a new helpers/fixtures.ts module that both files now import. Behaviour is unchanged: same SQL emitted, same apply order, same INSERT OR IGNORE semantics. Test count and outputs unchanged: pnpm --filter @sigma/web test still runs 335 unit + 34 integration = 369 tests, all green. Reviewer note: addresses the duplication comment from midt-bg#177 (ydimitrof on c733245).
The merge of main into ralph/web-route-integration brought in new unit tests, shifting the totals: 31 unit files / 335 unit tests, 7 integration files / 34 integration tests, 38 files / 369 tests total. The previous counts in apps/web/test/README.md (30 / 284, 37 / 318) were stale. Also drop a note that fixture declarations and SQL helpers now live in helpers/fixtures.ts (shared between setup.ts and global-setup.ts), so future maintainers editing the seed know where the source of truth is. Reviewer note: addresses the README/PR-description drift comment from midt-bg#177 (ydimitrof on e5e7cf7, repeated on c733245 after the main merge).
|
Адресирах трите неблокиращи полиращи бележки от ydimitrof (на
Промените:
(„does notime out" в Проверки локално:
Моля за свежа ревю pass, когато ви е удобно — diff е малък, нетно 3 файла премахнати от 4 в полза на 1 нов. |
|
Прегледах трите комита над |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR — интеграционна тест-лента за Worker route (#94)
Какво прави PR-ът
PR-ът добавя нова integration-test лента за apps/web, която упражнява реалния SSR Worker pipeline (wrangler.getPlatformProxy + in-memory D1 + caches polyfill). Включени са нови тестове (contracts-csv, contracts-detail-json, contracts-pagination, edge-cache, routes, sitemaps, rate-limit.csv), споделени fixtures/helpers, фикстурата setup.ts, vitest workspace/integration конфигурациите, wrangler.jsonc, както и подробен ADR-0002. Промените са изцяло в тестове, конфигурация и документация — нулев production код.
Като цяло работата е с високо качество: тестовете са добре именувани и асъртват смислени контракти, коментарите обясняват намеренията, fixtures/helpers са споделени без дублиране, изолацията на miniflare state и идемпотентната фикстура (INSERT OR IGNORE) са добре обмислени, а per-IP изолационният тест умишлено проверява за изтичане на глобален rate-limit брояч.
Сигурност (Фаза 0) — CLEAN
- Няма hardcoded секрети. Всички IP адреси са от документационните RFC 5737 диапазони (
203.0.113.0/24,198.51.100.0/24) — не са реални. - Няма нови зависимости, няма промени в URL whitelist, няма подозрителни/обфускирани патърни.
Най-важни забележки
-
contracts-csv.test.ts— тривиално минаващ тест (нарушава „NO CHEATER TESTS“). Тестът приема едновременно200И500като успех, което означава, че CSV export-ът може да е напълно счупен (винаги 500) и тестът пак ще е зелен — той не доказва, че маршрутът реално сервира CSV. Разбираемо е, че ADR-0002 маркира 200-пътя като отложен scope cut, но в текущия си вид тестът гарантира почти нищо за самата функционалност. Това е основната забележка за адресиране преди merge. -
global-setup.ts— риск от изолиранglobalThis.__SIGMA_PROXY__stash. VitestglobalSetupсе изпълнява в главния процес, а тестовите файлове — в pool worker-и, които не споделятglobalThis. Междувременно повечето тестове твърдят, че proxy-то се bootstrap-ва лениво отsetup.ts(per-file). Има рискglobal-setupда засява отделен in-memory D1 (persist: false), който тестовете никога не използват. (setup.tsе в другата партида — това е забележка за проверка, не потвърден дефект.)
Дребни забележки
stripSqlCommentsAndCollapseмаха--и колабсира whitespace преди string-aware парсването (виж inline).contracts-pagination.test.tsиhelpers/headers.tsзавършват без newline в края на файла.- Консистентност на коментарите:
contracts-detail-json.test.tsтвърди „proxy is bootstrapped by ./global-setup.ts“, докато други тестове твърдят „by ./setup.ts (lazy per-file)“. Едно от двете е неточно — архитектурата трябва да е описана еднакво навсякъде. - Няколко дребни бележки около конфигурацията и стила (инлайн) — нищо блокиращо.
Вердикт: COMMENT
Солидна, добре документирана работа без блокиращи проблеми. Преди merge препоръчвам да се адресира т.1 (cheater-тестът за CSV) и да се потвърди т.2 (реалният път на bootstrap-ване на proxy-то), за да минат quality gate-овете за тестове. Останалите забележки са незадължителни подобрения.
…omes, drop redundant globalSetup Nine review threads on the integration-test lane (PR midt-bg#177): T-002 — contracts-csv "cheater" 200||500 assertion. The disjunction passed even if /contracts.csv always 500'd in production. Gate the expected outcome on the build mode (import.meta.env.DEV): DEV asserts the documented devalue 500; a prod/pre-built lane asserts the 200 contract. A status outside the mode's expectation now fails loudly instead of being tolerated. T-003 — redundant vitest globalSetup. global-setup.ts booted a proxy, ran migrations, seeded fixtures, then disposed — but vitest runs each test file in its own worker, so globalThis.__SIGMA_PROXY__ was not visible to tests (setup.ts already bootstraps per-worker). Removed global-setup.ts, unwired it from the config, and updated setup.ts / fixtures.ts / README / sibling test comments to reflect per-worker lazy bootstrap as the only path. T-004 — stripSqlCommentsAndCollapse corrupted string literals. The per-line `--` strip ran before the string-aware split, so `'a--b'` became `'a`; and collapse-whitespace mangled `'a b'` → `'a b'`. Rewrote as a single string- aware char scanner (comment strip + statement split + whitespace collapse all honour in-string state). Added helpers/fixtures.test.ts (7 tests) covering good/bad paths including the two regression cases. TDD: failing tests first. T-005 — duplicated server.deps.inline. Defined identically at top-level `server` (Vite dev-server, unused by `vitest run`) and `test.server`. Removed the top-level copy with an explanatory comment. T-006 — dead exclude config. The exclude list targets paths the include glob never matches. Kept it as a defensive safety net with a comment explaining why (it blocks accidental double-runs if `include` is ever widened). T-007 — comment/regex mismatch in compareSemverDesc. The comment described a `(peer-deps-hash)` parens flavour that does not occur in pnpm store dir names (only in resolved package.json deps); the actual store dirs use plain semver or `_`-delimited peer-dep suffixes. Rewrote the comment to describe the real formats. T-008 / T-009 — missing trailing newline in routes.test.ts and sitemaps.test.ts. Added. T-010 — rate-limit 500 masking. assertCsvNonRateLimitedResponse accepted any 500 whose body matched the devalue text, which could mask a real regression with the same shape. Added a hard `not.toBe(429)` floor (rate-limit leak fails loudly regardless of body), gated the 500 acceptance on import.meta.env.DEV, and linked the tolerance to the ADR-0002 deferred item. Validation: integration lane 8 files / 41 tests pass (was 7 / 34, +7 new fixtures tests); unit lane 31 files / 335 tests pass; `pnpm typecheck` exit 0.
…p version (T-001) PR midt-bg#177 review T-001 (non-blocking): the `.find()` fallback picked the first @opentelemetry/api store entry after a descending semver sort. If pnpm ever hoists two versions, that can differ from the version the app actually imports, silently aliasing the wrong build/esm. Extract the store-walking logic into a pure, unit-tested helper `pickOtelStoreEntry` that prefers an EXACT match on the version the app declares in package.json (stripping semver range operators and ignoring the `_…` peer-dep hash), falling back to the highest semver when the app version is absent. Both branches are deterministic. TDD: 7 tests covering empty store, exact match (incl. peer-dep hash suffix), fallback to highest semver, determinism under input reordering, and ignoring unrelated @opentelemetry/* packages. Integration lane 9 files / 48 tests pass; typecheck exit 0.
|
Прегледах #177 на дълбочина срещу head Истински, не мокнат: Дискриминиращо там, където има значение:
Честно скоупнато (плюс, не минус): Две неща за яснота (не блокират):
Солидна работа — реален път, честни граници, дискриминиращи тестове. |
|
@lyubomir-bozhinov Благодаря за задълбочения преглед и одобрението. По двете бележки: 1. Координация с #183 — съгласен, това е точното място. Ще разширя 2. Обхват — потвърдено разбиране. Lane-ът валидира worker/header контракта (статус, security headers, rate-limit, content-type, Нищо блокиращо за action този пас — приемам одобрението. PR-ът е |
setup.ts and global-setup.ts each inlined ~60 lines of identical SQL helpers and fixture constants. The duplication was a drift hazard — any change to the fixture seed had to be made in lockstep in both files. Move stripSqlCommentsAndCollapse, buildContractsInsert, and the seven FIXTURE_* constants (plus a canonical FIXTURE_STATEMENTS array) into a new helpers/fixtures.ts module that both files now import. Behaviour is unchanged: same SQL emitted, same apply order, same INSERT OR IGNORE semantics. Test count and outputs unchanged: pnpm --filter @sigma/web test still runs 335 unit + 34 integration = 369 tests, all green. Reviewer note: addresses the duplication comment from midt-bg#177 (ydimitrof on c733245).
The merge of main into ralph/web-route-integration brought in new unit tests, shifting the totals: 31 unit files / 335 unit tests, 7 integration files / 34 integration tests, 38 files / 369 tests total. The previous counts in apps/web/test/README.md (30 / 284, 37 / 318) were stale. Also drop a note that fixture declarations and SQL helpers now live in helpers/fixtures.ts (shared between setup.ts and global-setup.ts), so future maintainers editing the seed know where the source of truth is. Reviewer note: addresses the README/PR-description drift comment from midt-bg#177 (ydimitrof on e5e7cf7, repeated on c733245 after the main merge).
…omes, drop redundant globalSetup Nine review threads on the integration-test lane (PR midt-bg#177): T-002 — contracts-csv "cheater" 200||500 assertion. The disjunction passed even if /contracts.csv always 500'd in production. Gate the expected outcome on the build mode (import.meta.env.DEV): DEV asserts the documented devalue 500; a prod/pre-built lane asserts the 200 contract. A status outside the mode's expectation now fails loudly instead of being tolerated. T-003 — redundant vitest globalSetup. global-setup.ts booted a proxy, ran migrations, seeded fixtures, then disposed — but vitest runs each test file in its own worker, so globalThis.__SIGMA_PROXY__ was not visible to tests (setup.ts already bootstraps per-worker). Removed global-setup.ts, unwired it from the config, and updated setup.ts / fixtures.ts / README / sibling test comments to reflect per-worker lazy bootstrap as the only path. T-004 — stripSqlCommentsAndCollapse corrupted string literals. The per-line `--` strip ran before the string-aware split, so `'a--b'` became `'a`; and collapse-whitespace mangled `'a b'` → `'a b'`. Rewrote as a single string- aware char scanner (comment strip + statement split + whitespace collapse all honour in-string state). Added helpers/fixtures.test.ts (7 tests) covering good/bad paths including the two regression cases. TDD: failing tests first. T-005 — duplicated server.deps.inline. Defined identically at top-level `server` (Vite dev-server, unused by `vitest run`) and `test.server`. Removed the top-level copy with an explanatory comment. T-006 — dead exclude config. The exclude list targets paths the include glob never matches. Kept it as a defensive safety net with a comment explaining why (it blocks accidental double-runs if `include` is ever widened). T-007 — comment/regex mismatch in compareSemverDesc. The comment described a `(peer-deps-hash)` parens flavour that does not occur in pnpm store dir names (only in resolved package.json deps); the actual store dirs use plain semver or `_`-delimited peer-dep suffixes. Rewrote the comment to describe the real formats. T-008 / T-009 — missing trailing newline in routes.test.ts and sitemaps.test.ts. Added. T-010 — rate-limit 500 masking. assertCsvNonRateLimitedResponse accepted any 500 whose body matched the devalue text, which could mask a real regression with the same shape. Added a hard `not.toBe(429)` floor (rate-limit leak fails loudly regardless of body), gated the 500 acceptance on import.meta.env.DEV, and linked the tolerance to the ADR-0002 deferred item. Validation: integration lane 8 files / 41 tests pass (was 7 / 34, +7 new fixtures tests); unit lane 31 files / 335 tests pass; `pnpm typecheck` exit 0.
…p version (T-001) PR midt-bg#177 review T-001 (non-blocking): the `.find()` fallback picked the first @opentelemetry/api store entry after a descending semver sort. If pnpm ever hoists two versions, that can differ from the version the app actually imports, silently aliasing the wrong build/esm. Extract the store-walking logic into a pure, unit-tested helper `pickOtelStoreEntry` that prefers an EXACT match on the version the app declares in package.json (stripping semver range operators and ignoring the `_…` peer-dep hash), falling back to the highest semver when the app version is absent. Both branches are deterministic. TDD: 7 tests covering empty store, exact match (incl. peer-dep hash suffix), fallback to highest semver, determinism under input reordering, and ignoring unrelated @opentelemetry/* packages. Integration lane 9 files / 48 tests pass; typecheck exit 0.
35764fb to
d9241a7
Compare
|
Daily autonomous review — всичките 18 review нишки на #177 са |
Clean automatic merge: no conflicts. Upstream advanced 3 commits since the PR's previous rebase (related-persons midt-bg#226, undici bump midt-bg#282, cacbg fix midt-bg#281); none of those touch the PR's test-only surface (apps/web/test/integration/*, docs/spec/integration-testing.md, apps/web/vitest.integration.config.ts). The single auto-merged file is docs/README.md, which gained a new ADR entry in upstream (0032); the merge preserves the alphabetical/numerical ordering without re-flowing the PR's content. Verification (local): - pnpm typecheck → 7/7 packages clean - pnpm --filter @sigma/web test → 530 passing (52 files, 8 integration files, 41 integration tests, 0 .skip) - pnpm lint → (run separately)
|
Daily autonomous review (re-pass). All 18 review threads remain resolved (PRRT_kwDOS183M86NEf0t / NEf00 / NEf04 / NEf08 / NEf1A / NEf1F / NEf1K / NEf1M / N640y / PWo3v / PWo32 / PWo35 / PWo37 / PWo3- / PWo4A / PWo4G / PWo4J / PWo4L). Reviewers' last substantive comments are from 2026-07-28, all approvals-by-reasoning recorded; no open question directed to the author under 24h. The blocker between this PR and merge was the upstream drift since the previous rebase (3 new upstream commits landed: Rebase summaryApproach. Local merge of Pre-existing drift fixup (follow-up commit
Verification (local)
State
Open thread tracking (informational)The 18 review threads are all The coordination note from lyubomir-bozhinov (2026-07-28, MAJOR-adjacent) — extending No other actionable threads on this PR. Daily review complete. |
|
Новият комит Изключването на |
cdb0940 to
5815601
Compare
setup.ts and global-setup.ts each inlined ~60 lines of identical SQL helpers and fixture constants. The duplication was a drift hazard — any change to the fixture seed had to be made in lockstep in both files. Move stripSqlCommentsAndCollapse, buildContractsInsert, and the seven FIXTURE_* constants (plus a canonical FIXTURE_STATEMENTS array) into a new helpers/fixtures.ts module that both files now import. Behaviour is unchanged: same SQL emitted, same apply order, same INSERT OR IGNORE semantics. Test count and outputs unchanged: pnpm --filter @sigma/web test still runs 335 unit + 34 integration = 369 tests, all green. Reviewer note: addresses the duplication comment from midt-bg#177 (ydimitrof on c733245).
The merge of main into ralph/web-route-integration brought in new unit tests, shifting the totals: 31 unit files / 335 unit tests, 7 integration files / 34 integration tests, 38 files / 369 tests total. The previous counts in apps/web/test/README.md (30 / 284, 37 / 318) were stale. Also drop a note that fixture declarations and SQL helpers now live in helpers/fixtures.ts (shared between setup.ts and global-setup.ts), so future maintainers editing the seed know where the source of truth is. Reviewer note: addresses the README/PR-description drift comment from midt-bg#177 (ydimitrof on e5e7cf7, repeated on c733245 after the main merge).
…omes, drop redundant globalSetup Nine review threads on the integration-test lane (PR midt-bg#177): T-002 — contracts-csv "cheater" 200||500 assertion. The disjunction passed even if /contracts.csv always 500'd in production. Gate the expected outcome on the build mode (import.meta.env.DEV): DEV asserts the documented devalue 500; a prod/pre-built lane asserts the 200 contract. A status outside the mode's expectation now fails loudly instead of being tolerated. T-003 — redundant vitest globalSetup. global-setup.ts booted a proxy, ran migrations, seeded fixtures, then disposed — but vitest runs each test file in its own worker, so globalThis.__SIGMA_PROXY__ was not visible to tests (setup.ts already bootstraps per-worker). Removed global-setup.ts, unwired it from the config, and updated setup.ts / fixtures.ts / README / sibling test comments to reflect per-worker lazy bootstrap as the only path. T-004 — stripSqlCommentsAndCollapse corrupted string literals. The per-line `--` strip ran before the string-aware split, so `'a--b'` became `'a`; and collapse-whitespace mangled `'a b'` → `'a b'`. Rewrote as a single string- aware char scanner (comment strip + statement split + whitespace collapse all honour in-string state). Added helpers/fixtures.test.ts (7 tests) covering good/bad paths including the two regression cases. TDD: failing tests first. T-005 — duplicated server.deps.inline. Defined identically at top-level `server` (Vite dev-server, unused by `vitest run`) and `test.server`. Removed the top-level copy with an explanatory comment. T-006 — dead exclude config. The exclude list targets paths the include glob never matches. Kept it as a defensive safety net with a comment explaining why (it blocks accidental double-runs if `include` is ever widened). T-007 — comment/regex mismatch in compareSemverDesc. The comment described a `(peer-deps-hash)` parens flavour that does not occur in pnpm store dir names (only in resolved package.json deps); the actual store dirs use plain semver or `_`-delimited peer-dep suffixes. Rewrote the comment to describe the real formats. T-008 / T-009 — missing trailing newline in routes.test.ts and sitemaps.test.ts. Added. T-010 — rate-limit 500 masking. assertCsvNonRateLimitedResponse accepted any 500 whose body matched the devalue text, which could mask a real regression with the same shape. Added a hard `not.toBe(429)` floor (rate-limit leak fails loudly regardless of body), gated the 500 acceptance on import.meta.env.DEV, and linked the tolerance to the ADR-0002 deferred item. Validation: integration lane 8 files / 41 tests pass (was 7 / 34, +7 new fixtures tests); unit lane 31 files / 335 tests pass; `pnpm typecheck` exit 0.
…p version (T-001) PR midt-bg#177 review T-001 (non-blocking): the `.find()` fallback picked the first @opentelemetry/api store entry after a descending semver sort. If pnpm ever hoists two versions, that can differ from the version the app actually imports, silently aliasing the wrong build/esm. Extract the store-walking logic into a pure, unit-tested helper `pickOtelStoreEntry` that prefers an EXACT match on the version the app declares in package.json (stripping semver range operators and ignoring the `_…` peer-dep hash), falling back to the highest semver when the app version is absent. Both branches are deterministic. TDD: 7 tests covering empty store, exact match (incl. peer-dep hash suffix), fallback to highest semver, determinism under input reordering, and ignoring unrelated @opentelemetry/* packages. Integration lane 9 files / 48 tests pass; typecheck exit 0.
Vitest project alongside the unit suite that boots the real SSR Worker pipeline (workers/app.ts) through wrangler.getPlatformProxy() with seeded D1 + Cache + the four RateLimit bindings. Adds an appFetch(request) helper plus header-contract assertions (security headers, Content-Type, Cache-Control, X-Edge-Cache, Retry-After on 429, Content-Disposition on CSV). Covers /search, /companies, /authorities, /contracts, /contracts/:slug, /contracts/:slug.json, /contracts.csv, the four /sitemap*.xml routes, and /robots.txt, including a real CSV rate-limit burst (11th request from a fixed CF-Connecting-IP → 429) and a midt-bg#87 keyset pagination regression for /contracts?cursor=…. ADR at docs/spec/integration-testing.md documents the harness choice and tradeoffs vs. unstable_dev / hand-rolled Node fetch / per-route handler tests; runbook at apps/web/test/README.md. pnpm --filter @sigma/web test → 318 passed (284 unit + 34 integration); pnpm --filter @sigma/web typecheck → exit 0.
setup.ts and global-setup.ts each inlined ~60 lines of identical SQL helpers and fixture constants. The duplication was a drift hazard — any change to the fixture seed had to be made in lockstep in both files. Move stripSqlCommentsAndCollapse, buildContractsInsert, and the seven FIXTURE_* constants (plus a canonical FIXTURE_STATEMENTS array) into a new helpers/fixtures.ts module that both files now import. Behaviour is unchanged: same SQL emitted, same apply order, same INSERT OR IGNORE semantics. Test count and outputs unchanged: pnpm --filter @sigma/web test still runs 335 unit + 34 integration = 369 tests, all green. Reviewer note: addresses the duplication comment from midt-bg#177 (ydimitrof on c733245).
resolveOtelEsmRoot's pnpm-store fallback picked the first matching @opentelemetry+api@* entry via Array.find. If the store ever hoists more than one version of @opentelemetry/api, .find() takes whichever the readdir returned first — which depends on inode order and is not stable across reinstalls. Sort the candidate entries by semver (descending) before .find() so the resolution is fully deterministic: same input, same output, every time. The pnpm-specific <semver>(<peer-deps-hash>) suffix is stripped before numeric comparison; tie-breaks in the suffix are not exercised by the current single-hoist install. The primary require.resolve path is unchanged and covers the normal case; this only affects the fallback.
The merge of main into ralph/web-route-integration brought in new unit tests, shifting the totals: 31 unit files / 335 unit tests, 7 integration files / 34 integration tests, 38 files / 369 tests total. The previous counts in apps/web/test/README.md (30 / 284, 37 / 318) were stale. Also drop a note that fixture declarations and SQL helpers now live in helpers/fixtures.ts (shared between setup.ts and global-setup.ts), so future maintainers editing the seed know where the source of truth is. Reviewer note: addresses the README/PR-description drift comment from midt-bg#177 (ydimitrof on e5e7cf7, repeated on c733245 after the main merge).
…omes, drop redundant globalSetup Nine review threads on the integration-test lane (PR midt-bg#177): T-002 — contracts-csv "cheater" 200||500 assertion. The disjunction passed even if /contracts.csv always 500'd in production. Gate the expected outcome on the build mode (import.meta.env.DEV): DEV asserts the documented devalue 500; a prod/pre-built lane asserts the 200 contract. A status outside the mode's expectation now fails loudly instead of being tolerated. T-003 — redundant vitest globalSetup. global-setup.ts booted a proxy, ran migrations, seeded fixtures, then disposed — but vitest runs each test file in its own worker, so globalThis.__SIGMA_PROXY__ was not visible to tests (setup.ts already bootstraps per-worker). Removed global-setup.ts, unwired it from the config, and updated setup.ts / fixtures.ts / README / sibling test comments to reflect per-worker lazy bootstrap as the only path. T-004 — stripSqlCommentsAndCollapse corrupted string literals. The per-line `--` strip ran before the string-aware split, so `'a--b'` became `'a`; and collapse-whitespace mangled `'a b'` → `'a b'`. Rewrote as a single string- aware char scanner (comment strip + statement split + whitespace collapse all honour in-string state). Added helpers/fixtures.test.ts (7 tests) covering good/bad paths including the two regression cases. TDD: failing tests first. T-005 — duplicated server.deps.inline. Defined identically at top-level `server` (Vite dev-server, unused by `vitest run`) and `test.server`. Removed the top-level copy with an explanatory comment. T-006 — dead exclude config. The exclude list targets paths the include glob never matches. Kept it as a defensive safety net with a comment explaining why (it blocks accidental double-runs if `include` is ever widened). T-007 — comment/regex mismatch in compareSemverDesc. The comment described a `(peer-deps-hash)` parens flavour that does not occur in pnpm store dir names (only in resolved package.json deps); the actual store dirs use plain semver or `_`-delimited peer-dep suffixes. Rewrote the comment to describe the real formats. T-008 / T-009 — missing trailing newline in routes.test.ts and sitemaps.test.ts. Added. T-010 — rate-limit 500 masking. assertCsvNonRateLimitedResponse accepted any 500 whose body matched the devalue text, which could mask a real regression with the same shape. Added a hard `not.toBe(429)` floor (rate-limit leak fails loudly regardless of body), gated the 500 acceptance on import.meta.env.DEV, and linked the tolerance to the ADR-0002 deferred item. Validation: integration lane 8 files / 41 tests pass (was 7 / 34, +7 new fixtures tests); unit lane 31 files / 335 tests pass; `pnpm typecheck` exit 0.
…p version (T-001) PR midt-bg#177 review T-001 (non-blocking): the `.find()` fallback picked the first @opentelemetry/api store entry after a descending semver sort. If pnpm ever hoists two versions, that can differ from the version the app actually imports, silently aliasing the wrong build/esm. Extract the store-walking logic into a pure, unit-tested helper `pickOtelStoreEntry` that prefers an EXACT match on the version the app declares in package.json (stripping semver range operators and ignoring the `_…` peer-dep hash), falling back to the highest semver when the app version is absent. Both branches are deterministic. TDD: 7 tests covering empty store, exact match (incl. peer-dep hash suffix), fallback to highest semver, determinism under input reordering, and ignoring unrelated @opentelemetry/* packages. Integration lane 9 files / 48 tests pass; typecheck exit 0.
…tstrap from env.DB scan After upstream's migration 0002 added contracts.current_value_currency (read by getContract → packages/db/src/queries/details.ts), the integration test proxy only loaded migrations 0000 and 0001. The local D1 therefore lacked the column every contract-route loader reads, and tests hitting /contracts/:id or /contracts/:id.json returned 500 instead of 200/404. Apply 0002 in setup.ts. The read-only D1 chokepoint guard (apps/web/app/lib/readonly-db-chokepoint.test.ts midt-bg#199/midt-bg#225) forbids env.DB in any web source. test/integration/setup.ts must use proxy.env.DB.exec() to apply migrations — schema admin, not application data access, and only runs inside the vitest integration config (not the deployed Worker). Exempt that single file from the scan with a rationale comment so the chokepoint stays hermetic for everything else.
…lint Eleven files in the PR's test/config surface had pre-existing prettier debt that the upstream prettier version (3.8.3) flags: the integration test files, the vitest integration config + workspace, and the root README. Same content, whitespace only. The lint gate is blocking on these (AGENTS.md / repo CI), so this is non-optional for merge. Verified: pnpm typecheck, pnpm --filter @sigma/web test → 530 passing (52 files, 8 integration files, 41 integration tests), pnpm lint clean.
5815601 to
52cf093
Compare
|
Daily autonomous review — rebase pass. No new reviewer activity since 2026-08-05. All 18 review threads remain Branch was Approach. Local rebase of Verification (local).
State. No new commits to PR surface (only upstream drift). Open thread tracking unchanged from 2026-08-05: 0 unresolved threads. Coordination follow-up from lyubomir-bozhinov (2026-07-28) for extending |
4 participants
Какво и защо
Този pull request добавя интеграционен тестов стек за
apps/web, който валидира реалния SSR Cloudflare Worker (workers/app.ts) чрез Wrangler/Miniflare, а не само изолирани unit тестове с мокове. Целта е критичните публични маршрути, логиката за кеширане и сигурност, ограничаването на честотата на заявките и поведението на пейджинг с ключови множества да бъдат проверявани по реалния път на заявките преди сливане на промените.Потребителска история
Като поддържащ СИГМА, искам CI да улавя регресии в реалните уеб маршрути — например неправилни хедъри за сигурност/кеширане, счупено ограничение на честотата на заявките за CSV, отклонения в пейджинга на
/contracts(#87) или проблеми с отговорите на sitemap/robots — без да се налага ръчна проверка или имитация на продукционна среда.Детайли по имплементацията
apps/web/vitest.integration.config.ts) и конфигурация за работната среда (apps/web/vitest.workspace.ts), така чеpnpm --filter @sigma/web testда изпълнява едновременно unit и интеграционни тестове.apps/web/test/integration/с помощна функцияappFetch(request), лениво зареждане на реалния Worker и настройка наwrangler.getPlatformProxy()за предварително заредени D1, Cache и RateLimit биндинги.Content-Type,Cache-Control,X-Edge-Cache,Retry-Afterпри статус 429 иContent-Dispositionза CSV файлове./search,/companies,/authorities,/contracts,/contracts/:slug,/contracts/:slug.json,/contracts.csv,/sitemap.xml,/sitemap-pages.xml,/sitemap-contracts.xml,/sitemap-companies.xml,/sitemap-authorities.xmlи/robots.txt.CF-Connecting-IPвръща коректен429съсRetry-After./contracts?cursor=…, който проверява стабилността на втория резултатен сет през публичния маршрут.docs/spec/integration-testing.md, инструкции за изпълнение вapps/web/test/README.md, както и препратки вREADME.mdиdocs/README.md.Свързан issue
Затваря #94. Покрива и проверка за регресия, свързана с #87.
Вид промяна
Как е тествано
pnpm --filter @sigma/web test— 318 успешни теста (284 unit + 34 integration), 0 неуспешни. Потвърдено стабилно при три последователни изпълнения.pnpm --filter @sigma/web test:unit— 284 успешни теста, 0 неуспешни.pnpm --filter @sigma/web test:integration— 34 успешни теста, 0 неуспешни.pnpm --filter @sigma/web typecheck— изходен код 0 (wrangler types && react-router typegen && tsc -b).Чеклист
Co-Authored-By:трейлърmidt-bg/sigma:mainpnpm --filter @sigma/web typecheckминава успешноpnpm --filter @sigma/web testминава успешноpnpm lintе чист (не е изпълняван отделно в този цикъл).env*или.dev.varsфайловеdocs/е актуализиранаdiscord: lubakmanqk