From aea0911bf917182df40c2ce0302bac9df50ec069 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 14:01:04 +0000 Subject: [PATCH] fix(devx): graduate metadata-fs and example-showcase out of TEST_DEBT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both packages' test layers were hidden from tsc by an include-shaped hole that #7353 taught TESTS_COVERED to see, and both were ledgered as measured TEST_DEBT rather than repaired. This repairs them via the #5286 route and deletes both entries in the same PR, which RECONCILED forces as a pair. Re-measured on the merged ref before repair; both matched their recorded numbers exactly (metadata-fs 6, example-showcase 4), so the entries are deleted against measurements rather than against hope. metadata-fs takes the sibling-config route. Its rootDir is `src` and `dev` emits (`tsc --watch`, outDir dist), so widening rootDir in the BUILD config would relocate dist/index.js and start emitting compiled tests; the sibling tsconfig.test.json carries the widened rootDir under noEmit instead and is named by the typecheck script. Its 6 errors were real: 5 dead imports (TS6133, from the root config's noUnusedLocals) and one TS2349 where an immediately-invoked drain loop made tsc narrow a captured `let` to `null`, so the optional call resolved to `never`. Binding the loop to a name before calling it restores the declared type; runtime behaviour is unchanged. example-showcase takes the widened-include route (#7312's shape for app-crm / app-todo) since its rootDir is already `.`. Its 4 TS2339 were `process.env` reads against the package's minimal ambient shim, fixed with the file-local `declare const process` idiom the package already uses in objectstack.config.ts and src/system/self-url.ts. The include glob is `e2e/**/*.spec.ts` and NOT `e2e/**/*`, holding the line the deleted entry's note drew: the wholesale glob would pull in e2e/global-setup.ts, a fixture rather than a test, and bill the layer 6 errors that are not its own. @objectstack/cli stays in the ledger — a programme, not a graduation. No other entry's recorded number is touched. --- .../e2e/bulk-capability-gate.spec.ts | 8 +++ .../app-showcase/e2e/detail-shapes.spec.ts | 7 +++ .../app-showcase/e2e/showcase-smoke.spec.ts | 8 +++ examples/app-showcase/tsconfig.json | 16 +++++- packages/metadata-fs/package.json | 2 +- packages/metadata-fs/test/contract.test.ts | 6 ++- .../test/watch-write-registration.test.ts | 18 +++++-- packages/metadata-fs/tsconfig.test.json | 51 +++++++++++++++++++ scripts/check-type-check-coverage.mjs | 25 +++++++-- 9 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 packages/metadata-fs/tsconfig.test.json diff --git a/examples/app-showcase/e2e/bulk-capability-gate.spec.ts b/examples/app-showcase/e2e/bulk-capability-gate.spec.ts index dd52e27120..f44c213b31 100644 --- a/examples/app-showcase/e2e/bulk-capability-gate.spec.ts +++ b/examples/app-showcase/e2e/bulk-capability-gate.spec.ts @@ -25,6 +25,14 @@ import { test, expect } from '@playwright/test'; * filtered on it (objectui#3492) — this spec is what notices either side * regressing. */ + +// Ambient `process` for the env read below — the showcase tsconfig doesn't pull +// in `@types/node`, and the package-global shim in test/node-shim.d.ts declares +// only `cwd()`. Same idiom (and same reason) as the declarations in +// objectstack.config.ts and src/system/self-url.ts: keeps `pnpm typecheck` green +// without widening the type surface. Playwright provides the real `process`. +declare const process: { env: Record }; + const APP = process.env.SHOWCASE_APP || 'com.example.showcase'; test('selection bar hides capability-gated inline defs from a caller without the grants', async ({ page }) => { diff --git a/examples/app-showcase/e2e/detail-shapes.spec.ts b/examples/app-showcase/e2e/detail-shapes.spec.ts index 9bfabe6581..9c5570f9fc 100644 --- a/examples/app-showcase/e2e/detail-shapes.spec.ts +++ b/examples/app-showcase/e2e/detail-shapes.spec.ts @@ -26,6 +26,13 @@ import { test, expect } from '@playwright/test'; * symbol. */ +// Ambient `process` for the env reads below — the showcase tsconfig doesn't pull +// in `@types/node`, and the package-global shim in test/node-shim.d.ts declares +// only `cwd()`. Same idiom (and same reason) as the declarations in +// objectstack.config.ts and src/system/self-url.ts: keeps `pnpm typecheck` green +// without widening the type surface. Playwright provides the real `process`. +declare const process: { env: Record }; + const APP = process.env.SHOWCASE_APP || 'com.example.showcase'; const API = process.env.SMOKE_API_URL || 'http://localhost:3000'; const recordUrl = (object: string, id: string) => diff --git a/examples/app-showcase/e2e/showcase-smoke.spec.ts b/examples/app-showcase/e2e/showcase-smoke.spec.ts index 2b6635f174..185078757f 100644 --- a/examples/app-showcase/e2e/showcase-smoke.spec.ts +++ b/examples/app-showcase/e2e/showcase-smoke.spec.ts @@ -6,6 +6,14 @@ import { test, expect } from '@playwright/test'; * / collapsed chart). Runs against the console the backend serves at /_console * (baseURL set in playwright.config.ts). Non-blocking nightly + manual. */ + +// Ambient `process` for the env read below — the showcase tsconfig doesn't pull +// in `@types/node`, and the package-global shim in test/node-shim.d.ts declares +// only `cwd()`. Same idiom (and same reason) as the declarations in +// objectstack.config.ts and src/system/self-url.ts: keeps `pnpm typecheck` green +// without widening the type surface. Playwright provides the real `process`. +declare const process: { env: Record }; + const APP = process.env.SHOWCASE_APP || 'com.example.showcase'; const base = (seg: string) => `/_console/apps/${APP}/${seg}`; diff --git a/examples/app-showcase/tsconfig.json b/examples/app-showcase/tsconfig.json index 69654652f9..f183228be9 100644 --- a/examples/app-showcase/tsconfig.json +++ b/examples/app-showcase/tsconfig.json @@ -10,5 +10,19 @@ "outDir": "./dist", "rootDir": "." }, - "include": ["src/**/*", "objectstack.config.ts", "test/**/*"] + // `e2e/**/*.spec.ts`, NOT `e2e/**/*` (#7923). This package took the widened- + // `include` route rather than a sibling `tsconfig.test.json` because its + // `rootDir` is already the package root, so nothing here needs neutralising — + // the same repair #7312 applied to app-crm / app-todo. + // + // The glob is spec-scoped on purpose, and it is the trap the TEST_DEBT entry + // that this change deletes was written to warn about: `e2e/**/*` would also + // pull in `e2e/global-setup.ts`, a Playwright fixture rather than a test, + // which carries 6 errors of its own (3 `process.env` reads plus 3 gaps in + // test/node-shim.d.ts — no `mkdirSync`/`writeFileSync` on the `node:fs` shim + // and no `node:path` module at all). Billing the test layer for a non-test + // file is what the ledger note measured around, so the repair holds the same + // line. `global-setup.ts` therefore stays outside this program; it is filed + // separately rather than silently folded in here. + "include": ["src/**/*", "objectstack.config.ts", "test/**/*", "e2e/**/*.spec.ts"] } diff --git a/packages/metadata-fs/package.json b/packages/metadata-fs/package.json index c21b81fd86..189fc87498 100644 --- a/packages/metadata-fs/package.json +++ b/packages/metadata-fs/package.json @@ -24,7 +24,7 @@ "clean": "rm -rf dist", "test": "vitest run", "test:watch": "vitest", - "typecheck": "tsc --noEmit" + "typecheck": "tsc --noEmit && tsc --noEmit -p tsconfig.test.json" }, "keywords": [ "objectstack", diff --git a/packages/metadata-fs/test/contract.test.ts b/packages/metadata-fs/test/contract.test.ts index 9cc34d6b38..853da5866f 100644 --- a/packages/metadata-fs/test/contract.test.ts +++ b/packages/metadata-fs/test/contract.test.ts @@ -1,11 +1,13 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, beforeEach, afterEach, expect } from 'vitest'; +// `describe`/`it`/`beforeEach`/`expect` are NOT imported here on purpose: this +// file registers the shared contract suite, which declares every case of its +// own, so the only hook this file itself uses is `afterEach` for cleanup. +import { afterEach } from 'vitest'; import fs from 'node:fs/promises'; import path from 'node:path'; import os from 'node:os'; import { runRepositoryContractTests } from '@objectstack/metadata-core/testing'; -import type { MetaRef } from '@objectstack/metadata-core'; import { FileSystemRepository } from '../src/index.js'; /** Track repos + tmpdirs across the contract suite for cleanup. */ diff --git a/packages/metadata-fs/test/watch-write-registration.test.ts b/packages/metadata-fs/test/watch-write-registration.test.ts index 548ed4f5e4..4255a77a2c 100644 --- a/packages/metadata-fs/test/watch-write-registration.test.ts +++ b/packages/metadata-fs/test/watch-write-registration.test.ts @@ -128,15 +128,27 @@ describe('FileSystemRepository watcher — writes register their own path (#7282 const iter = repo.watch({ org: 'system' }, 999)[Symbol.asyncIterator](); const events: MetadataEvent[] = []; let resolveNext: (() => void) | null = null; - void (async () => { + const nextEvent = () => new Promise((res) => { resolveNext = res; }); + // The drain loop is bound to a name and then called, rather than written as + // an immediately-invoked `void (async () => { … })()`. That is load-bearing + // for tsc, not style: for an IIFE, control-flow analysis narrows a captured + // `let` to its type AT THE POINT OF THE CALL — here `null`, from the + // initialiser above — so `resolveNext?.()` narrowed to `never` and reported + // TS2349 (one of the six errors this package's test layer was hiding until + // #7923 put it in front of tsc). A function expression that is not + // immediately invoked cannot be assumed to run at any particular point, so + // the declared `(() => void) | null` survives and the optional call is + // checked against what it can actually be. Runtime behaviour is identical: + // the loop still starts here, unawaited. + const drainEvents = async () => { for (;;) { const next = await iter.next(); if (next.done) return; events.push(next.value as MetadataEvent); resolveNext?.(); } - })(); - const nextEvent = () => new Promise((res) => { resolveNext = res; }); + }; + void drainEvents(); await Promise.race([scanned, sleep(EVENT_WAIT_MS)]); // The walk reached the type directory, so what follows is measuring the diff --git a/packages/metadata-fs/tsconfig.test.json b/packages/metadata-fs/tsconfig.test.json new file mode 100644 index 0000000000..d2c530fb09 --- /dev/null +++ b/packages/metadata-fs/tsconfig.test.json @@ -0,0 +1,51 @@ +// The TEST-layer type-check program (#7923, via the mechanism #5286/PR #5478 set +// for `packages/spec` and PR #5546 / #5476 carried to `packages/client` and +// `packages/metadata-core`). `tsconfig.json` beside this one stays as it is: it +// is the BUILD config, and `package.json`'s `typecheck` script NAMES this sibling +// (`tsc --noEmit -p tsconfig.test.json`), because a config no script invokes is +// exactly the phantom that mechanism exists to prevent. +// +// THE HOLE HERE IS THE INCLUDE-SHAPED ONE, structurally identical to +// `packages/metadata-core`: nothing was ever excluded — `include` is +// `["src/**/*"]` and all six test files live under a sibling `test/` tree, +// outside that root. No `exclude` entry names them, so the pre-#7353 +// exclude-shaped detector never saw them either; #7353 taught TESTS_COVERED the +// include-shaped form and this package surfaced with 6 hidden files. +// +// What differs from the build config, and what deliberately does NOT: +// - `rootDir` widens from `src` to the package root. It steers emit layout +// only, and this program emits nothing (`noEmit`), but inherited as `src` it +// reports TS6059 ("not under rootDir") for all six `test/**` files — the +// check being misconfigured, not the tests being wrong. Widening it in the +// BUILD config instead is not an option: `tsc` there emits (`dev`: +// `tsc --watch`, `outDir: dist`), so a package-root `rootDir` would relocate +// `dist/index.js` to `dist/src/index.js` — breaking `main`/`exports` — and +// start writing `dist/test/**/*.test.js`, which ci.yml gates against ("No +// compiled test files in any dist"). Emit constraints belong to the build +// config; this one has none. +// - MODULE SEMANTICS ARE UNTOUCHED. `@objectstack/metadata-core` is `"type": +// "module"` and so is this package, so the inherited NodeNext already reads +// these files as ESM — and it is the stricter reading, since it holds the +// `.js` import extensions this package must ship. Nothing to fix, so nothing +// is changed. +// - STRICTNESS IS UNTOUCHED. `strict`, `noUnusedLocals`, `noUnusedParameters`, +// `noImplicitReturns` and the rest are inherited from the root config. +// Nothing here may loosen a type rule; if a test does not compile, that is +// the finding. The five TS6133 this package's TEST_DEBT entry recorded were +// `noUnusedLocals` findings and were fixed in the tests, not silenced here. +// +// There is NO `test-typecheck-debt.json` beside this config, on purpose — same +// reasoning as `packages/metadata-core`: the whole test layer compiles at ZERO +// errors under it, so a per-file shrink-only ledger would hold nothing while +// costing this package a `tsx` dependency and two more scripts. A bare +// `tsc --noEmit -p tsconfig.test.json` is the strictly stronger gate at zero +// residue: ANY error here is red immediately, with no ledger to be added to. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": "." + }, + "include": ["src/**/*", "test/**/*"], + "exclude": ["node_modules", "dist"] +} diff --git a/scripts/check-type-check-coverage.mjs b/scripts/check-type-check-coverage.mjs index 7970e7f6ed..ca08835e0a 100644 --- a/scripts/check-type-check-coverage.mjs +++ b/scripts/check-type-check-coverage.mjs @@ -474,13 +474,34 @@ const EXEMPT = { // re-ratcheted. // // THE THREE ENTRIES THAT ARRIVED WITH #7353 -- cli 188, metadata-fs 6, -// example-showcase 4 -- are not new debt and did not slip past a ledger that is +// example-showcase 4 -- were not new debt and did not slip past a ledger that is // closed to it. They are debt this gate had never been able to SEE: TESTS_COVERED // asked whether an `exclude` named the tests, so a package that had simply never // pointed `include` at its test tree answered "covered" while nothing compiled a // line of it. All three were in that state before this ledger existed. 198 raw // errors is what the blind spot was worth on the day it was measured, and the // only thing that changed to surface them is the question. +// +// TWO OF THE THREE HAVE SINCE GRADUATED (#7923), which is the point of a TEST_DEBT +// entry: it is a holding position that makes a layer ratchet, not a destination. +// Both were re-measured on the merged ref before repair and both matched their +// recorded numbers exactly (metadata-fs 6, example-showcase 4), so the entries +// were deleted against measurements rather than against hope. +// - `@objectstack/metadata-fs` took the sibling-config route, because its +// `rootDir` is `src` and its `dev` script emits (`tsc --watch`, `outDir: +// dist`): a package-root `rootDir` in the BUILD config would relocate +// `dist/index.js` and start emitting compiled tests. `tsconfig.test.json` +// beside it is named by the `typecheck` script -- the #5286 mechanism, the +// same one `packages/metadata-core` uses for the structurally identical hole. +// - `@objectstack/example-showcase` took the widened-`include` route (#7312's +// shape for app-crm / app-todo), because its `rootDir` is already `.` and +// nothing needed neutralising. Its glob is `e2e/**/*.spec.ts` and NOT +// `e2e/**/*`, holding the same line the deleted entry's note drew: the +// wholesale glob would pull in `e2e/global-setup.ts`, a fixture rather than a +// test, and bill the test layer 6 errors that are not its own. That file is +// still read by no tsc program and is filed rather than folded in here. +// `@objectstack/cli` (188 raw across 56 files) is deliberately NOT part of that +// graduation -- it is a programme rather than a sitting, and its entry stands. const TEST_DEBT = { '@objectstack/plugin-approvals': { errors: 547, @@ -628,7 +649,6 @@ const TEST_DEBT = { '@objectstack/formula': { errors: 17, note: 'TS2591 x6 (`process`), TS2345 x3, TS2352 x3, TS1470 x2, TS2339 x2. Re-measured 17 at 5ab08428, up from 12; the TS2591 half doubled, which is the missing `types:["node"]` again rather than five new defects.' }, '@objectstack/trigger-record-change': { errors: 9, note: 'TS2353 x9 -- still the one unknown-property shape repeated, now in four files. Re-measured 9 at 5ab08428, up from 8.' }, '@objectstack/verify': { errors: 8, note: 'TS2835 x4, TS7006 x4. Re-measured 8 at 5ab08428, up from 6; both classes are the NodeNext pair from the top-of-ledger note.' }, - '@objectstack/metadata-fs': { errors: 6, note: 'TS6133 x5 (declared, never read -- the root config sets `noUnusedLocals`), TS2349 x1. Include-shaped: `include: ["src/**/*"]`, no `exclude` naming tests, and all 6 test files in a sibling `test/` tree, so this was invisible to the exclude-shaped detector (#7353). 5 of the 6 are in test/contract.test.ts; the TS2349 is in test/watch-write-registration.test.ts. Measured at b9f930b, recorded exactly.' }, '@objectstack/connector-mcp': { errors: 5, note: 'TS2339 x5. Re-measured 5 at 5ab08428, exact.' }, '@objectstack/connector-openapi': { errors: 5, note: 'TS2339 x5. Re-measured 5 at 5ab08428, exact.' }, '@objectstack/http-conformance': { @@ -639,7 +659,6 @@ const TEST_DEBT = { + 'this package\'s own code. Raw `tsc --noEmit` counts are what every number in these ledgers means, ' + 'so they are counted here rather than filtered out -- but they are not this package\'s debt to fix.', }, - '@objectstack/example-showcase': { errors: 4, note: 'TS2339 x4. The one entry here whose hidden files are Playwright specs rather than vitest tests: `include` names `src/**/*`, `objectstack.config.ts` and `test/**/*` -- so the vitest layer is compiled and the `e2e/` tree beside it is not. Three specs, 4 errors (detail-shapes.spec.ts x2, bulk-capability-gate.spec.ts x1, showcase-smoke.spec.ts x1). Measured by adding the three spec files to `include` one at a time, NOT `e2e/**/*`, which would also have pulled in e2e/global-setup.ts and billed this layer 6 errors from a file that is not a test. Sibling to #7312, which repaired app-crm and app-todo the same way and could not move this gate\'s count because neither app had ever counted toward it.' }, '@objectstack/platform-objects': { errors: 3, note: 'TS2339 x2, TS7006 x1. Re-measured 3 at 5ab08428, exact.' }, '@objectstack/plugin-sharing': { errors: 3, note: 'TS6133 x2, TS18048 x1. Re-measured 3 at 5ab08428, exact.' }, '@objectstack/service-sms': { errors: 1, note: 'TS2493 x1, in transports.test.ts. Re-measured 1 at 5ab08428 and still 1 at e8db1a230, after two more hidden test files: #5773 added sms-manifest-providers.contract.test.ts and #2814 / PR #6042 added sms-daily-quota.test.ts. The file count moved twice while the error count did not -- both new files are type-clean with the exclusion lifted.' },