test: измерване на coverage и ratchet праг в CI (#93) - #216
Conversation
|
Прегледах #216 стриктно (харнесът за #93). Сигурност — издържана: trigger е Ratchet — коректен и fail-closed: липсващ отчет = твърда грешка, липсваща метрика = грешка, спад > tolerance = fail; Две точни бележки — за да не се чете покритието погрешно (не блокер за merge): 1) Derive слоят на ETL е ИЗВЪН обхвата на покритието — по конструкция. 2) Сред измерените workspace-и, branch подовете на accuracy слоевете са ниски. Иначе е точно каквото #93 иска и е сигурно направено. Approve; двете бележки са за roadmap-а на покритието. |
…ар fix Адресира ревю находките по PR midt-bg#216: - validateBaseline: празен/невалиден workspaces обект, изтрит или забравен ключ за workspace с test script, stale ключ и traversal/__proto__ ключове вече fail-ват gate-а вместо тихо да го изключват - --update печата per-workspace делти и предупреждава шумно при спад, за да не може реална регресия тихо да влезе в нов baseline - coverage exclude покрива и .test.tsx/.spec.* варианти - sticky comment: gh api | head пренаписан на два стъпки (pipefail SIGPIPE) - по-точно съобщение при липсващ отчет след crash-нал vitest процес
|
Прегледах новия връх
Approve остава. Двете ми roadmap бележки (ETL derive извън обхвата на coverage; ниски branch подове на db/config) са проследени в #217 (priority: high) — не блокират този PR. |
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ratchet-ът е реален: check-coverage.mjs прави process.exit(1) при спад, self-test-ът върви пръв, а validateBaseline е fail-closed (хваща и занулен workspaces).
Една дупка за проверка: findTestWorkspaces (:239) открива workspace само по наличие на scripts.test. apps/web има test (vitest run --config vitest.config.ts), но интеграционният lane от #177 върви под отделен config/test:integration — ако той не е включен в vitest.config.ts, ratchet-ът няма да мери точно покритието, което #177 добавя. Увери се, че umbrella test-ът покрива и двата runner-а, иначе новото integration coverage остава невидимо за gate-а.
…ар fix Адресира ревю находките по PR midt-bg#216: - validateBaseline: празен/невалиден workspaces обект, изтрит или забравен ключ за workspace с test script, stale ключ и traversal/__proto__ ключове вече fail-ват gate-а вместо тихо да го изключват - --update печата per-workspace делти и предупреждава шумно при спад, за да не може реална регресия тихо да влезе в нов baseline - coverage exclude покрива и .test.tsx/.spec.* варианти - sticky comment: gh api | head пренаписан на два стъпки (pipefail SIGPIPE) - по-точно съобщение при липсващ отчет след crash-нал vitest процес
d16f32c to
4a982eb
Compare
- @vitest/coverage-v8 през общ preset (vitest.shared.ts); vitest.config.ts за всичките 6 workspace-а с тестове, с експлицитен include за да не са невидими непокритите модули - scripts/check-coverage.mjs: per-workspace ratchet (lines/branches срещу комитнат coverage-baseline.json, 0.5pp толеранс), markdown таблица с делти + информативен общ сбор; --update вдига baseline-а локално - CI: pnpm test -- --coverage + ratchet gate (self-test първо), artifact с отчета, step summary, sticky PR коментар само за same-repo PR-и (fork token-ът е read-only); turbo test task пази coverage/** outputs - документирано в docs/review-testing.md
…ар fix Адресира ревю находките по PR midt-bg#216: - validateBaseline: празен/невалиден workspaces обект, изтрит или забравен ключ за workspace с test script, stale ключ и traversal/__proto__ ключове вече fail-ват gate-а вместо тихо да го изключват - --update печата per-workspace делти и предупреждава шумно при спад, за да не може реална регресия тихо да влезе в нов baseline - coverage exclude покрива и .test.tsx/.spec.* варианти - sticky comment: gh api | head пренаписан на два стъпки (pipefail SIGPIPE) - по-точно съобщение при липсващ отчет след crash-нал vitest процес
4a982eb to
924b276
Compare
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах стриктно механизма на връх 924b276. Силен ratchet — реален gate, не просто измерване.
Проверих:
scripts/check-coverage.mjsfail-ва CI приpct < base − tolerance(0.5pp) — истински gate,process.exit(1). CI го вика blocking (pnpm check:coverage, без|| true/continue-on-error), и self-тества checker-а преди gate (check:coverage:test) — добра дисциплина.- Anti-gaming-ът е точно правилен: explicit
includeglob вvitest.shared.ts— коментарът го казва изрично: без него v8 репортва само файловете, заредени от тестове, тъй че нов нетестван модул би бил невидим за ratchet-а. С explicit include нетестван source дърпа coverage-а надолу вместо да се скрие — най-честият начин ratchet-и да се заобикалят е затворен. Exclude листата е само test/dist/build/node_modules — нищо source не се крие. - Baseline-ите в
coverage-baseline.jsonса реални (apps/web 89.3/81.3, shared 94.8/78.1, db 82/65.5), не 0%. Умишлено намаление се изразява със свалянето на baseline-а в същия PR (reviewable).
Една бележка (strict):
apps/etlbaseline е 18.7% lines / 19.4% branches — а ETL-ът е точно там, където живеят accuracy бъговете на СИГМА (#154/#158 FX, #194/#195 канонични имена/ЕИК). Ratchet-ът замразява най-рисковия код на най-слабия под. Механизмът е коректен (записва текущото състояние, не е дефект в PR-а), но предлагам follow-up (вържете с #217) да вдигне etl floor-а — ratchet на 18.7% там е слаба защита. Branch-покритията (config 58.3, db 65.5) също са скромни — edge-бъговете се крият в клоните.
Одобрявам ratchet механизма; etl baseline-ът е за follow-up (#217), не блокер за този PR.
Resolve the add/add conflict in apps/etl/vitest.config.ts by keeping BOTH sides: main's sql-text-module plugin, the cloudflare:workers/workflows aliases and the 120s timeout (without them the Workflow suite cannot load), plus this branch's sharedCoverage(['src/**']) reporter config. Refresh coverage-baseline.json from the current corpus. The committed baseline predates the tests main gained since the branch was cut, so it no longer described reality - apps/etl alone reads 74.0% lines against a 18.7% floor. A ratchet 55pp below the real level cannot catch a regression, which is the whole point of the gate. Measured on this merge: etl 74.0/58.2, web 89.7/81.8, config 92.8/72.2, db 94.2/79.0, ingest 85.8/80.0, shared 95.4/80.0.
todorkolev
left a comment
There was a problem hiding this comment.
Одобрявам. Ratchet върху реалното текущо покритие, с честни числа вместо кръгли амбиции - включително ниските за ETL - е правилният подход: пази от регресия, без да принуждава хората да пишат тестове за показност. Само-тестващият се гейт (check:coverage:test преди check:coverage) също е хубав детайл.
Две неща поправих в клона:
Конфликтът add/add в apps/etl/vitest.config.ts - запазих и двете страни. Без плъгина за .sql модули и alias-ите към cloudflare:workers/workflows от main, наборът на Workflow-а изобщо не се зарежда; към тях добавих sharedCoverage от този клон и 120-секундния timeout.
Обнових coverage-baseline.json. Записаната базова линия беше отпреди тестовете, които main получи междувременно, тоест вече не описваше реалността - apps/etl мери 74,0% редове срещу праг 18,7%. Праг с 55 пункта под истинското ниво не може да хване регресия, което е целият смисъл на гейта. Измерено върху този merge: etl 74,0/58,2, web 89,7/81,8, config 92,8/72,2, db 94,2/79,0, ingest 85,8/80,0, shared 95,4/80,0.
За протокола: #254 предлага същия механизъм с прагове 96-100%; предпочитам този.
midt-bg#216 (coverage harness) merged to main, so the vendored harness reconciles to the canonical one. Also folds in the feature work that landed since the branch base — midt-bg#263 (worker-native FX load, rewritten refresh Workflow), midt-bg#252 (Bulstat EIK control code), midt-bg#210 (similar-contracts cohort) — and re-validates the whole suite against the moved source. Conflict resolutions: - apps/etl/vitest.config.ts: take upstream (needs the SQL text-module plugin + cloudflare:workers alias for midt-bg#263's real-Workflow index.ts). - vitest.shared.ts: keep our fixtures/json/md/d.ts excludes; add **/src/test/** (SQLite/workers stubs are test scaffolding, not product code). - coverage-baseline.json: keep our floors (baseline reconciliation to the merged tree's actuals follows in a separate commit). - apps/etl/src/index.test.ts: take upstream's FX integration test; restore the orchestration coverage it does not cover in a new mock-based control-flow test. - packages/db/src/queries/home.test.ts: union both added fakeDb params (singleOffer + capture). Post-merge fixes for source drift: - search.suggest.test.tsx: stub getDb (the route now wraps env in getDb()). - index.control-flow.test.ts (new): capped-window, zero-ingest, FX-uncovered, integrity-gate logger + failure (Error and non-Error), scheduled — restoring the run()/scheduled() branch coverage displaced by taking upstream's test. Full suite green: config 27, shared 56, ingest 116, etl 40, db 458, web 469.
Какво и защо
В monorepo-то нямаше измерване на test coverage — нито
@vitest/coverage-*, нито праг, а CI пускашеpnpm testбез--coverage. Този PR добавя харнеса от #93: измерване per workspace, ratchet праг който fail-ва CI при спад, и coverage делта видима в PR-а. Само харнесът — новите тестове бяха отделен обхват (#74, вече merged).@vitest/coverage-v8(root devDep) през общ presetvitest.shared.ts;vitest.config.tsза всичките 6 workspace-а с тестове, с експлицитенinclude— иначе v8 provider-ът брои само файловете, заредени от тестове, и нов непокрит модул би бил невидим за ratchet-а.coverage-baseline.json(seed-нат от реалните числа) +scripts/check-coverage.mjs— lines% и branches% на всеки workspace не може да падне >0.5pp под baseline-а. При покачване >1pp скриптът подканя--update(локално; никога в CI). Умишлен спад = сваляне на числото в baseline-а в същия PR, видимо на ревю. Общият сбор е информативен, смятан от сумирани covered/total бройки, не осреднени проценти.pnpm test -- --coverage(тестовете се пускат веднъж), ratchet gate със self-test първо (по модела на docs проверката), таблицата с делтите в step summary + artifactcoverage-report. Sticky PR коментар само за same-repo PR-и — fork PR-ите получават read-only token независимо отpermissions:, затова коментарният job е отделен, гейтнат поhead.repo.full_nameиcontinue-on-error;checkjob-ът остава с read-only token.testtask-ът декларираoutputs: ["coverage/**"], така че cache replay възстановява отчетите (проверено: FULL TURBO + наличенcoverage-summary.json).docs/review-testing.md.Бележка:
apps/web/tsconfig.node.jsonдобавя../../vitest.shared.tsвinclude(composite проектът изисква изброени файлове).packages/api-contractе извън baseline-а, докато няма тестове. Sticky-коментарният job може да се провери реално чак на този PR — всичко останало е проверено локално.Свързан issue
Closes #93.
Вид промяна
test/ci/build/chore— поддръжкаКак е тествано
pnpm check:coverage:test— 12/12 (node:test, адверсариални случаи: спад отвъд толеранса, спад в толеранса, независим branches ratchet, липсващ отчет/метрика, merge на бройки vs осредняване).pnpm test -- --coverage— 6/6 workspace-а зелени, всеки произвеждаcoverage/coverage-summary.json.packages/shared+2pp → exit 1 с actionable съобщение; +0.3pp → exit 0 (толеранс).outputs).pnpm typecheck,pnpm lint,pnpm check:docs— зелени.Чеклист
Co-Authored-By:trailermidt-bg/sigma:mainpnpm typecheckминаваpnpm test(поне за засегнатите пакети) минаваpnpm lintе чисто.env*или.dev.varsdocs/е обновена, ако промяната го налага