diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7148a599d..730906cc6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -401,7 +401,7 @@ "run": "deno install --frozen" }, { - "run": "deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='.*module\\.f\\.(ts|mjs)'" + "run": "deno task cov" } ] }, diff --git a/CHANGELOG.md b/CHANGELOG.md index d5397979a..a5e5f5766 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,15 @@ history. ## Unreleased +- **BREAKING CHANGES:** `fjs/ci/deno` no longer exports `coverageInclude`. + The Deno CI job runs `deno task cov`, so `deno.json` owns the coverage + filter, matching how the Node jobs leave `npm run cov` to `package.json` + [#1512](https://github.com/functionalscript/functionalscript/pull/1512) +- `npm run cov` names its test entrypoint instead of relying on + `node --test` default discovery, which silently reported no tests on some + Node versions. The dead `.f.ts` extension is dropped from the Node and + Deno coverage filters + [#1512](https://github.com/functionalscript/functionalscript/pull/1512) - **BREAKING CHANGES:** `fjs/djs/examples/input.f.ts` and `fjs/djs/examples/m.f.ts` migrate to `.f.mjs`. Being `.mjs` they now ship in the tarball, which `.f.ts` did not diff --git a/deno.json b/deno.json index ea2a2da39..6f8350666 100644 --- a/deno.json +++ b/deno.json @@ -2,7 +2,7 @@ "tasks": { "fjs": "deno run --allow-read --allow-write --allow-env --allow-net --allow-sys ./fjs/module.mjs", "test": "deno test --allow-read --allow-env --allow-sys", - "cov": "deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='.*module\\.f\\.(ts|mjs)'" + "cov": "deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='.*module\\.f\\.mjs'" }, "fmt": { "indentWidth": 4, diff --git a/fjs/ci/deno/module.f.mjs b/fjs/ci/deno/module.f.mjs index 0eccfaf5b..d62c40aa0 100644 --- a/fjs/ci/deno/module.f.mjs +++ b/fjs/ci/deno/module.f.mjs @@ -2,6 +2,11 @@ * CI step builder for Deno: installs the pinned Deno version and runs the * FunctionalScript package smoke test plus Deno coverage in one canonical job. * + * Coverage runs through `deno task cov`, so `deno.json` owns the permission set + * and the coverage filter, exactly as `npm run cov` leaves them to + * `package.json` for the Node jobs. The two `cov` definitions select the same + * modules and should stay semantically equal. + * * @module * * @import { MetaStep } from '../common/types.ts' @@ -10,17 +15,6 @@ import { deno } from '../config/module.f.mjs' import { install, test, uses } from '../common/module.f.mjs' -const denoTest = 'deno test --allow-read --allow-env --allow-sys' - -/** - * The regular expression selecting FunctionalScript implementation modules for - * Deno coverage. Both authored extensions are included so a module migrated - * from `.f.ts` to `.f.mjs` stays in the report. Keep it semantically equal to - * the `--test-coverage-include` list in `package.json` and to the `cov` task in - * `deno.json`. - */ -export const coverageInclude = '.*module\\.f\\.(ts|mjs)' - /** @type {(version: string) => readonly MetaStep[]} */ export const denoSteps = version => [ install(uses('denoland/setup-deno', { 'deno-version': deno })), @@ -31,5 +25,5 @@ export const denoSteps = version => [ install({ run: `deno install -g -A --minimum-dependency-age=0 npm:functionalscript@${version}` }), test({ run: `deno run -A --minimum-dependency-age=0 npm:functionalscript@${version} test` }), test({ run: 'deno install --frozen' }), - test({ run: `${denoTest} --coverage && deno coverage --include='${coverageInclude}'` }), + test({ run: 'deno task cov' }), ] diff --git a/fjs/ci/deno/proof.f.mjs b/fjs/ci/deno/proof.f.mjs index d6d2c60db..41d773a36 100644 --- a/fjs/ci/deno/proof.f.mjs +++ b/fjs/ci/deno/proof.f.mjs @@ -1,25 +1,21 @@ -import { coverageInclude, denoSteps } from './module.f.mjs' +import { denoSteps } from './module.f.mjs' import { toSteps } from '../common/module.f.mjs' import { assert, assertEq } from '../../asserts/module.f.mjs' /** @type {(version: string) => readonly string[]} */ const coverageRuns = version => toSteps(denoSteps(version)) - .flatMap(s => s.run !== undefined && s.run.includes('deno coverage') ? [s.run] : []) + .flatMap(s => s.run !== undefined && s.run.includes('cov') ? [s.run] : []) export const proof = { - // A regression guard: dropping either authored implementation extension - // from the Deno coverage filter silently removes those modules from the - // CI coverage report while CI still passes. - coverageInclude: () => { - assertEq(coverageInclude, '.*module\\.f\\.(ts|mjs)') - }, + // A regression guard: the job must delegate coverage to `deno.json`'s `cov` + // task. Inlining the command here instead would give the coverage filter a + // second owner that can silently drift from `deno.json`. coverageStep: () => { const runs = coverageRuns('0.0.0') assertEq(runs.length, 1) const [run] = runs - assert(run !== undefined) - assert(run.includes(`deno coverage --include='${coverageInclude}'`)) + assertEq(run, 'deno task cov') }, installsPinnedVersion: () => { const runs = toSteps(denoSteps('1.2.3')) diff --git a/fjs/ci/proof.f.mjs b/fjs/ci/proof.f.mjs index 30eb7e99a..7d88cb952 100644 --- a/fjs/ci/proof.f.mjs +++ b/fjs/ci/proof.f.mjs @@ -4,7 +4,6 @@ import { ci, main } from './module.f.mjs' import { functionalscript, node } from './config/module.f.mjs' import { nodeNixJobs } from './node/module.f.mjs' -import { coverageInclude } from './deno/module.f.mjs' import { utf8, utf8ToString } from '../text/module.f.mjs' import { empty as emptyVec } from '../types/bit_vec/module.f.mjs' import { test, ubuntu, parseGitHubAction } from './common/module.f.mjs' @@ -163,7 +162,7 @@ export const proof = { assert(hasRun(`deno install -g -A --minimum-dependency-age=0 npm:functionalscript@${functionalscript}`)(gha), 'expected configured-version deno install cache') assert(hasRun('deno install --frozen')(gha), 'expected deno lock install') assert(hasRun(`deno run -A --minimum-dependency-age=0 npm:functionalscript@${functionalscript} test`)(gha), 'expected configured-version deno install') - assert(hasRun(`deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='${coverageInclude}'`)(gha), 'expected limited-permission deno coverage') + assert(hasRun('deno task cov')(gha), 'expected deno coverage task') assert(hasRun(`bun install -g functionalscript@${functionalscript}`)(gha), 'expected configured-version bun cache') assert(hasRun('bun install --frozen-lockfile')(gha), 'expected bun lock install') assert(hasRun(`bunx functionalscript@${functionalscript} test`)(gha), 'expected configured-version bun install') diff --git a/fjs/emergent_testing/module.f.mjs b/fjs/emergent_testing/module.f.mjs index 42ac66fdc..325cbfe0d 100644 --- a/fjs/emergent_testing/module.f.mjs +++ b/fjs/emergent_testing/module.f.mjs @@ -122,10 +122,12 @@ export const collectTests = (path, throws, v) => { export const registerModule = (ctx, k, v, star) => { /** @type {(ctx: TestContext, entry: _TestAndPath) => Effect} */ const registerOne = (ctx, [path, { fn, throws }]) => { - // ' *' (non-empty only for Bun) signals that all sub-tests run - // inline inside this single registration. Not appended to throw-tests since - // those never produce sub-tests. The path already contains '.throw' when a - // test is expected to throw, so no extra suffix is needed. + // `star` (non-empty for Bun and for Node below the 26 baseline) signals + // that all sub-tests run inline inside this single registration, so an + // external runner reports fewer tests than `fjs t` for the same suite. + // Not appended to throw-tests since those never produce sub-tests. The + // path already contains '.throw' when a test is expected to throw, so no + // extra suffix is needed. const base = fmtImport(k, path) const name = throws ? base : `${base}${star}` return test(ctx, name, throws, (/** @type {TestContext} */ t) => diff --git a/fjs/emergent_testing/todo/f-mjs-test-and-coverage.md b/fjs/emergent_testing/todo/f-mjs-test-and-coverage.md index 60845eaaa..c6bcbf0fc 100644 --- a/fjs/emergent_testing/todo/f-mjs-test-and-coverage.md +++ b/fjs/emergent_testing/todo/f-mjs-test-and-coverage.md @@ -9,7 +9,8 @@ Tooling recognition of `.f.mjs` has landed: `shouldLoad` in `fjs/dev/module.f.mjs` matches `.f.mjs`, `npm run cov` and `deno task cov` include `module.f.mjs`, and the canonical Deno CI generator -(`fjs/ci/deno/module.f.mjs`) exports `coverageInclude` with a regression proof. +(`fjs/ci/deno/module.f.mjs`) delegates to `deno task cov` with a regression +proof, so `deno.json` is the only owner of the Deno coverage filter. What is still missing is end-to-end evidence from an actual `.f.mjs` runtime fixture. No repository fixture currently proves that a migrated diff --git a/package.json b/package.json index 7961d2b81..b98d1ca1f 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "scripts": { "prepack": "tsc --noEmit false --emitDeclarationOnly && tsc --noEmit false --declaration false", "test": "tsc && node ./fjs/module.mjs t", - "cov": "node --test --experimental-test-coverage --test-coverage-include=**/module.f.ts --test-coverage-include=**/module.f.mjs", + "cov": "node --test --experimental-test-coverage --test-coverage-include=**/module.f.mjs fjs/emergent_testing/all.test.ts", "start": "node ./fjs/module.mjs", "ci-update": "node ./fjs/module.mjs ci && node ./fjs/module.mjs r ./fjs/nanvm/update/module.f.mjs", "dev-update": "node ./fjs/module.mjs r ./fjs/dev/update/module.f.mjs", diff --git a/todo/migrate-typescript-to-mjs.md b/todo/migrate-typescript-to-mjs.md index ca008aa4a..ae6980678 100644 --- a/todo/migrate-typescript-to-mjs.md +++ b/todo/migrate-typescript-to-mjs.md @@ -820,38 +820,48 @@ Each item below is stated with the measurement that produced it, so the next person can re-check rather than re-derive. Counts are as of [#1505](https://github.com/functionalscript/functionalscript/pull/1505). -- [ ] **Make `npm run cov` report real coverage.** It has been vacuous for - several PRs — reviewers keep reporting `100.00` over 0 tests and having to - exclude coverage from their verification — but *not* because of the - include globs, so do not start there. The script already passes - `--test-coverage-include=**/module.f.mjs` alongside the now-dead - `**/module.f.ts` (the `.mjs` glob was added in #1422). Dropping the dead - glob is worth doing but changes nothing measurable: - `--test-coverage-include` only filters which files appear in the report, - so it cannot make a run that executed nothing report something. - - The cause is test *discovery*. With no path arguments `node --test` looks - for its own default patterns (`*.test.*`, `test.*`, `*-test.*`, - `test-*.*`, `*_test.*`, `test/**`); the repo's proofs are `proof.f.mjs` / - `module.f.mjs` and match none of them. The only file in the tree that does - match is `fjs/emergent_testing/all.test.ts`, and what happens then is - Node-version-dependent — on v23 the run reports 0 tests, while on v22.22.2 - it discovers that file and executes the suite through it. The real suite - runs via the repo's own runner (`npm test` -> `node ./fjs/module.mjs t`, - 2495 tests). `scenarios/run.sh` corroborates the discovery problem: it - hard-links `all.ts` to `_all.test.ts` precisely so `node --test` will find - it. - - So restoring the signal means giving `node --test` entrypoints it actually - runs, or collecting coverage through `fjs`'s runner — not editing globs. - The cheapest candidate is naming the entrypoint in the script, which is - measured to work on both versions: adding - `fjs/emergent_testing/all.test.ts` as a path argument to `cov` yields - `tests 2431 / pass 2431 / fail 0` and a real per-file report on v22.22.2, - and the same file named explicitly also runs on v23. Confirm it reports on - whichever Node CI uses before adopting it, and pin that version — the - 2431 here against 2495 from `npm test` is a second discrepancy worth - understanding rather than papering over. +- [x] **Make `npm run cov` report real coverage.** Done: `cov` now names its + entrypoint (`fjs/emergent_testing/all.test.ts`) instead of relying on + `node --test` default discovery, and the dead `**/module.f.ts` glob is + gone. + + The diagnosis above was right and the "vacuous for several PRs" framing + was wrong, so both are recorded here rather than deleted. The cause was + test *discovery*: with no path arguments `node --test` looks for its own + default patterns (`*.test.*`, `test.*`, `*-test.*`, `test-*.*`, + `*_test.*`, `test/**`); the repo's proofs are `proof.f.mjs` / + `module.f.mjs` and match none of them, and the only file in the tree that + does is `fjs/emergent_testing/all.test.ts`. Whether that file is picked up + is Node-version-dependent, because it is TypeScript. Measured with the + pre-fix command + (`node --test --experimental-test-coverage --test-coverage-include=**/module.f.ts --test-coverage-include=**/module.f.mjs`): + + | Node | tests | coverage report | + |---|---|---| + | 22.22.2 | 2431 | yes, `all files 99.93` | + | 23.11.1 | **0** | **none** | + | 24.18.1 | 2431 | yes, `all files 99.93` | + | 26.7.0 | 2495 | yes, `all files 99.93` | + + So every Node version CI actually runs (`22.23.2`, `24.18.1`, `26.7.0` in + the `node22` / `node24` / `node26` jobs) already reported real coverage; + v23 — released, EOL, not in CI — is the version that reports nothing. The + fix removes the dependency on default discovery altogether: naming the + entrypoint yields the same counts and the same `99.93` report on 22, 24 + and 26, and turns v23's `tests 0` into `tests 2431` with a full report. + + The 2431-vs-2495 gap is **not** a second defect. It is exactly the 64 + sub-tests — those reachable only through a test function's *return value*. + `usesInlineTestContext` (`fjs/effects/node/module.f.mjs`) selects the + flattened registration strategy below the Node 26 baseline, where + `inlineContext` runs sub-tests inside their parent registration instead of + declaring them to the framework, so the framework counts the parent only. + Node 26 uses native `expectFailure` and nested registration, and counts + all 2495, matching `npm test` (`node ./fjs/module.mjs t`). Measured on + 26.7.0: 2431 top-level plus 64 nested `✔` lines; on 24.18.1: 2431 + top-level, 0 nested, of which 2376 carry the ` ...` inline marker (the + remaining 55 are throw-tests, which never produce sub-tests). Same work + executed either way — only the reported count differs. - [ ] **Settle whether generated `types.js` is required for portable resolution.** This gates the two items after it and is the one open correctness risk for published consumers. After `npm run prepack`,