test: споделен фалшив D1, който хвърля при непозната заявка - #4
Open
ydimitrof wants to merge 17 commits into
Open
test: споделен фалшив D1, който хвърля при непозната заявка#4ydimitrof wants to merge 17 commits into
ydimitrof wants to merge 17 commits into
Conversation
Test coverage
✅ No workspace dropped below its baseline (tolerance 0.5pp). 📈 Coverage rose by more than 1pp — run |
Collaborator
Review — ✅ APPROVE · Quality 9.6/1045 files, +1502/−797, all 5 CI checks green. Phase 0 — Security-Critical Scan: CLEAN
What the PR doesCollapses ~29 hand-rolled D1 test doubles into one shared helper. The core idea is sound and well-motivated: an unmatched query throws instead of silently returning Strengths
Non-blocking observations
ComplianceNo TODO/FIXME/HACK, no Merge-ready. The two observations are optional polish, not change requests. |
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.
…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.
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.
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.
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.
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.
…s 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 (midt-bg#122, review midt-bg#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.
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.
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.
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.
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`.
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.
ydimitrof
force-pushed
the
test/shared-fake-d1-helper
branch
from
August 26, 2026 06:25
9b1bad1 to
3009dc3
Compare
… 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.
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.
…bg#334) Печат за пълнота до суровия корпус, за да не се публикува отрязан корпус като цял. midt-bg#313 направи частичния корпус ОЧАКВАНО състояние (краулът спира сам на срока и запазва каквото има), а restore-keys: cacbg-raw- връща най-СКОРОШНИЯ запис, не най-пълния. extract.mjs изброяваше файловете, без да ги сверява с описа - тоест отрязан корпус даваше по-малка повърхност, без грешка никъде, а нито гейтът за монотонност (вижда нетен ръст), нито подът --min-links (само брои) го хващат. Как работи: - fetch.mjs пише .corpus-complete.json САМО когато корпусът се сверява с list.xml, и го чисти в началото на всяко обхождане, тоест всеки ненормален изход оставя корпуса неподпечатан. - extract.mjs отказва без печат (--allow-partial-corpus е изричният override). - workflow-ът се лекува сам: непечатан кеш пуска краула, който за ~2 минути го сверява срещу живия регистър и подпечатва - никаква зависимост от поредността на пусканията. Преминало пет кръга независимо adversarial ревю (Codex), които намериха и затвориха ~26 находки: подмножество, което печата цялото дърво; празен индекс; страница за поддръжка; неатомарно разархивиране; осакатен и многокоренов XML; свиване на списъка; и цяла серия пробиви в PII предпазителя на override пътищата (наследен GIT_DIR, вложени хранилища, pathspec магия, symlink пренасочване, локализиран git, подразбиращи се пътища). Тестове: 25 в новия corpus-sentinel.test.mjs, 332 в целия cacbg+tr пакет; 23 мутанта убити. Follow-up (не блокери): пряко броене на top-level XML елементите, структурна валидация на кеширания списък, bind-mount регресия за CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Отнася се за midt-bg/sigma#325. PR-ът е тук, на форка, за преглед преди да тръгне нагоре — заради припокриването с midt-bg#254 (вж. долу).
Какво прави
Един споделен двойник в нов workspace
@sigma/test-support. Маршрут = набор от маркери и отговор; всеки маркер трябва да се среща в SQL-а, а непозната заявка хвърля и назовава както изпълнения оператор, така и всички регистрирани маркери. Празният резултат става изричен избор —{ onUnmatched: 'empty' }или маршрут с празен отговор.Три входни точки:
fakeD1за тестове на заявки,recordingD1за тестовете на обвивка над D1 (readonlyD1), които трябва да приемат произволен SQL и да твърдят срещу дневника на извикванията, иthrowingD1за пътищата с грешка.Мярката, а не твърдението
Преди да е пипнат кой да е тест: счупих всеки маркер в производствения SQL и пуснах съответния тест. 11 маркерни пътя в 9 файла останаха зелени срещу изпразнен резултат — тоест минаваха, без да проверяват нищо:
authoritiesFROM authority_totalscompaniesORDER BY bidder_idcompetitionFROM sector_totalscontractsfacet_countsflowssector_totalshomebids_received = 1,JOINnetworkFROM company_totals,FROM authority_totals WHERE authority_idsearchsqlite_mastertrendFROM sector_totalsВсичките отхвърлят вече. След мигрирането същата проверка дава 47 маркера →
no route matched.Гейтът
scripts/check-fake-d1.mjsе написан пръв и беше червен от първия комит: 36 каста в 24 файла. Сега: 280 сканирани файла, нула каста извън@sigma/test-support.Числата в issue-то са остарели
Issue-то е от преди midt-bg#309/midt-bg#313/midt-bg#314/midt-bg#323 и сочи файлове, които вече не съществуват (
search.suggest.test.tsx,index.control-flow.test.ts) или вече нямат гол каст (agent.test.ts).mainD1Databaseas unknown as/ голas{ results: [] }при непозната заявкаПосоката е вярна, редът на величините — не.
Какво излезе наяве, извън описаното в issue-то
regions.test.tsподаваше редове за области наsectorOptions— съвсем друга таблица. Стигаше до верния отговор само защото фикстурата няма полеdivisionи филтърът изхвърляше всеки ред. Сега маршрутът казваall: []и обяснява защо.companies.test.tsрегистрираше два маршрута за заявки, които никой тест в него не издава (getCompanyFacetsне се упражнява там). А CSV потокът и списъчната заявка четат една и съща таблица, така че счупенORDER BYв потока тихо падаше върху списъчния маршрут и връщаше нестранициран резултат. Разделени са по собствен маркер.readonly-d1.test.tsразличавашеprepare:отexec:в дневника си. Сплескването им щеше да отнеме смисъла на теста — обвивка, която пратиexecпо пътя наprepare, издава същия текст.FakeD1Callносиvia.Три дефекта в самия помощник, намерени от употребата му
sqlбеше getter, тоестconst { db, sql } = fake()хващаше празна снимка, която никога не се пълни. Точно тихата грешка, срещу която е целият PR.throwingD1хвърляше отprepare(). В D1prepare()е мързелив — липсваща таблица излиза при изпълнение. Тест би „покривал" път за грешка, до който не стига.throwingD1.bind()четешеcalls.at(-1), тоест приписваше аргументите на последно подготвения оператор, а не на своя.По-широко от искането — вашето решение
Комит
823c61fсъбира на едно място фасадата над истинско SQLite. Тя съществуваше в четири копия, аapps/etlстигаше до едно от тях през../../../packages/ingest/src/test/— релативен път през граница на workspace.midt-bg#325 се самоограничава до фалшивите двойници и оставя тестовете с истинско SQLite извън обхват. Тук е, защото „един каст навсякъде" е негово собствено условие за готовност, а два от последните четири каста бяха точно тези копия. Комитът се вади чисто, ако предпочиташ.
Покритие
Пет от шестте workspace-а не мърдат.
packages/ingestсе качва с +0.71pp, защотоd1-sqlite.tsнапуска знаменателя му, напускайки workspace-а — резултатът, който midt-bg#254 гони с поименен списък за изключване, постигнат по устройство.Седми ключ в
coverage-baseline.jsonбеше неизбежен:findTestWorkspacesвъвcheck-coverage.mjsсе проваля затворено за workspace съсtestскрипт без запис.@sigma/test-support: 100% редове, 100% клонове, 44 теста. Покриването на фасадата в новия ѝ дом опипа случай, който досега не се тестваше никъде:batch()се връща назад, когато един оператор се провали.Уговорки
FROM <rollup>вauthoritiesиcompanies. Това е ограничение на измерването, не на тестовете:FROM ${src.from}се сглобява по време на изпълнение, така че литералът го няма в кода. Чупенето на самата стойностfrom:се хваща и от двата.Припокриване с midt-bg#254
#254 е отворен и пренаписва 19 от 29-те файла. Който влезе втори, го чака съществен rebase — и по всяка вероятност това сме ние. Затова PR-ът стои първо тук.
Проверка
Всичко зелено. db 487, web 493, ingest 84, etl 20, test-support 44.