test: тестово покритие ≥95% за всички workspaces (надгражда #216) - #254
test: тестово покритие ≥95% за всички workspaces (надгражда #216)#254lyubomir-bozhinov wants to merge 48 commits into
Conversation
Vendored from midt-bg#216 (feat/coverage-ratchet) to put the coverage machinery in place ahead of midt-bg#217, while midt-bg#216 is pending merge upstream. Squashes ydimitrof's two harness commits into one; original authorship preserved via the commit author. @vitest/coverage-v8 through a shared vitest preset, per-workspace coverage configs with explicit include, committed coverage-baseline.json, the scripts/check-coverage.mjs ratchet gate (+ node:test self-test), and the CI wiring. Ref midt-bg#93, midt-bg#216.
regionByName had zero tests (both lines uncovered). Adds its full branch matrix (valid, whitespace-trim, null/undefined/empty/unknown, verbatim round-trip of all 28 regions) plus edge inputs for categoryForDivision and procedureGroup, and taxonomy-integrity invariants (unique CPV codes, single procedure-type ownership, classified = competitive∪non-competitive, 28 unique NUTS3 regions). config: 92.85/72.22 -> 100/100 lines/branches.
… periodRange) Adds eik/unp passthrough, one-sided and empty periodRange, date/monthYear/ longDate no-match + datetime-prefix + out-of-range-month fallbacks, count sign/absence, pct/signedPct dp + non-finite, entityName non-collapsing paths, cleanName unbalanced-quote drop, ЕТ/ET latin detection. shared branches 78.1 -> 98.3; residual is signedPct's provably-unreachable defensive return. Caught: count(-0.4) emits '−0' (missing money()'s rounded-zero sign guard); latent only (count takes non-negative integers), logged not patched.
New staging.test (0%->100%): scoped DELETE, chunked INSERT at CHUNK=100 boundaries (100/101/250 rows), null-fill of absent columns, table+column routing per target. New refresh.test: SQL splitter (escaped '' in literals, -- comments inside/outside literals, trailing statement), @refresh-batch grouping, transient-table drop order, D1 orchestration. base/ocds additions: toBool, Date.parse date fallback, annexes mapping, baseSqlLiteral numeric/ text/null branches, secured_inverse/variants coercions, party/lot/amendment and catch-up-window branches. ingest lines 83.8->100, branches 78.9->95.1.
…dology, flows identity slug fallbacks + undecodable name slug; keyset decodeCursor malformed/oversized/bad-type-guard paths; home zero single-offer aggregate; regions year + EU/national funding predicates; methodology absent-count coalesce with positive total; flows sankey sort tiebreak on shared authority.
streamAuthority/Company/Contract sitemaps via a paginating fake D1: XML escaping + C0 stripping, lastmod fallbacks (row date -> as_of -> none), natural-person filtering, empty-chunk skip loop, CHUNK-boundary pagination, contract page rowid windowing, and contractSitemapPages math. sitemaps branch 34.9 -> 95.3; residual is the defensive post-close pull guard.
getCompany + getAuthority were entirely untested. Adds DTO assembly, share math (won/spent denominators, zero-guards), avg-bids rounding, consortium membership (list->participants, prose->note), hasEik, sector top6+tail rollup, and getContract subcontractor (EUR/BGN/null/blank), framework call-off detection, eurFromNative currency paths (EUR/BGN peg/FX/no-rate), deltaPct suspect+zero-base guards, lot dedup/totals, and not-found. details branch 43.6 -> 88.5, lines 53 -> 100.
Adds the company-centre direction, null-param default (top authority) + its empty fallback, includeCenterOptions=false, loadCenter sample-name fallback (authority + company), hop-2 top-1-per-neighbour dedup, centre self-skip, and edgeless-node weighting. network branch 50 -> 88, lines 86.7 -> 100.
… guards search: empty/punctuation query -> empty shape, searchMoreHref unknown-kind fallback. trend: YoY guarded against a zero prior year, coverage pct when nothing is dated (no divide-by-zero).
Add coverage for the base-aggregation source (year/EU/single- vs multi-sector primary_sector), the entity WHERE type/text filters, sort normalization, facet label fallback and sort, page overflow, and the CSV stream body across the CHUNK boundary. Branch 55%->96%, lines 100%.
Add coverage for sort normalization, the base-aggregation source (year/EU/ single- vs multi-sector), the kind/text entity WHERE, facet kind mapping and sector sort, page overflow, missing total row, and the CSV body across the CHUNK boundary. Branch 61%->95%, lines 100%.
Add coverage for the authority-detail wrappers (getAuthoritySingleOffer, getAuthorityProcedureCompetition), getCompetitionSummary (both the qualifying and null-topConcentration paths), the MAX_TOP cap, EU/national funding scope, and a degenerate corpus exercising the zero-guard fallbacks. Branch 70%->95%.
Add coverage for buildFilters (every year/sector/procedure/value-bucket/EU/ bids/authority/bidder/text predicate), summary override, page overflow, contractsSummary null row, listSingleOfferContracts modes, facet procedure folding / sector sort / year ordering, and the streamed CSV body across the CHUNK boundary. Branch 66%->96%, lines 100%.
Add degenerate-input coverage for getCompany (absent metadata/bids/suspect rows, null primary sector and procedure value), getAuthority (spent-nothing authority with an unknown CPV division, zeroed tail share, null bids/suspect), and a getContract lot defaulting to the BGN peg. Branch 88%->98%, lines 100%.
Add coverage for an unresolvable company centre (null name → empty network), the company-kind fallback when neither rollup nor sample carries a kind, the empty-default path with includeCenterOptions off, and deduping a hop-1 neighbour that appears twice. Branch 88%->98%, lines 100%.
flows: EU/national/all funding scope + long-label truncation (branch 89->100). search: nullish raw-query coalescing (branch ->97). trend: EU/national funding, includeSectors=false, and an empty series with an absent coverage row (branch 89->100).
index.ts: full RefreshWorkflow.run coverage (staging lifecycle, capped and zero-ingest branches, derive-slice loop, finally-drop on success and error) and the scheduled cron entrypoint, via mocked platform/ingest/eop seams. eop.ts: bucket-key parse/classify, catch-up planning (uncapped/capped/default), bucket listing status + redirect guard, OCDS/base staging, and the window walk. Workspace 19%->98% branch, lines 100%.
Add coverage for singleSelectFilters (unknown sector/year flags, funding/top defaults), buildSectorGroup (category grouping, summed vs absent counts, uncategorised skip), sortHref, and the withParams/pageNav null-override, array, empty-result, and page-default branches. filters.ts branch 47%->97%.
agent.ts: SDK-wiring coverage (model/base-URL resolution, tool-set assembly, stream Response + onError) via mocked ai/@ai-sdk. report-schema.ts: flows block, facts sub-line, unknown-handle, empty title, 0-row and null chart edges (branch 78->94). tools.ts: run_sql AST-reject/error/meta-less paths, semantic hits, eop_fetch, source_link (branch 57->95). Also exclude test fixtures and type-only declarations from coverage (permanent 0% data files, not code).
render-format null-date; tool-results missing-cell + truncation flag; eop-fetch null-date/non-array/invalid-JSON/thrown-fetch; rag embed mismatch + metadata mapping + empty-vector guards; validateEmitShape callout/flows/timeseries and the object/question/items/columns negatives.
Add coverage for the v0 freshness fallback, non-string/null q classification, empty and empty-chunk multipart bodies, the abort-on-part-failure path, and suffix (bytes=-N) / open-ended (bytes=A-) R2 range shapes (the fake now emits R2-native range objects). csv-export lines 87->95, branch 76->90.
The v8 provider instruments every file matched by a workspace's include globs. Markdown docs colocated in src (e.g. the assistant README) carry no coverable statements, report a permanent 0%, and — being non-JS — make the reporter's remap step throw a parse error. Exclude **/*.md alongside the existing JSON and type-only exclusions so the ratchet total reflects executable code only.
Cover the remaining thin spots in the web workspace with real behavioural tests, no code changes: - cache.publicCache: default and explicit stale-while-revalidate windows - eopSource: missing/malformed dates, DD.MM.YYYY key shape, OCDS cutoff boundary - search.suggest: trimGroup cap + loader query/trim/headers, empty-q default - app.ts hardening: nonce re-read path, OPTIONS short-circuit, no-Content-Type - retry: non-Error rejection logging + default backoff past the table - security: nonce-less headers omit the CSP outside production - riskLogic: unknown bid count (null) and missing bidsRejected fallback - ScrollToTop: rAF coalescing of a scroll burst Web branch total 81.3% -> 95.47%, lines 89.3% -> 99.25%.
With the new tests in place, every workspace clears 95% on both lines and branches. Regenerate the ratchet baseline from current coverage so the gate now enforces the 95 floor going forward: etl 18.7/19.4 -> 100/98.5 web 89.3/81.3 -> 99.2/95.4 config 88.2/58.3 -> 100/100 db 82/65.5 -> 100/97.3 ingest 83.8/78.9 -> 100/95 shared 94.8/78.1 -> 98.8/98.3
buildSectorGroup always returns categories, but the group type marks it optional. vitest transpiles without type-checking so this passed locally; tsc --noEmit under noUncheckedIndexedAccess (CI typecheck) rejected the possibly-undefined access. Assert non-null at the three call sites.
Add branch-completion tests for releaseToContracts/Amendments/Lots on releases with absent optional fields: missing tag/contracts keys, id-less and identifier-less parties, a scheme-less CPV classification, an empty-string value amount, a blank date, and an ocid/tender-id-less lot. ocds.ts branch 92.17% -> 98.26%, ingest workspace 95.06% -> 98.7%. Residual uncovered branches are unreachable defensive code: the validDateOnly regex reject (day is always pre-normalized to YYYY-MM-DD) and the `rel.contracts ?? []` / `c.id ?? null` right-sides the length/id guards above them make impossible.
- keyset: unsafe-direction guard, before-cursor with an ascending sort, and the empty before-page (both cursors null) - flows: two-authority sankey so the authority-column sort comparator runs - regions: empty dataset → the total==0 coverage-pct guard (no divide-by-zero) - companies: base aggregation from a non-sector filter (no CPV predicate) - contracts: the „Неизвестна" year bucket sinking below real years regardless of input order - authorities/companies/contracts: backward pagination — page forward for a cursor, back for a before-cursor, then feed it back so keyset's reverse path runs db branch 97.35% -> 98.39%. Residual gaps are unreachable defensive code: CSV/sitemap `if(done)` re-entry (a stream never pulls after close), the minContracts `?? DEFAULT` the orchestrator already normalises, split().pop() `?? ` fallbacks (pop is always defined), the homoglyph map (every regex-matched char is mapped), and cross-namespace network self-edges.
- assertReadOnlySelect: empty/comment-only query, a forbidden keyword hidden in a single CTE-prefixed statement (cheap keyword layer, not just the AST guard), and the sqlite_master/sqlite_schema catalog-table rejection - run_sql: a driver returning no results array (the results ?? [] fallback) - eop-fetch: a non-Error thrown value → the generic fetch-error label web branch 95.47% -> 96.0%. Residual gaps are deep AST-shape defenses (sql-ast-guard), schema-validation guards (report-schema), and unreachable code: regex capture-group ?? fallbacks (csp, always matched), PROD-gated redirect/OPTIONS paths under vitest, and the module-init Date.now tag.
Coverage rose across web/db/ingest with the new branch tests; raise the ratchet floors to match (never down): web 99.2/95.4 -> 99.4/96 db 100/97.3 -> 100/98.3 ingest 100/95 -> 100/98.7 etl (100/98.5), config (100/100) and shared (98.8/98.3) unchanged. Monorepo total 99.73% lines / 97.51% branches.
|
@ydimitrof @todorkolev PR-ът е синхронизиран с текущия main и CI е зелен — готов за повторен преглед. Състояние: merge-нат е целият main до момента — #216 (coverage harness), #263 (FX в cron), #252 (Булстат ЕИК), #210 (сходни договори), #226 (свързани лица), #281/#282. Клонът е на 0 зад upstream. И двата workflow-а минават: Какво се промени след последния преглед на @ydimitrof:
Единственото сваляне на праг в целия PR: Финално: gate зелен за всичките 6 workspace-а, 1348 workspace теста + 51 scripts + 84 cacbg, typecheck 7/7. Merge редът вече е без значение — #216 е merge-нат, PR-ът е върху него. |
|
Този клон е в конфликт с |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщен преглед на PR: „test: тестово покритие ≥95% за всички workspaces (надгражда #216)“
Какво прави PR-ът
PR-ът добавя и разширява unit тестове с цел покритие ≥95% за всички workspaces — apps/etl, apps/web, packages/db, packages/ingest, packages/config, packages/shared — заедно със спомагателна конфигурация за покритие (coverage-baseline.json, vitest.shared.ts). Прегледан е в 5 партиди. Не се променя продукционен код — обхватът е изцяло тестове и конфигурация, което ограничава риска до качеството на самите тестове и на праговете за покритие.
Сигурност (Phase 0): ЧИСТО
Във всички партиди няма зашити тайни (стойности като BGGPT_API_KEY: 'k', abc123nonce, IP от документационния диапазон TEST-NET-3 са очевидно фиктивни), няма нови изходящи URL адреси (единственият външен адрес https://storage.eop.bg/... отразява вече съществуващ продукционен източник), няма нови зависимости и няма зловредни/обфускирани шаблони. Няма блокиращи находки.
Силни страни
- Смислени тестове, не „измамни“. Тестовете целят конкретни клонове и асертват поведение, а не тривиално преминаване — напр. коалесциране на скролове в точно един
requestAnimationFrame,staged: 28с реално покрити събираеми, точно съответствие наBACKOFF_MSвretry.test.ts, пълниtoEqualформи вnetwork.test.ts, chunking математиката вstaging/refresh, гранични случаи за дати въвformat.test.ts. - Коректно почистване на ресурси и детерминизъм. Последователно
afterEach/try-finallyвъзстановяване на глобални стойности, пинване на часовника (vi.setSystemTime), fake timers — без изтичане на стейт и без flakiness. - Добро покритие на гранични, негативни и защитни клонове — включително сигурностните клонове на read-only SQL guard-а.
- Съответствие с CLAUDE.md — атомични, добре именувани, обяснителни коментари; без мъртъв код, дублиране или TODO-та.
Несблокиращи забележки (за обмисляне)
- Крехкост на праговете за покритие. В
coverage-baseline.jsonпраговете заlinesса вдигнати до100приtolerance: 0.5. Това е крехко — всеки бъдещ PR (включително извън обхвата на тази задача), който добави дори един непокрит ред вapps/etl/packages/config/packages/db/packages/ingest, ще падне под 99.5 и ще счупи CI. Препоръчва се малко по-щедра толерантност или прагове малко под 100 за оперативен буфер. - Изключвания в
vitest.shared.ts. Стесняването на знаменателя (**/fixtures/**,**/src/test/**) частично допринася за метриката ≥95% — легитимно, но заслужава да се отбележи.**/*.mdи**/*.jsonса практически излишни и могат да се премахнат за яснота. - Крехкост на фалшивите D1 обекти. Няколко fake-а маршрутизират чрез подниз-съвпадения (
sql.includes(...)), които са чувствителни към бъдещи промени в SQL текста — при рефакторинг тези тестове може тихо да върнат грешен клон. - Тестове на „защитни/недостижими“ клонове. Приемливо за defensive fallback-и, но граничи с преследване на покритие — струва си да се потвърди, че самите защитни клонове са реално нужни, а не мъртъв код.
- Дребни подобрения на асершъни. В
tools.test.tssource_linkпроверката/https?:\/\//е слаба — по-добре конкретен очакван дълбок линк; вemit-report-schema.test.tsда се потвърди, чеquestion: ''е умишлено валиден.
Заключение
Висококачествен, фокусиран PR с чисто тестово покритие и без установени дефекти или проблеми със сигурността. Няма блокиращи концерни. Пълната абсолютна оценка за покритие ≥95% зависи от CI резултата, тъй като продукционните файлове не са част от дифа. Забележките са несблокиращи; основната препоръка за проследяване е крехкостта на праговете в coverage-baseline.json.
Общ вердикт: Одобрявам (Approve).
| "apps/etl": { | ||
| "lines": 74, | ||
| "branches": 58.2 | ||
| "lines": 100, |
There was a problem hiding this comment.
Прагът за lines е зададен на точно 100 (при tolerance: 0.5). Това означава практически нулев буфер: следващ PR, който добави дори един непокрит ред в този workspace, пада под 99.5 и чупи CI — включително PR-и, несвързани с тази задача. Обмислете прагове малко под 100 или по-щедра толерантност за оперативен запас.
There was a problem hiding this comment.
Тук ще откажа, съзнателно. Праговете в този репозиторий вървят само нагоре — свалянето на праг, за да мине CI, е точно навикът, който ratchet-ът съществува да спре, а „оперативен запас" е името, което това си слага първия път.
Буферът, който описваш, вече го има: tolerance: 0.5 е точно това — праг 100 приема 99.5 без да чупи. Втори буфер върху първия прави гейта декоративен.
За PR, който добавя един непокрит ред в такъв workspace: това е правилният момент да се напише тест за него, а не да се разхлаби прагът. При тази вълна от upstream (20 коммита) точно това направих — packages/db падна под прага си и е върнат с тестове, не с нов baseline.
| // Test-support helpers (SQLite D1 shims, cloudflare:workers/workflows stubs) live under src/test/. | ||
| // They exist only to drive the suites — instrumenting them measures test scaffolding, not product | ||
| // code, so they belong with fixtures on the exclude list. | ||
| '**/src/test/**', |
There was a problem hiding this comment.
Изключването на **/src/test/** и **/fixtures/** от покритието е разумно за тестова инфраструктура. Само забележка по прозрачност: тъй като целта на PR е ≥95%, част от повишението идва от стесняване на знаменателя, а не само от нови тестове — струва си да се спомене в описанието на PR/CHANGELOG.
There was a problem hiding this comment.
Честна бележка — измерих я, за да не остане на думи. Единственият workspace, който **/src/test/** въобще мести, е packages/ingest:
| с изключването | без него | |
|---|---|---|
| ingest lines | 100% | 98.97% |
| ingest branches | 98.69% | 98.33% |
Другите пет са непроменени до последната цифра. Тоест стесняването на знаменателя струва ~1pp върху един workspace, и дори без него всичките шест остават над 95% — целта на PR-а не зависи от изключването. Ще го запиша в описанието на PR-а.
Това, което излиза от знаменателя, е packages/ingest/src/test/d1-sqlite.ts — SQLite шим, който съществува само за да върти сюитите.
| // permanent 0% that drags the workspace total without any coverable statement (e.g. the | ||
| // assistant JSON fixtures). | ||
| '**/fixtures/**', | ||
| '**/*.json', |
There was a problem hiding this comment.
**/*.json и **/*.md тук са на практика излишни — инструментирането за покритие се прилага само върху изпълними модули, които реално се импортират, така че markdown/JSON никога не влизат в отчета. Може да се премахнат за по-чист списък (без функционален ефект).
There was a problem hiding this comment.
Прав си — премахнати. Проверих го емпирично, вместо да разсъждавам: махнах **/*.md и **/*.json и пуснах покритието на всичките шест workspace-а — числата са байт по байт същите (etl 100/97.64, web 99.47/96.21, config 100/100, db 100/98.47, ingest 100/98.69, shared 98.87/98.33).
Проверката си струваше, защото include е гола директория (app/**, src/**), а не *.ts, и под app/** наистина живее app/lib/assistant/README.md. Дори тогава не влиза в отчета — провайдърът докладва само това, което може да инструментира като модул. Записах точно това в коментара на мястото на двата glob-а. 6971a09
…ve, поименни връзки midt-bg#309) Third sync onto upstream/main (20 commits). Resolved four conflicts by union, keeping both sides' tests in every case: - apps/etl/src/eop.test.ts — our orchestration suite plus upstream's stream-cancel tests (midt-bg#284); the openBodyResponse helper moves under our import block. - apps/web/app/lib/conflicts.test.ts — our temporalLabel/timeline/authorityShares cases alongside upstream's registryEvidenceLabel wording tests (midt-bg#309). - packages/ingest/src/ocds.test.ts — union of the daysInWindow and fullDeriveIsSafe imports. - coverage-baseline.json — ours; every floor is strictly higher than upstream's. No coverage floor was lowered. Two upstream changes moved coverage and were met with tests rather than a looser baseline: - refresh.test.ts asserts the drop order, which now carries the derive-step scratch table amendment_contract_resolve (midt-bg#306) as its last entry. - packages/db branch coverage fell below its floor on the new poименни-връзки and contract-detail code. Restored to 98.47% (floor 98.3) and lines to 100% with tests for: registry_role narrowing to the two rungs the card can render, the ordering-unit vs authority fold, personSlug on a prefix-less key, getDb as the read-only chokepoint, and assertReadOnlyExec's empty-statement refusal. Also fixes a stale fixture in search.test.ts: the свързани-лица probe returned n:1 while the source requires n===2, so the "table present" mode silently exercised the un-migrated fallback and the conflict-aware hits SQL was never run. The fixture now reports the real count, with tests for the partial-migration and no-row cases and for a group whose count and hits disagree. apps/web regained its floor with tests for the conflict pages' cache headers and the methodology page's indexability, the confirmed-seal provenance line, a timeline with no declared-period band, and Pagination's disabled end-of-range state. Every new test was mutation-verified: the production line it covers was broken and the test confirmed failing. HITL-ACK: agent-instructions-self-mod AGENTS.md arrives from upstream (midt-bg#296/midt-bg#297, the Co-Authored-By trailer rule), not authored here — a merge cannot omit it.
|
@nedda76 Готово — клонът е слят с актуалния Четирите конфликта са разрешени по union — и двете страни са запазени, нищо не е изхвърлено:
Нито един праг не е свален. Два от upstream промените мръднаха покритието и са посрещнати с тестове, не с по-хлабав baseline:
Покрай това — намерен и поправен изтекъл fixture в
Всеки нов тест е mutation-verified: редът в production кода е счупен и тестът е потвърден, че пада. @ydimitrof — сливането е след твоето ревю; съдържателно нищо от бележките ти не е пипано, само са добавени тестове. Ако вердиктът ти стои, може ли да го подадеш и като GitHub Approve? В момента ревюто е |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
You've hit your session limit · resets 5:40pm (Europe/Sofia)
| import { describe, expect, it, vi } from 'vitest'; | ||
| import type { SearchGroup, SearchResults } from '@sigma/api-contract'; | ||
|
|
||
| const { searchMock } = vi.hoisted(() => ({ searchMock: vi.fn() })); |
There was a problem hiding this comment.
Незавършена референция в коментара („upstream #…"). Препоръчвам да попълните номера на PR/issue или да премахнете референцията, за да не остане placeholder в кода. Незадължително — не блокира.
| }); | ||
|
|
||
| it('caps the leaderboard size at MAX_TOP when top=50 is requested', async () => { | ||
| const { scope } = await getCompetition(fakeDb(), { top: 50 }); |
There was a problem hiding this comment.
Тази проверка не тества реално ограничаването до MAX_TOP. Ако MAX_TOP === 50 и лимитът е Math.min(top, MAX_TOP), то при заявка top: 50 резултатът е 50 независимо дали clamp-ът съществува — това е обикновено passthrough, а не тест на клона за капинг. Заглавието „caps ... at MAX_TOP" надценява какво се проверява.
За да се покрие реално ограничаването, поискайте стойност над лимита и очаквайте свеждане до MAX_TOP, напр.:
const { scope } = await getCompetition(fakeDb(), { top: 999 });
expect(scope.top).toBe(50); // clamped down to MAX_TOPВ текущия вид това е близо до „cheater test" по правилата на CLAUDE.md (NO CHEATER TESTS).
There was a problem hiding this comment.
Прав си, че тестът не струваше колкото името си — но не по причината в бележката. Източникът не прави clamp:
const top = p.top === MAX_TOP ? MAX_TOP : DEFAULT_TOP;Това е двузначен toggle, не Math.min: точно 50 избира големия размер, а всичко останало пада на 20. Твоят предложен фикс (top: 999 → очаквано 50) щеше да падне — връща 20.
Проверих и дали старият тест е cheater: не беше. Мутация const top = DEFAULT_TOP го чупи. Но си прав в същественото — името „caps ... at MAX_TOP" описва поведение, което кодът няма, и fallback-ът не се тестваше изобщо, а той е половината, която пази от това повикващият да си избере размера на класацията (и своя LIMIT).
Преименуван и разширен с 999, 51, 35, 0, -1, NaN и липсващ top. Сега убива три мутации, включително Math.min(p.top ?? DEFAULT_TOP, MAX_TOP) — тази, която старият тест щеше да пропусне. Благодаря, бележката хвана реална дупка. 6971a09
| // Test-support helpers (SQLite D1 shims, cloudflare:workers/workflows stubs) live under src/test/. | ||
| // They exist only to drive the suites — instrumenting them measures test scaffolding, not product | ||
| // code, so they belong with fixtures on the exclude list. | ||
| '**/src/test/**', |
There was a problem hiding this comment.
Изключването на **/src/test/** от покритието е обосновано за тестово скеле, но glob-ът е широк: ако бъдещ пакет постави продукционен код под src/test/, той тихо ще излезе от знаменателя на покритието и може да маскира непокрит код. Не блокира — при текущата подредба тук живеят само D1/cloudflare шимове. Струва си коментар/линт правило, което да поддържа src/test/ само за помощници.
There was a problem hiding this comment.
Съгласен — glob-ът е широк, а договорът зад него е тесен, и това никъде не беше записано. Добавен коментар точно на това основание: src/test/ е само за тестови помощници, защото продукционен код там излиза от знаменателя тихо — единственият начин този списък да скрие непокрит модул, а не непокриваем файл.
Не добавям lint правило засега — правило, което различава „помощник" от „продукционен код" по път, ще е или тривиално заобиколимо, или ще лъже. Ревюто на нов запис в src/test/ е по-честият гейт. Ако прецениш, че си струва, ще го вдигна като отделен issue вместо да го натоварвам тук. 6971a09
| bind() { | ||
| return this; | ||
| }, | ||
| async all<T>() { |
There was a problem hiding this comment.
Незначително: customDb.all() връща series за всяка заявка, вкл. sector_totals (includeSectors е по подразбиране включено в този тест). Тестът не асертира sectors, така че поведението е коректно, но fake-ът е по-разхлабен от останалите в файла — ако по-късно добавите проверка на секторите, той ще върне неподходящи редове (period/value_eur вместо division).
There was a problem hiding this comment.
Прав си — fake-ът беше по-разхлабен от съседите си в същия файл. Вече диспачва по заявката: sector_totals връща [], останалото — серията. 6971a09
…g#254) Four findings from ydimitrof's review, verified against the source before acting. competition.test.ts — the „caps at MAX_TOP" test asserted top:50 → 50 and named a clamp the source does not perform. `getCompetition` reads `p.top === MAX_TOP ? MAX_TOP : DEFAULT_TOP`: an exact 50 selects the large size and everything else falls back to 20, so nothing is ever reduced to 50. The old assertion did kill a mutation (collapsing the toggle to DEFAULT_TOP fails it), but it left the fallback — the half that stops a caller naming its own leaderboard size and its own LIMIT — untested, under a name that overstated it. Renamed and extended to cover 999, 51, 35, 0, -1, NaN and an omitted top. Now kills three mutations, including the `Math.min` clamp the old test would have passed. vitest.shared.ts — dropped `**/*.json` and `**/*.md` from the coverage excludes. The provider only reports files it can instrument as modules, so neither ever reached a report; removing both leaves all six workspaces' numbers byte-identical. Recorded that verification in the comment rather than the globs. vitest.shared.ts — documented the contract behind `**/src/test/**`: the glob is wide, so the exclusion holds only while src/test/ stays test HELPERS. Product code placed there would leave the coverage denominator silently, which is the single way this list can hide an untested module rather than an uncoverable file. search.suggest.test.tsx — filled the dangling „upstream #…" placeholder with the real reference (midt-bg#225, the read-only D1 chokepoint for midt-bg#199). trend.test.ts — the local `customDb` returned the period series for every query, including the sector_totals read. Dispatches on the SQL now, like the other fakes in the file, so a later assertion on `sectors` cannot be fed rows of the wrong shape. Coverage unchanged: etl 100/97.64, web 99.47/96.21, config 100/100, db 100/98.47, ingest 100/98.69, shared 98.87/98.33. No floor moved.
|
@ydimitrof Всичките шест бележки са отговорени по нишките, четири са поправени в 6971a09, две са отклонени с аргумент:
Описанието на PR-а е обновено с текущите числа и с това каква част от покачването идва от стесняване на знаменателя (мести само Между другото — тялото на последното ти ревю е излязло като CI е зелено. Ако вердиктът от 20.07 („Одобрявам") стои, ще подадеш ли ревюто като GitHub Approve? В момента и трите са |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщение на прегледа — PR: тестово покритие ≥95% за всички workspaces (надгражда #216)
Какво прави PR-ът
Този PR разширява тестовото покритие до ≥95% за всички workspaces, без промени в продукционен код. Прегледан е на 5 партиди и добавя изключително модулни тестове (*.test.ts / *.test.tsx) плюс две конфигурационни промени: обновяване на coverage-baseline.json (по-високи прагове) и разширяване на изключенията в vitest.shared.ts. Покрити са:
- оркестрация и control-flow клонки, retry/backoff логика, config таксономия (CPV сектори, NUTS/области);
- заявките в
packages/db/src/queries— keyset/cursor пагинация, изграждане на SQL филтри, CSV стрийминг, HHI/share сметки с защита от деление на нула, prototype-key защита; - render/UI поведение (
ScrollToTop, форматиране, CSP nonce→hash), R2/D1 фалшиви двойници с вярна семантика, sitemaps/staging/refresh/ocds пътища.
Сигурност (Фаза 0) — ЧИСТО във всички партиди
Няма твърдо кодирани тайни (стойностите като apiKey: 'k', eik:111111111, abc123nonce са очевидни фикстури; IP-тата са RFC 5737 документационни). Няма нови зависимости, няма нови недоверени URL-и (само вече одобрените storage.eop.bg, api.bggpt.ai). Няма злонамерени шаблони. Тестовете дори затвърждават security контроли: redirect allowlist, rel="noopener noreferrer", read-only SQL guard, маскиране на суровите D1 грешки.
Качество на тестовете — високо
Тестовете са смислени и целят да разкриват дефекти, а не да минават тривиално: почти всеки случай документира коя клонка/мутация покрива (напр. числено сортиране „1, 2, 10", peg BGN→EUR 1.95583, signed_at ?? published_at ?? as_of приоритет, инвертиране на посоката при курсор). Гранични случаи (празни набори, липсващи rollup редове, враждебни курсори, немигрирала среда) са добре покрити. Cleanup е овладян (vi.unstubAllGlobals, vi.useRealTimers в finally). Съответствие с CLAUDE.md: без частична имплементация, без TODO, без мъртъв код, без cheater-тестове.
Точки за човешко потвърждение (неблокиращи, но важни)
vitest.shared.ts— знаменател на покритието (партида 5): разширяването на изключенията (**/src/test/**,**/fixtures/**,**/*.d.ts) намалява знаменателя. В PR с изрична цел ≥95% това заслужава преглед — да се потвърди, чеsrc/test/съдържа само тестови помощници, а не продуктов код, за да не се „постигне" число без реално тестване.coverage-baseline.json(партида 2): праговете се вдигат агресивно (100% редове за няколко workspaces приtolerance: 0.5). Уверете се, че всички партиди реално достигат тези цифри, иначе CI ще е червен до сливането на цялата серия; при 100% всяка бъдеща непокрита клонка ще спира CI.
Дребни забележки (незадължителни)
- Крехкост при SQL string-matching (партиди 3 и 4): фалшивите D1 обекти избират клон чрез
sql.includes(...)с non-null assertion — при рефактор на SQL може да минат по грешен клон или да паднат с неясно съобщение. Помощник с описателна грешка би помогнал. - Дублиран boilerplate за fake D1 (партида 3): би могъл да се извлече общ
makeFakeDbв споделен test-util. - Дребни консистентности: излишен
afterEachвeop.test.ts; тавтологично твърдение вrender-format.test.ts; несъответствие в кастовете (as D1Databasevsas unknown as D1Database) вsearch.test.ts; два отделниdescribe('getContractFacets…')блока. - Няколко render-теста разчитат на
setTimeout(r, 0)вact()— стандартно, но следете за потенциална нестабилност.
Присъда
Четири от петте партиди одобряват безусловно; петата препоръчва COMMENT. Няма блокиращи проблеми, няма уязвимости, тестовете са с високо качество. Препоръка: одобрение след потвърждение, че промяната в знаменателя на покритието (**/src/test/** в vitest.shared.ts) не скрива продуктов код и че baseline праговете реално се достигат от цялата серия.
| }); | ||
|
|
||
| describe('formatCell — date null branch', () => { | ||
| it('renders a null date value as the shared em-dash', () => { |
There was a problem hiding this comment.
Незадължително: тази проверка сравнява formatCell(null, 'date') директно с date(null), което е леко тавтологично — тя минава за всяка стойност, стига formatCell да делегира на date(), без да фиксира конкретния очакван изход (em dash). За да е по-устойчива на регресии/мутации, разгледайте твърдо очаквания низ, напр. expect(formatCell(null, 'date')).toBe('—').
There was a problem hiding this comment.
Прав си — беше тавтологично. Заковано на буквалния символ, плюс не-null пътя, който липсваше изобщо:
expect(formatCell(null, "date")).toBe("—");
expect(formatCell("2026-03-05", "date")).toBe("05.03.2026");
expect(formatCell(20260305, "date")).toBe("20260305"); // непарсируемо → ехо, никога фалшива датаДвойката убива две мутации, които старото твърдение не виждаше — включително case "date": return "—" (винаги em dash), която минаваше и през toBe(date(null)). 4b47ca0
| // This glob is wide on purpose but narrow in contract: src/test/ is for test HELPERS only. Product | ||
| // code placed there would leave the coverage denominator silently, which is the one way this list | ||
| // can hide an untested module rather than an uncoverable file. Review any new src/test/ entry on | ||
| // that basis (review ydimitrof, #254). |
There was a problem hiding this comment.
Изключването на **/src/test/** е широко и е единственият запис в списъка, който може да скрие непокрит продуктов модул (а не непокриваем файл), защото премахва целия каталог от знаменателя на покритието. В PR с цел ≥95% покритие това е чувствителна промяна.
Моля потвърдете, че към момента под нито един src/test/ не живее продуктов код (само тестови помощници — D1 shim-ове, стъбове за cloudflare:workers). Предложение за по-строг договор: стеснете глоба до конкретните помощни файлове/суфикси (напр. **/src/test/**/*.helper.ts или **/src/test/support/**), за да не може бъдещ продуктов файл, поставен там, да напусне мълчаливо покритието.
There was a problem hiding this comment.
Потвърждавам, и вместо да го оставя на дума — премахнах самата възможност.
Пълното съдържание на всеки src/test/ в репозиторието днес, три файла:
apps/etl/src/test/cloudflare-workers-stub.ts
apps/etl/src/test/cloudflare-workflows-stub.ts
packages/ingest/src/test/d1-sqlite.ts
Единственият не-тестов файл, който изобщо ги внася, е apps/etl/vitest.config.ts — alias-ва cloudflare:workers / cloudflare:workflows към стъбовете. Нула продуктови модули.
Взех предложението ти, но в по-строгия му вариант: вместо **/src/test/**/*.helper.ts, изброих трите файла поименно. Суфиксна конвенция все още пуска продуктов файл, кръстен *.helper.ts; изричният списък не пуска нищо — нов файл под src/test/ се брои в покритието, докато някой съзнателно не го добави тук, което е ревюируем акт, а не страничен ефект от местоположението.
Покритието е непроменено и в шестте workspace-а след смяната, което потвърждава, че списъкът покрива точно това, което glob-ът покриваше. 4b47ca0
There was a problem hiding this comment.
За протокола — вдигнах рефактора на фалшивия D1 като #325, с числата зад бележката ти: 29 файла, ~130 локални билдъра, и само 2 от тях хвърлят при непозната заявка срещу 42 места, които връщат { results: [] } по подразбиране. Мълчаливото падане в грешен клон при рефактор на SQL е същественият риск, не дублираният boilerplate.
И една поправка от моя страна: за кастовете беше прав, а аз ти отговорих погрешно. Проверих вътрешната консистентност на search.test.ts (там и двата са as D1Database), а ти си имал предвид отклонението от конвенцията в останалите файлове — 55 срещу 33 в полза на as unknown as D1Database, и search.test.ts е един от 7-те файла с голия каст. Влиза в #325.
…l assertions (midt-bg#254) Second round of ydimitrof's review. vitest.shared.ts — replaced the `**/src/test/**` directory glob with the three files it actually covers: the cloudflare:workers/workflows stubs apps/etl aliases the real modules to, and the ingest SQLite D1 shim. A directory glob was the one entry on this exclude list that could hide an untested product MODULE rather than an uncoverable file, since anything later dropped into src/test/ would leave the coverage denominator by virtue of its location. With an explicit list, a new file there is measured until someone deliberately adds it — a reviewable act. Stricter than a suffix convention, which would still let a *.helper.ts product file through. Coverage is unchanged in all six workspaces, confirming the list covers exactly what the glob did. render-format.test.ts — the date-null case asserted `formatCell(null, 'date')` equals `date(null)`, which only proves delegation and passes for any value the shared formatter returns. Pinned to the literal em-dash, and added the non-null path (a formatted date and an unparseable value that must be echoed, never rendered as a fake date). The pair now kills two mutations the old assertion could not see — including a `date` branch hard-coded to return the em-dash. eop.test.ts — dropped the describe-local afterEach that duplicated the file-level vi.unstubAllGlobals introduced when this file was union-merged with upstream. contracts.test.ts — the two `describe('getContractFacets')` blocks now carry distinct names for what each covers. Coverage unchanged: etl 100/97.64, web 99.47/96.21, config 100/100, db 100/98.47, ingest 100/98.69, shared 98.87/98.33.
|
@ydimitrof Благодаря за обстойния преглед. Двете условия за одобрение — потвърдени, и двете с промяна, не само с отговор. 1. Знаменателят на покритието ( Взех предложението ти в по-строгия му вариант: изброих трите файла поименно вместо суфиксна конвенция. Отделно измерих и колко изобщо струва това изключване, за да не стои на думи: мести само 2. Достигат ли се baseline праговете. Да, и това не е серия — това е един PR, ревюиран от теб на 5 партиди. Няма следващи части, които да чакаме. CI на
Дребните забележки: тавтологичното твърдение в
Ако това затваря двете условия, ще подадеш ли ревюто като GitHub Approve? И четирите досега са |
…g#254 review) The comment said a whitespace-only value 'trims to ""' and grouped it with the nullish inputs, which reads as if it hits the `if (!procedureType)` guard. It does not: ' ' is truthy, passes the guard, and reaches the map lookup, where .get('') misses and the ?? fallback supplies PROCEDURE_UNKNOWN. Spelling out both paths so the map-miss branch is visibly exercised here as well as by the unrecognised-type case above.
…table midt-bg#312) Fifth sync. Upstream midt-bg#312 turned /conflicts from a card list into a one-row-per-person table and renamed ConflictCards → ConflictDetail, which the person/company pages now render eagerly. One conflict, in conflicts.render.test.tsx. Resolution: took upstream's file whole rather than union-merging. Our tests there targeted the card LIST — expand-to-drill-down, per-card contract lists — and that route no longer renders cards at all, so keeping them would have asserted against a UI that does not exist. Upstream's 16 table tests replace them on their own terms. That resolution cost real coverage, because the component SURVIVED the route change while its tests did not: apps/web branches fell to 94.71% (floor 96), with 18 uncovered branches concentrated in ConflictDetail.tsx. Restored with tests against the component directly rather than by moving the floor: - apps/web/app/components/ConflictDetail.test.tsx (new, 10 tests) — the thinner row shapes the feed really produces: a 'confirmed' seal that cites no register act (no bare „№"), a link with no source declaration, a declaration with no usable period, a sub-threshold authority share („под 0,1%", track but no fill), a share with no denominator („—", no track at all — „0%" would be a different and false claim), a winner whose amounts are all NULL, a timeline with marks but no band, unnumbered contracts (index-keyed, two rows not one), and an unresolved awarding body („—"). ConflictDetail branches 76/94 → 91/94. - conflicts.test.ts — declaredStakeNoun's mixed-page branch. A page carrying both a self stake and a family stake must fall back to the neutral wording; either single-sided noun would be false about half its cards, and asserting „собствен дял" over a family card is a false claim about a named individual. Deliberately left uncovered: the officialSlug tie-break in groupByPerson's comparator. Its own comment documents it as unreachable belt-and-braces (isStrongerLink already breaks every tie on the globally unique link_key), kept for totality. Also repaired one test the merge broke: related-persons.test.ts registered contract rows under a bare link_key, but upstream's improved fake namespaces contract reads under `contracts:<key>` so a scope query can never be mistaken for a contract read. Verified still discriminating — leaking `r.authority` instead of `?? ''` fails it. Coverage after: etl 100/97.64, web 99.42/95.96, config 100/100, db 100/98.12, ingest 100/98.69, shared 98.87/98.33. Gate green, no floor moved.
|
@todorkolev @ydimitrof PR-ът е готов и стои само на натискането на бутона. Състояние на Пети sync — #312 е поетТаблицата по лице смени Това обаче костваше реално покритие, защото компонентът оцеля, а тестовете му — не:
Умишлено непокрит остава tie-break-ът по Покрай това поправих един тест, който сливането счупи: Покритие след сливането: etl 100/97.64, web 99.42/95.96, config 100/100, db 100/98.12, ingest 100/98.69, shared 98.87/98.33. Нито един праг не е местен. @ydimitrof — отворените нишкиМинах през всичките още веднъж срещу текущия HEAD:
Молба@todorkolev, нямам write достъп (triage), тъй че не мога да го слея сам. PR-ът е на пети sync и всеки път поема нова upstream работа — колкото по-дълго стои, толкова повече конфликти събира. Ще го погледнеш ли? |
midt-bg#325: 24 test files hold 36 `as D1Database` casts, one hand-rolled double each. Every one dispatches on `sql.includes('…')` and falls through to `{ results: [] }` when no marker matches, so renaming a CTE or reordering a JOIN leaves the test green against emptiness — asserting nothing. Only details.test.ts throws today. This is the acceptance test for that work, written before the work: outside an explicit allowlist, no file under apps/ or packages/ may type a value as a D1Database. It is red now (24 files, 36 casts) and goes green when the last double moves to the shared helper. The allowlist is by name, never a directory glob — the argument the midt-bg#254 review already made about the coverage exclusion list. A glob lets a new double leave the gate by where it sits; a named entry means someone had to add it, which is reviewable. A stale entry is an error rather than a no-op, so a renamed double cannot leave the gate widened by a line nobody reads again. That fail-closed branch is what fires right now, since the helper does not exist yet. Matching is over blanked source — comments, strings and regex literals removed, byte positions kept — so a comment describing the old design is not a finding. `as unknown as D1Database` is matched before `as D1Database` because the short spelling is a suffix of the long one and a naive pattern counts one cast twice. `satisfies` is covered too: it is the only other operator that types a literal. Self-test is mutation-checked — dropping the `as unknown as` alternative, the `satisfies` alternative, the comment blanking, the trailing word boundary, or the stale-entry check each kills exactly one named test, and no others. scripts-test.yml needs no edit: its lane already globs scripts/*.test.mjs.
The helper midt-bg#325 asks for, as its own private workspace. `@sigma/db` exports only `.`, and neither apps/etl nor packages/ingest depends on it, so putting the double under db/src/test/ would have meant a subpath export plus two new workspace deps. A separate package also sits outside all six measured workspaces, so it cannot enter their coverage denominators by construction — stronger than the by-name vitest.shared.ts exclusion the issue proposes, and it leaves that file (which midt-bg#254 rewrites) untouched. A route is a marker set and a response; every marker must appear in the SQL, and the first matching route wins so a specific route can precede a general one. Unmatched throws, naming the offending statement and every registered marker. `{ onUnmatched: 'empty' }` buys emptiness back, at the call site, in writing. Three entry points, one core: fakeD1 for query tests, recordingD1 for the tests of a *wrapper* over D1 (readonlyD1) that must accept arbitrary SQL and assert on a call log, throwingD1 for the error paths. Two design notes worth keeping: - `first` is typed `object | null | (call) => object | null`, not `unknown`. A top type absorbs the union and the callback form silently loses its parameter type — tsc caught it. A D1 row is an object or nothing anyway. - No pagination feature. Keyset slicing is already `all: (call) => rows.filter(r => r.id > call.binds.at(-2))`, which is what the doubles in companies.test.ts do by hand today. Tests written before the code, behaviour by behaviour, and mutation-checked: never throwing on an unmatched all() or first(), matching a route that answers a different method, `some` for `every` over the markers, last-match instead of first, no truncation, dropping the marker list from the message, discarding bind() arguments, not recording prepare() or batch(), and ignoring throwingD1's supplied error — eleven mutations, each killing a specific named test. The `?? []` fallback in all() went away rather than getting a test: the route lookup already guarantees the response is defined, so it was unreachable. Returning the response instead of the route also keeps `first: null` — a route meaning "no such row" — distinct from no route at all. Seventh key in coverage-baseline.json: check-coverage's findTestWorkspaces fails closed on a workspace that has a test script without a baseline entry, and this one should be measured. 100% lines, 100% branches. The six existing workspaces are untouched — they were already reading above their baselines before this branch, which is pre-existing drift and not for a test refactor to ratchet.
d1FromSqlite lived in packages/ingest/src/test/, and packages/db had two byte-identical re-implementations of it (contracts-filter-sql, value-base-sql, differing only in a local variable name) while apps/etl reached the original through ../../../packages/ingest/src/test/d1-sqlite — a relative path across a workspace boundary, which is what a missing shared home looks like. It moves to @sigma/test-support beside the fake. The two are different tools and stay different: this one runs the real SQL against a real node:sqlite database where SQL semantics are what is under test; fakeD1 is for the TypeScript logic around a query. Now they at least live in the same place, and the gate's allowlist names one package instead of two. Slightly wider than midt-bg#325 asked — the issue scopes itself to the fake doubles and puts real-SQLite tests out of scope. It is here because "one cast everywhere" is one of its own done-when boxes, and two of the four remaining casts were these copies. Reviewer's call; it lifts out cleanly. Side effect worth noting: d1-sqlite.ts leaves packages/ingest's coverage denominator by leaving the workspace, which is the outcome midt-bg#254 wanted from a by-name exclusion, reached by construction instead. db 487, ingest 84, etl 20 — all pass.
* test(ci): gate the fake-D1 doubles, and fail on an unmatched query #325: 24 test files hold 36 `as D1Database` casts, one hand-rolled double each. Every one dispatches on `sql.includes('…')` and falls through to `{ results: [] }` when no marker matches, so renaming a CTE or reordering a JOIN leaves the test green against emptiness — asserting nothing. Only details.test.ts throws today. This is the acceptance test for that work, written before the work: outside an explicit allowlist, no file under apps/ or packages/ may type a value as a D1Database. It is red now (24 files, 36 casts) and goes green when the last double moves to the shared helper. The allowlist is by name, never a directory glob — the argument the #254 review already made about the coverage exclusion list. A glob lets a new double leave the gate by where it sits; a named entry means someone had to add it, which is reviewable. A stale entry is an error rather than a no-op, so a renamed double cannot leave the gate widened by a line nobody reads again. That fail-closed branch is what fires right now, since the helper does not exist yet. Matching is over blanked source — comments, strings and regex literals removed, byte positions kept — so a comment describing the old design is not a finding. `as unknown as D1Database` is matched before `as D1Database` because the short spelling is a suffix of the long one and a naive pattern counts one cast twice. `satisfies` is covered too: it is the only other operator that types a literal. Self-test is mutation-checked — dropping the `as unknown as` alternative, the `satisfies` alternative, the comment blanking, the trailing word boundary, or the stale-entry check each kills exactly one named test, and no others. scripts-test.yml needs no edit: its lane already globs scripts/*.test.mjs. * test(test-support): a shared D1 double that throws on an unmatched query The helper #325 asks for, as its own private workspace. `@sigma/db` exports only `.`, and neither apps/etl nor packages/ingest depends on it, so putting the double under db/src/test/ would have meant a subpath export plus two new workspace deps. A separate package also sits outside all six measured workspaces, so it cannot enter their coverage denominators by construction — stronger than the by-name vitest.shared.ts exclusion the issue proposes, and it leaves that file (which #254 rewrites) untouched. A route is a marker set and a response; every marker must appear in the SQL, and the first matching route wins so a specific route can precede a general one. Unmatched throws, naming the offending statement and every registered marker. `{ onUnmatched: 'empty' }` buys emptiness back, at the call site, in writing. Three entry points, one core: fakeD1 for query tests, recordingD1 for the tests of a *wrapper* over D1 (readonlyD1) that must accept arbitrary SQL and assert on a call log, throwingD1 for the error paths. Two design notes worth keeping: - `first` is typed `object | null | (call) => object | null`, not `unknown`. A top type absorbs the union and the callback form silently loses its parameter type — tsc caught it. A D1 row is an object or nothing anyway. - No pagination feature. Keyset slicing is already `all: (call) => rows.filter(r => r.id > call.binds.at(-2))`, which is what the doubles in companies.test.ts do by hand today. Tests written before the code, behaviour by behaviour, and mutation-checked: never throwing on an unmatched all() or first(), matching a route that answers a different method, `some` for `every` over the markers, last-match instead of first, no truncation, dropping the marker list from the message, discarding bind() arguments, not recording prepare() or batch(), and ignoring throwingD1's supplied error — eleven mutations, each killing a specific named test. The `?? []` fallback in all() went away rather than getting a test: the route lookup already guarantees the response is defined, so it was unreachable. Returning the response instead of the route also keeps `first: null` — a route meaning "no such row" — distinct from no route at all. Seventh key in coverage-baseline.json: check-coverage's findTestWorkspaces fails closed on a workspace that has a test script without a baseline entry, and this one should be measured. 100% lines, 100% branches. The six existing workspaces are untouched — they were already reading above their baselines before this branch, which is pre-existing drift and not for a test refactor to ratchet. * test(test-support): keep `sql` live, and fail the throwing double at execution Two defects the first migration batch walked straight into. `sql` was a getter over `calls`, so `const { db, sql } = fake()` — the natural way to use it, and what flows.test.ts and authorities.test.ts already wrote against their hand-rolled spies — captured an empty snapshot that never filled in. Every later assertion then read nothing and passed for the wrong reason, which is the exact failure this helper exists to remove. It is a live array kept in step with `calls` now, and a test pins the destructured form. throwingD1 threw from prepare(). D1's prepare() is lazy and never touches the database: a missing table surfaces on all()/first()/run(). A double that failed earlier would let a test claim it covers an error path it never reaches — and related-persons.test.ts, whose whole point is that an un-migrated environment degrades instead of 500ing, hand-rolled a double that threw at execution for exactly that reason. It now rejects from the three execution methods and records the statement that failed, so the offending SQL stays inspectable. * test(db): route the query doubles through the shared fake Sixteen files in packages/db/src/queries, each of which built its own D1 double that dispatched on `sql.includes('…')` and fell through to no rows. Fixtures and assertions are unchanged — only the double moves. Measured before touching anything, by breaking each marker in the production SQL and running the test: ELEVEN marker paths across nine files stayed green against an emptied result. authorities (FROM authority_totals), companies (ORDER BY bidder_id), competition and trend and flows (FROM sector_totals), contracts (facet_counts), home (bids_received = 1, JOIN), network (FROM company_totals, FROM authority_totals WHERE authority_id), search (sqlite_master). Every one of them now rejects with the marker set it was looking for. Three things the migration turned up that were not in the issue: - regions.test.ts served *region* rows to sectorOptions, which asks a completely different table. It reached the same answer only because sectorOptions reads r.division, the region fixture has no such field, and the filter dropped every row. The route says `all: []` now, and says why. - companies.test.ts registered two facet routes for queries no test in it ever issues — getCompanyFacets is not exercised there. Dropped rather than kept as decoration. - companies' CSV stream and list query both read company_totals, so breaking the stream's ORDER BY quietly fell through to the list route and returned an unpaginated page. They are separated by their own markers now (ORDER BY bidder_id vs AS sort_value), and breaking either one throws. Two markers still survive being broken — authorities' and companies' `FROM <rollup>`. That is the harness, not the tests: `FROM ${src.from}` is composed at runtime, so the literal never appears in the source to be mutated. Mutating the `from:` value itself is caught by both. Route matching is still substring-based, so a query can fall from a specific route to a more general one in the same set. What is gone is the *default* fall-through to emptiness — an unrouted query throws. packages/db: 487 tests pass; coverage unmoved. * test(test-support): one real-SQLite D1 facade instead of four copies d1FromSqlite lived in packages/ingest/src/test/, and packages/db had two byte-identical re-implementations of it (contracts-filter-sql, value-base-sql, differing only in a local variable name) while apps/etl reached the original through ../../../packages/ingest/src/test/d1-sqlite — a relative path across a workspace boundary, which is what a missing shared home looks like. It moves to @sigma/test-support beside the fake. The two are different tools and stay different: this one runs the real SQL against a real node:sqlite database where SQL semantics are what is under test; fakeD1 is for the TypeScript logic around a query. Now they at least live in the same place, and the gate's allowlist names one package instead of two. Slightly wider than #325 asked — the issue scopes itself to the fake doubles and puts real-SQLite tests out of scope. It is here because "one cast everywhere" is one of its own done-when boxes, and two of the four remaining casts were these copies. Reviewer's call; it lifts out cleanly. Side effect worth noting: d1-sqlite.ts leaves packages/ingest's coverage denominator by leaving the workspace, which is the outcome #254 wanted from a by-name exclusion, reached by construction instead. db 487, ingest 84, etl 20 — all pass. * test(db): recording doubles for the two readonly wrapper suites readonly-d1 and readonly-corpus test a *wrapper* over D1, not a query: what matters is which statements reach the handle underneath, not what comes back. Marker dispatch is the wrong shape for that, so both use recordingD1 — answers anything, records everything — with `when: []`, a route that constrains nothing. Two things came out of it, both in the helper: - `when: []` matching every query was already true (every() over no markers), but undocumented and unpinned. Now both. - readonly-d1's hand-rolled log tagged its entries `prepare:` / `exec:`, and flattening that into plain SQL would have cost the test its point: a wrapper that sent an exec down the prepare path emits identical text, and the assertion could no longer tell. FakeD1Call carries `via` now, and the corpus's zero-proxy row survives as the response to a constraint-free route. readonly-corpus also dropped a `raw()` no production path calls. packages/db: 487 tests pass. * test(etl): route the ETL doubles through the shared fake Three doubles. The integrity gate's fake dispatched on eleven markers and fell through to no rows; it now names all eleven as routes and rejects anything else. Its local builder was called `fakeD1`, which is the shared helper's name, so it becomes `servedD1` — which is what it models anyway: a served D1 after precompute, not any old one. eop.test.ts also passed `{} as D1Database` twice, for paths that fail before they reach the database. `fakeD1([])` states that instead of implying it: a route-less double rejects any query, so if one of those paths ever did reach D1 the test would say so rather than throwing an incidental TypeError on an empty object. The freshness double's guard survives as a route that throws its own message — "raw staging should not be read for planning" is a claim worth keeping in the test, rather than degrading to the generic no-route error. apps/etl: 20 tests pass. * test(test-support): per-route meta, and cover the SQLite facade in its new home apps/web's assistant tests need meta.rows_read and meta.total_attempts: they drive the rows-read budget that keeps a retried full scan from under-billing the Denial-of-Wallet limit (#122, review #80). Flattening that to a fixed empty meta would have quietly removed what those two tests assert, so a route can declare its own meta. Default stays `{}`. Moving d1-sqlite.ts here left it with no tests of its own — its callers live in db, ingest and etl, and none of them count toward this workspace. The ratchet caught it at 81% and it is covered directly now, including the case nothing tested anywhere before: batch() rolls back when one statement fails. A half-applied batch would leave a fixture in a state no production path can reach, and whoever met it would be debugging a ghost. While covering it, throwingD1's bind() read `calls.at(-1)` — so binding statement A after preparing B recorded the arguments against B. Same statement-independence bug fakeD1 already had a test against; it captures its own record now, and so does the test. 100% lines, 100% branches, 44 tests. * test(web): route the last two doubles through the shared fake assistant/tools built a double whose only real job was carrying meta; it now declares that meta on a route. csv-export asserted the expected SQL *inside* its fake — that assertion becomes the route's own marker, so a query that no longer matches rejects and names both the statement and what was expected, instead of failing an inline expect from inside a stub. With these two the gate from the first commit goes green: 279 files scanned, no D1Database cast outside @sigma/test-support. It opened at 36 casts in 24 files. apps/web: 493 tests pass. * fix(test-support): route exec() and batch() through the same contract batch() recorded each statement and returned a synthetic success without ever consulting the routes, so a batch of unregistered SQL passed against nothing — the silent green this helper exists to kill, on the one entry point the write paths use exclusively (staging, refresh, fx never call prepare().run()). exec() had the identical hole one method up. Both now look the statement up. They ask only whether it is registered at all, not for a particular response shape the way all()/first()/run() do, and throw naming the SQL and every marker when it is not. Also: run() and batch() carry the `results` key a real D1Result always has — the cast to D1Database was hiding its absence; the header no longer points at the facade's pre-move path; and the second batch() record is documented as a log of entry points rather than a double count. * fix(test-support): carry the full D1Result shape in the SQLite facade all() returned no `meta`, run() neither `meta` nor `results`, batch() no `results`. The cast to D1Database hid every one of them: the first caller to read one would get `undefined` from the facade where real D1 hands back `[]` or `{}`. One D1Shape type spells out all three keys. * test(web): keep the exact-SQL check csv-export had before the migration The hand-rolled double asserted `expect(sql).toBe(...)` on the whole statement. Migrating turned that string into a `when` marker, and markers match by substring — so the one place the refactor loosened a check rather than tightening it. Measured: wrapping the production statement leaves all 34 tests green. The equality moves inside the route, where the callback sees `call.sql`. * test(ci): pin the gate's scan roots and catch an aliased D1Database Two ways past the gate, both reproduced. `type DBAlias = D1Database` and then `as unknown as DBAlias` leaves no D1Database token for the pattern to find; a renamed type import does the same. A second pass treats giving the type another name outside the allowlist as the offence, while leaving ordinary annotations (`db: D1Database`, a field on an Env type) alone. And SCAN_ROOTS was module-private, so deleting 'apps' from it left the self-test 12/12 green while web and etl dropped out of enforcement. Exported and pinned: a pattern applied to half the repo is a gate that passes while enforcing nothing. * fix(test-support): route a batched statement by what it does, not its markers Markers alone are blind to the method, and it is measurable: a route declaring only `all:` answered a batched `DELETE FROM staging` with its rows, because `FROM staging` is a substring of the write. The same SQL through prepare().run() threw. Narrower than an unrouted batch, but the same silent pass. A batched write now needs a `run:` route and a batched SELECT an `all:` one; neither settles for the other, and a SELECT no longer fires a write effect it happens to match. A write still serves rows when it has them, for RETURNING. exec() asks for `run:` too — it hands back no rows, so nothing else means anything to it. That retires `registered()`: every entry point is method-aware. Reading is decided by the leading keyword, so a `WITH … INSERT` reads as a SELECT here. That costs a false rejection, never a false pass. Also: run() carries the route's meta, which batch() already resolved for the same statement, and batch() documents that it is not transactional — real D1 and the d1-sqlite.ts facade roll back, this does not. * test(ci): widen the alias rule to where an alias actually goes The pattern closed three spellings while the comment promised the class. Four more walked past it: `D1Database & {}`, `Pick<D1Database, …>`, a namespaced `import('…').D1Database`, and a heritage list naming it off the first position. The rule is now positional — the mention must sit right of `=`, or inside an intersection, union, type argument or namespace, never where a parameter or a field goes. `type Env = { DB: D1Database }` and the conditional type in readonly-corpus.test.ts stay clean, both pinned. `implements` is deliberately out: ReadonlyD1 implements D1Database in production, and TypeScript forces a complete implementation there, so it is no shortcut to a stub. The comment now says best-effort and means it. --------- Co-authored-by: Todor Kolev <tkolev@obecto.com>
|
Основата под този PR се размести, откакто беше отворен - две неща вече са на #216 е слято. Ти vendor-ваше харнеса му едно към едно ( #331 е слято току-що. Замени ~29 ръчни фалшиви D1 двойника с общия Актуалните нишки от прегледа на @ydimitrof, групирани: Вероятно се решават от само себе си при мигриране към
Остават за твоя преценка след ребейза:
Само тестове, критичният път за свързаните лица не минава оттук - тоест няма спешност. Като изравниш върху |
Adopts midt-bg#331's shared fake-D1 double and its check:fake-d1 gate across every suite this branch adds, and midt-bg#334's CACBG corpus work. - migrate all 71 hand-rolled D1 doubles (18 files) to @sigma/test-support: fakeD1 for query tests, recordingD1 for the two ingest wrappers whose SQL is generated rather than routed, throwingD1 for the run_sql error path. An unmatched query now throws instead of answering with no rows, which surfaced two silent gaps: getCompany/getAuthority never declared the listContracts panel reads, and eop/etl passed `{}` as a binding that could never fail. - keep the doubles that must record batch GROUPING (ingest staging/refresh) by wrapping batch() over the shared double rather than re-rolling one. - coverage-baseline.json: union of this branch's raised floors and upstream's new packages/test-support entry; apps/etl branches ratcheted 97 -> 97.6. - vitest.shared.ts: drop the now-stale src/test/d1-sqlite.ts exclusion (midt-bg#331 moved that shim into packages/test-support, which carries its own entry). All seven workspaces stay at or above their floors; check-fake-d1, check-docs and check-coverage are green.
Line-length reflow only — no assertion or fixture changes. The three files whose fakeD1 call sites pushed a line past printWidth after the midt-bg#331 migration.
|
@todorkolev Изравнено върху #331 — мигрирано изцялоВсичките 71 ръчни двойника в 18 файла минаха на Разпределението не е механично — трите помощника решават различни задачи:
Строгостта хвана два реални пропуска, не само стилови:
По нишките, които ти групира като „вероятно се решават от само себе си" — решават се, но с проверка, не по презумпция:
#216 — две уточненияVendor-ната част я няма. Проверено байт по байт срещу
Тоест по тези седем файла няма какво да се маха — дифът им срещу
Махнах и остарялото За прага Три от петте „остават за преценка" бяха вече свършениНе е спор — просто нишките на @ydimitrof не се резолват, тъй че списъкът се чете от състояние отпреди фиксовете:
Покритие след сливането
Дребните минуси при Няма продуктова промяна: 59 файла, всичките тестове, конфигурация за покритие и базовата линия. Като си готов — бутонът е при теб. |
|
Бърза и добра работа - миграцията на 71-те двойника е точно каквото трябваше, а че строгостта на Четирите нишки, които предвидих, че се решават от общия двойник, наистина се решиха - проверих Две от останалите пет си струват още едно докосване, преди да го натиснем - и двете са секунди работа:
Другите три ( Само тестове, не блокира свързаните лица. Кажи като си готов и минавам. |
|
@todorkolev Благодаря. По четирите „козметични" — три от тях вече са направени на 1. Праговете — измерено, не на окоЧислата от
Тоест: прав си за три от петте, не за петте. При Причината обаче не е числото 100, а мерната единица. Струва си да се каже и че капанът вече е на Затова не бих свалял праговете. Праг 99 върху покритие 100 не е ratchet — това е предварително разрешение то да падне: следващ PR може да остави ред непокрит в напълно тестван workspace и заданието остава зелено. Това е точно преместването на гредата, само отложено. Предложение — оправи се механизмът, не числата. Буферът да е поне един елемент, независимо от размера: // scripts/check-coverage.mjs, compareWorkspace()
const perItem = actual[metric].total > 0 ? 100 / actual[metric].total : 0;
const slack = Math.max(tolerance, perItem);
if (pct < base - slack - 1e-9) {Прототипирах го и го измерих локално:
Това е твоят харнес и твоят слой. Готово е — кажи къде да влезе: отделен PR срещу 2.
|
Per @todorkolev on midt-bg#254: the five workspaces sitting at lines 100 drop to a 99 floor. Measured coverage is unchanged at 100% in all five — this only widens the margin before the ratchet fires. Why it mattered: `tolerance` is a percentage, so it is near-zero slack in a small workspace. At 100/100 a single uncovered line red-builds packages/config (28 lines), packages/test-support (99) and apps/etl (169) — including on PRs that never touch tests. packages/db needed 6 and packages/ingest 3. Branch floors are untouched.
|
@todorkolev Направено — CI е 4/4 зелено на Другите четири ги оставям както са — трите вече бяха направени на Една бележка за протокола, не за този PR: Твой е бутонът. |
4 participants
Надгражда coverage харнеса от #216 с реалните тестове и вдига ratchet прага на ≥96% клонове и ≥98.8% редове за всеки workspace (повечето на 100%). Само тестове — нулева промяна в продуктов код.
Този PR трябва да се слее след #216 (
ydimitrof:feat/coverage-ratchet), който все още е отворен.#216 въвежда самия харнес —
scripts/check-coverage.mjs+check-coverage.test.mjs, споделенияvitest.shared.ts, per-workspacevitest.config.ts,coverage-baseline.json(с началните, ниски прагове) и CI стъпката. Тук този харнес е vendor-нат едно към едно (двата скрипта са байтово идентични с #216), за да могат тестовете да се пишат и пускат срещу реалната порта преди #216 да е слят.Слят преди #216, този клон би въвел харнеса, CI стъпката и конфигурациите за втори път → конфликт.
След като #216 се слее, клонът се rebase-ва върху
main; vendor-натият commit (69d0a34) се свива (идентично съдържание), а нетното участие остава:vitest.shared.ts: изключване на**/fixtures/**,**/*.json,**/*.md,**/*.d.tsот инструментирането (данни/типове/докове, не изпълним код — иначе висят на постоянни 0% и v8 remap-ът чупи на markdown);coverage-baseline.json: праговете вдигнати от началните на test: измерване на coverage и ratchet праг в CI (#93) #216 до текущите (виж таблицата).Резултат
Общо (информативно): 99.75% редове / 97.55% клонове.
node scripts/check-coverage.mjsминава; собственият му self-test (check-coverage.test.mjs) също. CI е зелено (typecheck, lint, тестове).Бележки
**/src/test/**е изключен като тестово скеле, и това мести самоpackages/ingest— 100%/98.69% с изключването срещу 98.97%/98.33% без него. Другите пет workspace-а са непроменени до последната цифра, и дори без изключването всичките шест остават над 95%. Изключеното еpackages/ingest/src/test/d1-sqlite.ts— SQLite шим, който съществува само за да върти сюитите.??fallback-и върху regex групи, които винаги съвпадат;if(done)re-entry на stream (stream не дърпа следclose());split().pop() ??(pop винаги връща стойност); homoglyph картата (всеки съвпаднал символ е мапнат);minContracts ?? DEFAULT, което оркестраторът вече нормализира; env-gatedPRODпътища под vitest. Оставени са непокрити, а не заобиколени чрез отслабване на кода илиv8 ignoreпрагми.Свързани: #216 (харнес), #93 (епик за покритие).