test: споделен фалшив D1, който хвърля при непозната заявка - #331
Conversation
nikimilenkov
left a comment
nikimilenkov
left a comment
There was a problem hiding this comment.
Прегледах този PR на HEAD b6667f4 по строгия протокол: четири измерения (сигурност, коректност на помощника, вярност на миграцията по всичките 29 файла, гейт + хигиена на workspace-а), собствена мутационна батерия върху помощника и гейта, и повторение на измерванията от описанието — не на доверие, а на машина.
Първо честното: това е образцово построен PR. Гейтът е написан пръв и е бил червен от първия commit (проверих еквивалентно: върху main преброявам 37 каста в 25 файла — вашите 36/24 плюс едно споменаване в коментар, което blanking-ът на гейта правилно не брои); измерването на празните тестове е възпроизводимо; трите дефекта, които сам сте намерили в помощника, са реално поправени и всеки има именуван регресионен тест; уговорките в описанието са честни и — както се убедих лично — точни. Двата ми първи опита да „опровергая" измерването се оказаха точно двете разкрити ограничения (сглобяваният FROM ${src.from} и заявка, маршрутизирана по друг дискриминиращ маркер), което е добър знак за описанието, не лош.
Възпроизведох централното твърдение двустранно върху home/listSingleOfferContracts с мутация, която не променя семантиката на SQL-а (bids_received = 1 → bids_received=1): на main — 6/6 зелени срещу счупен маркер (тестът минава, без да проверява нищо); на този клон — fake D1: no route matched this all() query. Точно обещаното.
Миграцията е вярна: по диффа на всеки от 29-те файла нито едно твърдение не е отслабено, фикстурите не променят какво се доказва, а на няколко места (details, flows, cohort, sectors) новите маршрути са по-строги от старите произволно-отговарящи двойници. Трите разкрити семантични корекции (regions, companies, readonly-d1 via) са каквото твърдят.
Присъдата ми (съветодателна): одобрение, щом една-единствена находка се затвори — batch() трябва да мине през същия договор като всичко останало. Всичко друго по-долу е за втвърдяване по преценка.
Находката, която бих искал затворена преди сливане
batch() изобщо не маршрутизира — винаги връща синтетичен успех (fake-d1.ts:174-185). Възпроизведох директно: fakeD1 с една регистрирана run: пътека, db.batch([...]) с оператор към несъществуваща, нерегистрирана таблица — зелено, без хвърляне, синтетичен успех и за двата оператора. Това е точно тихото „минава срещу нищо", което целият PR съществува да убие, при това на входната точка, която производственият код за писане ползва изключително (staging.ts, refresh.ts, fx.ts — всичките пишат само през batch(), никога през prepare().run()). Днес е спящо — нито един тест не съчетава run: маршрут с batch(), а двата тестови файла по пътищата за писане правилно ползват истинско SQLite — но първият бъдещ тест, който посегне към fakeD1 за batch-базирана функция, ще получи фалшиво зелено от споделения договор, на който вече всички разчитат. Поправката е малка: прекарайте всеки оператор от batch() през същото matches/responder търсене (с хвърляне при пропуск, освен при lenient), плюс един тест, който пина, че непозната заявка в batch хвърля. Алтернативата — изрично да забраните batch() върху fakeD1 („ползвай d1FromSqlite") — също затваря вратата, стига да е грешка, а не мълчание.
Гейтът — две втвърдявания по преценка
- Заобикаля се с един ред индиректност. Пробвах емпирично: файл с
import type { D1Database as DB }+as DBи сtype DBAlias = D1Database+as unknown as DBAliasминава гейта чисто („280 файла сканирани" — файлът е сканиран, пропускът е в шаблона, не в обхода). Самотестът покрива границите на идентификатора и коментарите/низовете, но не и този клас. Едно второ минаване, което маркираtype X = D1Databaseи преименуван import извън allowlist-а, го затваря — заедно с по два фикстурни случая в самотеста. - Самотестът не пина обхожданите директории. Мутирах скрипта двукратно: отслабен шаблон (без голото
as D1Database) — самотестът гърми с 3 провала, точно както трябва; махнато'apps'отSCAN_ROOTS— 12/12 зелени. Тих регрес на обхвата (тестовете на web и etl отпадат от прилагането) би минал незабелязано. Един тест, който твърди какво се обхожда, пина втората ос.
Извън това гейтът е стабилен: blanking-ът на коментари/низове/regex-литерали е коректен, allowlist-ът е по име и гърми при остарял запис (проверих върху main — отказва точно както е замислено), CI стъпката е последователна със съседните („самотест първо, после гейт").
Дребни
run()/batch()на двойника връщат резултат безresults(а фасадата и безmeta), докато истинскиятD1Resultтип винаги ги носи — маскирано от вътрешния каст. Спящо (никой производствен код не чете резултата отrun()/batch()днес), но първият читател ще получиundefinedот двойника и[]от истинското D1. Едноresults: []на трите места го изравнява.- Остарял коментар на
fake-d1.ts:9сочиpackages/ingest/src/test/d1-sqlite.ts— път, който същият PR премести. Иронично предвид собствената философия на PR-а за остарелите записи, които никой не препрочита. trend.test.tsе четвърта корекция от класа на regions — старият двойник е хранелsectorOptionsс редове за периоди и[]е излизало по случайност; новият маршрут го прави изричен избор с коментар. Коректно поправено в кода, само липсва от разкритието в описанието — казвам го за протокола, не като недостатък.- Две пределно теоретични отслабвания на пазачите, за протокола:
eop.test.tsмести пазача наraw_contractsотprepare()към изпълнение (подготвен, но неизпълнен оператор вече не се лови — на практика нищо не чете така);csv-export.test.tsсменя точно съвпадение с подниз върху пълния текст на оператора (надниз би минал — при маркер, който е целият оператор, е почти невъзможно).
Какво изпълних и проверих лично
- Петте пакета на този HEAD: db 486/487 (единственият провал е познатият env артефакт —
ship-domain.test.tsизвикваpnpmбез пълен път; файлът не е пипан от PR-а), web 493/493, ingest 84/84, etl 20/20, test-support 44/44.check:fake-d1:test+check:fake-d1зелени;check:docsзелен;tsc --noEmitчист в новия пакет и в двата главни консуматора. - Двустранната мутация на
home(по-горе), пробата заbatch(), двете проби за заобикаляне на гейта, двете мутации на самия гейт скрипт — всичко върнато, дървото чисто след всяка стъпка. - Диффът на
coverage-baseline.jsonе точно както е описан: само седмият ключ (100/100 — имайте предвид, че с глобалния толеранс това е най-стегнатият ratchet в репото; всяко бъдещо недотествано добавяне в test-support гърми веднага, което приемам за замислено); +0.71pp на ingest е предимство над базата, не промяна на базата. - Липсата на декларирани зависимости в
packages/test-supportе съществуващата конвенция на репото (shared/db/etl правят същото;node:sqlite, не външен пакет) — не е нов риск.
Координация
По въпроса от описанието за #254: това е решение на поддържащите; отбелязвам само, че тази промяна затваря условието за готовност на #325 по устройство, а #254 пренаписва 19 от същите файлове — редът на сливане определя кой поема rebase-а.
Едно затваряне на batch() и от моя страна това е одобрение — измерването-преди-твърдението, червеният-пръв гейт и честно разкритите ограничения са точно как трябва да изглежда тестова инфраструктура в слой, който публикува твърдения за реални хора.
|
Рефакторът е верен и всъщност вдига чувствителността на пакета — throw-on-unmatched по подразбиране, специфични маркери вместо catch-all fall-through (
// преди: равенство върху изпълнения SQL
expect(sql).toBe('SELECT refreshed_at FROM home_totals WHERE id = 1');
// сега: същият низ е само `when` маркер → match по sql.includes(), не по ===
when: 'SELECT refreshed_at FROM home_totals WHERE id = 1',
Фикс без да въвеждаш нов handle — callback формата на маршрута получава {
when: 'SELECT refreshed_at FROM home_totals WHERE id = 1',
first: (call) => {
expect(call.sql).toBe('SELECT refreshed_at FROM home_totals WHERE id = 1');
return refreshedAt === undefined ? null : { refreshed_at: refreshedAt };
},
} |
|
Благодаря и на двамата — прегледите бяха по-полезни от одобрение. Четири комита (
|
nikimilenkov
left a comment
nikimilenkov
left a comment
There was a problem hiding this comment.
Прегледах четирите нови commit-а до 9b1bad1 със същата дисциплина: повторих всяка своя проба и мутация срещу новия HEAD, а после пуснах и втори пълен строг кръг само върху делтата (253 нови реда), за да проверя дали поправките не внасят нещо ново. Всичко от прегледа е адресирано; от моя страна това е одобрение (съветодателно; решението е на поддържащите). Вторият кръг намери остатъчни втвърдявания по преценка — нищо блокиращо; изредени са след проверките.
Повторени проби — всички дават верния резултат
| Проба | При прегледа | На 9b1bad1 |
|---|---|---|
непознат оператор в batch() |
синтетичен успех, без хвърляне | хвърля no route matched — включително когато по-ранен оператор в същия batch е съвпаднал |
| маршрутизиран batch | n/a | run: callback-ът се вика по веднъж на оператор със своите binds (проверих с два bind-а — [[1],[2]]) |
непознат exec() |
синтетичен {count: 0} (същият клас, намерен от вас) |
хвърля |
recordingD1 + batch |
пропуска всичко | продължава да пропуска — lenient режимът е незасегнат |
файл с import type { D1Database as DB } + type DBAlias = D1Database |
минаваше гейта чисто | гейтът гърми и назовава двата реда с действено съобщение; чистото дърво остава ok без фалшиви аларми по обикновените анотации |
махнато 'apps' от SCAN_ROOTS |
самотестът минаваше 12/12 | 1 провал |
| отслабен alias шаблон | n/a | 2 провала |
| отслабен основен шаблон | 3 провала | 3 провала — остава пинат |
обвит производствен SQL в csv-export.ts:189 (… LIMIT 1) |
34/34 зелени (разхлабването, което открихме независимо с @lyubomir-bozhinov) | 7 провала — точността се върна вътре в маршрута през call.sql, точно предложената форма |
Дребните: D1Shape<T> обединява results/success/meta и тестът е toEqual, тъй че изпуснат ключ пада ✓; коментарът на fake-d1.ts:9 вече сочи фасадата до себе си ✓; разкритието за trend.test.ts е в описанието ✓. Двете решения с аргументи приемам и двете: пазачът в eop.test.ts на изпълнение е по-верен на мързеливия prepare() на D1 (същата грешка, която сам поправихте в throwingD1), а отделният registered() с коментара защо batch()/exec() не искат форма е вярното разграничение по замисъл — с една уговорка от втория кръг по-долу.
Втори кръг върху делтата — остатъчни, по преценка
registered()е сляп за метода, и това е измеримо. Възпроизведох:fakeD1([{ when: 'FROM staging', all: [ред] }])и batch-натDELETE FROM staging WHERE id = 1— минава и връща редовете на read маршрута като резултат на write оператора, докато същият SQL презprepare().run()правилно хвърля. Припокриване на маркер между SELECT маршрут и write оператор е реалистично (FROM stagingе подниз наDELETE FROM staging). По-тясно от оригиналната дупка — иска нещастно съвпадение, не нищо — но е точно мястото, където договорът още пропуска. Стягане, което пази замисъла ви: batch да предпочитаrun:, да пада къмall:за batch-нат SELECT, а съвпадение само поfirst:(или само по маркер на чужд метод) да се брои за пропуск.- ALIAS шаблонът затваря три конкретни изписвания, а коментарът обещава класа. Възпроизведох:
type Evade = D1Database & {}+ каст минава гейта чисто (файлът се сканира — 281 файла). Същото важи заPick<D1Database, …>,import('…').D1Database,class X implements D1Databaseиextends Foo, D1Database(не на първа позиция). Позицията на репото за content-mode гейтове е честен-оператор, не противник — легитимна е; но тогава коментарът „give the type a second name … and the gate treats that as the offence" надобещава. Или разширете първата алтернатива до „RHS, който споменава D1Database" +implements/списъченextends, или смекчете коментара до best-effort. run()изпускаmeta, коетоbatch()вече резолвира — възпроизведох:meta: {changes: 5}излиза{}презprepare().run()и{changes: 5}презbatch()за същия оператор. Никой не чете run-meta днес; един ред го изравнява със заглавието на собствения ви commit („същия договор").- За протокола, дребни: провален строг batch оставя частични странични ефекти (истинското D1 е транзакционно; фасадата отива до rollback — за тест, който твърди състояние след провален batch, разликата ще личи); при два маршрута с застъпващи се маркери batch-нат SELECT може да отговори от
run:-only маршрута с празни редове, докато.all()на същия SQL отговаря от втория; alias находките излизат с exit 1 преди cast находките, тъй че файл с двете иска два пуска.
Нищо от горното не е от класа на затвореното: оригиналът беше „зелено срещу нищо при нула маршрути", а остатъкът иска конкретно съвпадение на маркери. Затова е по преценка, не условие.
Изпълнено тук
Петте пакета: db 486/487 (единственият провал е познатият env артефакт — тестът извиква pnpm без пълен път; файлът не е пипан), web 493/493, ingest 84/84, etl 20/20, test-support 53/53. check:fake-d1:test 19/19 + check:fake-d1 зелени; typecheck чист. Дървото чисто след всяка проба. Една бележка за протокола: първата ми проба срещу новия batch() подаде run: като статичен обект и получи route.run is not a function — мой зле типизиран вход, който typecheck лови в реален тест.
Образцов цикъл: блокиращата находка затворена с шест теста, съседната дупка в exec() намерена и затворена от самия автор, разхлабването в csv-export премерено с мутация преди да се приеме, и едно съзнателно не-пипане, защитено с по-добрия аргумент. Благодаря — и на @lyubomir-bozhinov за независимото стигане до същото място.
|
Клонът изостава от Три конфликта: Тривиални два. Същинският е
Твоят маршрут го свежда до едно Затова го оставям на теб: как споделеният Останалото по PR-а е наред - CI беше зелен преди За контекст защо |
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.
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.
|
Клонът е ребейзнат върху @todorkolev — конфликтът е решен така, както го описаБлагодаря, че спря. Прав беше да го оставиш: и двете неща, които изброи, се губеха при небрежно разрешаване, и то тихо.
Записването на връзванията е запазено, но през Немеханичността е проверена: чупенето на @nikimilenkov — трите остатъчни1. Взех по-широката поправка, а не подредбата: всяка входна точка вече е метод-осъзната. Batch-нат запис иска Четенето се решава по водещата дума, тъй че 2. Шаблонът обещаваше класа. Засадих твоите изписвания: и петте минаваха чисто при 281 сканирани файла. Правилото вече е позиционно — споменаването трябва да стои вдясно от
3. 4. За протокола. Непрозрачността на Проверка на
|
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.
4 participants
Какво и защо
Един споделен двойник за D1 в нов workspace
@sigma/test-support, вместо ~29 ръчни двойника из тестовете.Маршрут = набор от маркери и отговор; всеки маркер трябва да се среща в SQL-а, а непозната заявка хвърля и назовава както изпълнения оператор, така и всички регистрирани маркери. Празният резултат става изричен избор —
{ onUnmatched: 'empty' }или маршрут с празен отговор.Три входни точки:
fakeD1за тестове на заявки,recordingD1за тестовете на обвивка над D1 (readonlyD1), които трябва да приемат произволен SQL и да твърдят срещу дневника на извикванията, иthrowingD1за пътищата с грешка.Свързан issue
Closes #325
Вид промяна
test/ci/build/chore— поддръжкаМярката, а не твърдението
Преди да е пипнат кой да е тест: счупих всеки маркер в производствения 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-то е от преди #309/#313/#314/#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в потока тихо падаше върху списъчния маршрут и връщаше нестранициран резултат. Разделени са по собствен маркер.trend.test.tsе от същия клас катоregions— старият двойник хранешеsectorOptionsс редове за периоди, а[]излизаше по случайност. Новият маршрут го прави изричен избор с коментар.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.#325 се самоограничава до фалшивите двойници и оставя тестовете с истинско SQLite извън обхват. Тук е, защото „един каст навсякъде" е негово собствено условие за готовност, а два от последните четири каста бяха точно тези копия. Комитът се вади чисто, ако предпочитате.
Покритие
Пет от шестте workspace-а не мърдат.
packages/ingestсе качва с +0.71pp, защотоd1-sqlite.tsнапуска знаменателя му, напускайки workspace-а — резултатът, който #254 гони с поименен списък за изключване, постигнат по устройство.Седми ключ в
coverage-baseline.jsonбеше неизбежен:findTestWorkspacesвъвcheck-coverage.mjsсе проваля затворено за workspace съсtestскрипт без запис.@sigma/test-support: 100% редове, 100% клонове, 44 теста. Покриването на фасадата в новия ѝ дом опипа случай, който досега не се тестваше никъде:batch()се връща назад, когато един оператор се провали.След прегледа
Четири комита отгоре, всеки с червен тест преди поправката (@nikimilenkov, @lyubomir-bozhinov):
batch()изобщо не маршрутизираше — записваше и връщаше синтетичен успех. Точно тихото зелено, срещу което е PR-ът, и то на входната точка, която производствените пътища за писане ползват изключително.exec()имаше същата дупка един метод по-горе. И двата вече търсят оператора и хвърлят, ако не е регистриран. Питат само дали е познат, не за конкретна форма на отговора — това остава разликата спрямоall()/first()/run().csv-export.test.tsбеше единственото място, където миграцията разхлаби проверка. Измерено: обвиване на производствения оператор оставя 34/34 зелени. Равенството се върна вътре в маршрута, презcall.sql.type DBAlias = D1Databaseи после каст към псевдонима. Второ минаване хваща даването на второ име на типа извън allowlist-а, без да пипа обикновените анотации. ИSCAN_ROOTSбеше частен, тоест махането на'apps'оставяше самотеста 12/12 зелен. Изнесен и пинат — мутирах и двете оси, за да видя, че вече гърмят.results/metaпо трите места, където кастът къмD1Databaseкриеше липсващ ключ.Не пипнах пазача в
eop.test.ts: местенето му отprepare()към изпълнение е по-вярно на D1, къдетоprepare()е мързелив.Уговорки
FROM <rollup>вauthoritiesиcompanies. Това е ограничение на измерването, не на тестовете:FROM ${src.from}се сглобява по време на изпълнение, така че литералът го няма в кода. Чупенето на самата стойностfrom:се хваща и от двата.Припокриване с #254
#254 е отворен и пренаписва 19 от 29-те файла. Който влезе втори, го чака съществен rebase. Ако предпочитате #254 да мине пръв, кажете — ребейзвам този отгоре.
Как е тествано
Всичко зелено локално. db 487, web 493, ingest 84, etl 20, test-support 53, shared 45, config 10.
Клонът мина и през пълния CI на форка (
ydimitrof/sigma#4) — 5 от 5 проверки зелени:check,test,cacbg,semgrep,coverage-comment.Чеклист
Co-Authored-By:trailer към агентmidt-bg/sigma:mainpnpm typecheckминаваpnpm testминаваpnpm lintе чисто.env*или.dev.varsREADME.mdе обновен със седмия пакет;docs/не се нуждае от промяна