Skip to content

test: split unit tests into node:test suite, add c8 coverage#271

Merged
robertsLando merged 12 commits intomainfrom
test/node-test-unit-suite-267
Apr 24, 2026
Merged

test: split unit tests into node:test suite, add c8 coverage#271
robertsLando merged 12 commits intomainfrom
test/node-test-unit-suite-267

Conversation

@robertsLando
Copy link
Copy Markdown
Member

@robertsLando robertsLando commented Apr 24, 2026

Summary

Phases 1 + 2 of #267. Phase 3 (migrating e2e onto node:test) intentionally dropped — the custom test/test.js harness is battle-tested across 169 e2e directories and the maintainer prefers to keep it.

  • Phase 1test/unit/*.test.ts on Node's built-in node:test, loaded via esbuild-register so lib sources are imported directly (no yarn build). Three in-process tests migrated out of the e2e harness:
    • test-00-sea-picker/test/unit/sea-picker.test.ts
    • test-48-common/test/unit/common.test.ts (platform-split via describe(..., { skip }))
    • test-50-config-parse/test/unit/config-parse.test.ts (ad-hoc t() harness replaced with node:test + beforeEach/afterEach)
    • Old dirs deleted; test-00-sea-picker removed from the npmTests list in test/test.js.
    • New scripts: yarn test:unit, yarn test:unit:watch.
  • Phase 1.5 (extended coverage) — now that the suite exists, filled in the pure-pure modules identified as priorities in the issue:
    • compress-type.test.ts — CompressType enum stability + Zstd accessor branches
    • detector.test.ts — parse() tolerant flags, detect() warn-don't-throw, every public visitor
    • esm-transformer.test.ts — rewriteMjsRequirePaths across quote styles, transformESMtoCJS for top-level-await wrapping (with/without exports) and import.meta shim injection
    • common.test.ts extended: isPackageJson / isDot{JS,JSON,NODE} / unlikelyJavascript / replaceSlashes / isRootPath / isESMPackage / isESMFile (last two use an mkdtemp fixture)
    • config-parse.test.ts extended: isConfiguration, stringifyTarget, PKGRC_FILENAMES precedence, findPkgrc (mkdtemp fixture)
    • 162 assertions run in ~1.7s (up from 96 in ~1.3s).
  • Phase 2c8 (added as --exact devDep) wraps NODE_V8_COVERAGE; the e2e path inherits the env var through utils.pkg.sync and captures subprocess coverage without code changes. Scripts: yarn coverage:unit, yarn coverage:e2e (--clean=false), yarn coverage for the merged report. Excludes wired in .c8rc.json.
  • CIyarn test:unit now runs in the existing build matrix (3 OS × Node 22/24) so common.test.ts's win32 branch and Node 24 get coverage at ~1s/cell.
  • Toolingtsconfig.test.json extends the base for typescript-eslint's parser to see test/unit/**/*; eslint.config.js's parserOptions.project is now an array.
  • Integration-heavy modules (walker, producer, packer, fabricator, resolver, sea-assets, refiner, follow, mach-o, chmod, bin, help, colors, index, most of sea) intentionally not unit-tested — they inherently need fixtures / subprocesses / binary manipulation that the e2e suite already exercises end-to-end.

Coverage baseline (unit only)

From yarn coverage:unit:

File Coverage
lib/ overall stmts 35.62%
lib/ overall branch 79.78%
common.ts 79.20%
compress_type.ts 77.27%
config.ts 67.69%
detector.ts 80.53%
esm-transformer.ts 74.88%
options.ts 78.26%
sea.ts 25.86% (pickMatchingHostTargetIndex)

Other lib/*.ts files are exercised only by e2e today; the merged yarn coverage report is the right way to see total coverage.

Test plan

  • yarn test:unit — 162 pass, 0 fail, ~1.7s
  • yarn coverage:unit — lcov + text reporters emit, baseline captured
  • yarn lint — clean
  • npx tsc -p tsconfig.test.json --noEmit — clean
  • Spot-run one e2e test (test-50-api) to confirm test/test.js harness still works after npmTests edit
  • First CI run (96-test version) — all 27 checks green on Linux / macOS / Windows × Node 22 / 24
  • CI green on extended suite

Closes #267.

🤖 Generated with Claude Code

Three tests under test/test-XX-*/ were already in-process assertions
(sea-picker, common, config-parse) — they required('../../lib-es5/...')
without ever spawning pkg or producing a binary. Keeping them in the
e2e harness paid the per-directory spawn cost for trivial checks and
mixed "lib regression" with "build regression" under one runner.

Migrate them to test/unit/*.test.ts on Node's built-in node:test runner,
loaded via esbuild-register so the lib/ TS source is imported directly —
no yarn build required. 96 assertions run in ~1.3s.

Coverage is wired through c8 (thin reporter over NODE_V8_COVERAGE).
utils.pkg.sync already spreads process.env into the spawned pkg CLI,
so coverage:e2e captures subprocess coverage with zero code changes.
A second c8 run with --clean=false merges unit + e2e into one report.

Baseline from yarn coverage:unit: lib/ 25.49% statements / 85.78%
branch. Highest per-file: config.ts 66.73%, common.ts 57.60%, sea.ts
25.86%.

CI: yarn test:unit rides the existing build matrix (3 OS × Node 22/24),
giving common.test.ts's win32 branch and Node 24 coverage at ~1s/cell.

Closes #267.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces a fast unit-test suite alongside the existing e2e harness, and adds V8-based coverage reporting to improve feedback speed and visibility for lib/ behavior.

Changes:

  • Adds node:test-based unit tests in test/unit/*.test.ts, migrating three prior in-process “e2e” tests into the unit suite.
  • Adds c8 coverage tooling (+ config) and new yarn scripts for unit/e2e coverage reporting.
  • Updates CI/docs/tooling configs to recognize and run the new unit suite.

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
yarn.lock Locks new dev dependency graph for c8 and its transitive deps.
tsconfig.test.json Adds a TS config for typechecking unit tests alongside lib/**.
test/unit/sea-picker.test.ts New unit coverage for SEA host/target matching helper.
test/unit/config-parse.test.ts Migrates config parsing/flag resolution validation to node:test.
test/unit/common.test.ts Migrates lib/common path/snapshot helpers test with platform-skipped suites.
test/test.js Removes the migrated test-00-sea-picker directory from the e2e npm test list.
test/test-50-config-parse/main.js Deletes old in-process config parser test previously living in e2e structure.
test/test-48-common/main.js Deletes old in-process common test previously living in e2e structure.
test/test-00-sea-picker/main.js Deletes old in-process SEA picker test previously living in e2e structure.
package.json Adds c8 devDep and introduces test:unit + coverage scripts; wires unit tests into yarn test.
eslint.config.js Updates TS parser config to include both tsconfig.json and tsconfig.test.json.
docs-site/development.md Documents the new unit suite and coverage workflows.
CLAUDE.md Updates quick reference to mention yarn test:unit and clarify build requirement is for e2e.
.gitignore Ignores coverage/ outputs.
.github/workflows/ci.yml Runs yarn test:unit in the existing OS/Node build matrix.
.github/copilot-instructions.md Updates contributor instructions to describe unit suite + coverage commands.
.claude/rules/testing.md Updates internal testing guidance to include the unit suite and coverage commands.
.c8rc.json Adds c8 include/exclude/reporting configuration for lib/**.

Comment thread docs-site/development.md
Comment thread test/unit/config-parse.test.ts Outdated
Comment thread package.json
robertsLando and others added 3 commits April 24, 2026 11:06
…r, common, config

Add three new test files and extend the two existing ones. Focus is on the
pure-pure modules — AST helpers, path predicates, enum gates, esbuild-based
transformation — without touching the integration-heavy ones (walker,
producer, packer, fabricator, resolver, sea-assets) that inherently need
fixtures pkg already stresses via the e2e suite.

  New: test/unit/compress-type.test.ts
    CompressType enum layout (bytecode-format stability guard) and
    getZstdCompressSync/Stream availability branches.

  New: test/unit/detector.test.ts
    parse() tolerant flags (decorator-legacy, top-level return,
    import.meta), detect() warn-don't-throw + descent control, and every
    public visitor (successful / nonLiteral / malformed / useSCWD) with
    literal, dynamic-expr, and must/may-exclude hints.

  New: test/unit/esm-transformer.test.ts
    rewriteMjsRequirePaths regex across quote styles + relative/bare
    specifiers; transformESMtoCJS for top-level-await wrapping (with and
    without exports), import.meta shim injection, and esbuild-failure
    handling.

  Extended: test/unit/common.test.ts
    isPackageJson / isDot{JS,JSON,NODE} / unlikelyJavascript /
    replaceSlashes / isRootPath / isESMPackage / isESMFile (latter two
    use an mkdtemp fixture — still ~10ms, no subprocess).

  Extended: test/unit/config-parse.test.ts
    isConfiguration, stringifyTarget, PKGRC_FILENAMES precedence, and
    findPkgrc against an mkdtemp fixture.

Coverage baseline (yarn coverage:unit):
  lib/ overall:      25.49% → 35.62% stmts / 79.78% branch
  common.ts:         57.60% → 79.20%
  detector.ts:       13.64% → 80.53%
  esm-transformer:   20.04% → 74.88%
  compress_type.ts:  59.09% → 77.27%

Totals: 162 pass / 0 fail in ~1.7s (up from 96 in ~1.3s).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h edges

Hot path: parseTargets is called on every pkg invocation. Previously
untestable because it was internal — exported alongside differentParts
and stringifyTargetForOutput with a test-only comment, matching the
pattern already used for pickMatchingHostTargetIndex.

  config.ts target parser:
    'host' short-circuit, single-token host-fill (platform/arch/
    nodeRange), full triple in any token order, repeated-dash empty
    tokens, fancy-alias ('windows' → 'win'), unknown-token error with
    offending spec in message, list-order preservation, round-trip
    with stringifyTarget, differentParts on every dimension combo,
    stringifyTargetForOutput with each subset of varying axes.

  esm-transformer.ts closes the remaining success branch:
    Top-level await + imports but no exports — imports are kept at
    top level so esbuild can rewrite them, body goes into the async
    IIFE. Also: for-await-of at top level triggers the same wrap;
    await inside a function does NOT trigger it.

  detector.ts edges:
    visitorMalformed on require(dynamicExpr); visitorUseSCWD with
    multi-arg reconstruction, zero args (empty alias), 'url.resolve'
    (non-path object), bare resolve() call.

  common.ts toNormalizedRealPath:
    missing-path pass-through, existing-dir realpath, symlink → target
    (symlink subtest skipped on Windows — requires admin/Dev Mode).

Coverage baseline: 35.62% → 37.42% stmts / 82.08% branch
  esm-transformer.ts: 74.88% → 87.09%  (TLA branch closed)
  config.ts:          67.69% → 73.51%  (target parser exercised)
  common.ts:          79.20% → 81.33%  (realpath covered)
  detector.ts:        80.53% → 81.69%

Totals: 193 pass / 0 fail in ~1.8s (up from 162 in ~1.7s).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rage docs

- test/unit/config-parse.test.ts: the `infoed` capture buffer was never
  asserted. Dropped it and silenced `log.info` directly so
  resolveTargetList/parseTargets fallback paths don't print during tests.
- docs-site/development.md: `yarn coverage:e2e` uses `--clean=false` and
  therefore accumulates on top of whatever is in `coverage/tmp` — it is
  not "e2e-only" in general. Docs now state the actual behavior: fresh →
  e2e-only, after `yarn coverage:unit` → merged.

No script change — the `--clean=false` design is intentional per #267
(unit + e2e accumulate via a single temp dir).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 22 changed files in this pull request and generated no new comments.

robertsLando and others added 6 commits April 24, 2026 11:29
Per-PR coverage signal via Codecov. Unit suite only — `yarn coverage` for
the full merged (unit + e2e) report stays a follow-up job (30+ min is too
expensive per PR; issue #267 plans a nightly or on-label job for that).

- Adds `yarn coverage:unit` step to build_artifact (Linux, Node 22) after
  lint/build, then uploads coverage/lcov.info via codecov-action@v5.
  build_artifact already runs on every PR and produces the distributable
  bundle, so coverage fits naturally without adding a separate job.
- codecov.yml: informational status (never fail a PR on coverage swings,
  since e2e coverage isn't uploaded yet), ignore compiled output / tests
  / docs-site / dictionary, carryforward disabled on the `unit` flag.
- README: Codecov badge next to CI badge, labeled "Coverage (unit)" so
  the scope is obvious at a glance.

Requires: Codecov GitHub App installed on yao-pkg and CODECOV_TOKEN set
as a repo secret (optional on public repos but recommended for PR uploads).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pins uploads to the yao-pkg/pkg Codecov project (prevents mis-routing on
forks) and adds a readable step label. Path stays coverage/lcov.info
(what .c8rc.json emits); flags: unit tags this as the unit-suite upload
so a future e2e upload can use a separate flag.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Standing up the missing half of #267's Phase 2 acceptance criteria:
"CI runs the unit suite on every PR (cheap) and the full coverage job
on-label or nightly (expensive — full e2e)."

- .github/workflows/coverage-nightly.yml: daily 03:00 UTC job (also
  workflow_dispatch) that runs yarn build + yarn coverage:e2e on Node 22
  / Linux and uploads coverage/lcov.info to Codecov with `flags: e2e`.
  Reuses the same ~/.pkg-cache key as test.yml so it doesn't refetch all
  pkg-fetch binaries each night.
- README: badge alt text is now "Coverage" — once the first nightly
  lands, Codecov merges the `unit` (from PR / main pushes) and `e2e`
  flag uploads per commit, so main-branch reports reflect the combined
  picture. PRs still show unit-only signal via flags.

Codecov's flag-based merge means this doesn't touch the per-PR upload
path — the existing build_artifact step keeps uploading unit with the
fast ~2s feedback, and e2e drifts in once per day.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tals

Without carryforward, Codecov treats each commit independently. PR commits
only have a `unit` upload (~37% of lib/) because the nightly e2e job runs
on main, not on PR branches — so PR reports and status were showing
unit-only numbers and the user perceived "no merge".

carryforward: true tells Codecov to reuse the most recent upload tagged
with that flag when the current commit doesn't have its own. A PR now sees:
  - unit flag: direct from this PR's build_artifact run
  - e2e flag:  carried forward from the last nightly on main
  - Total:     union of the two → the merged number we want

Trade-off: carried-forward e2e data is slightly stale for PR-new code,
but e2e covers integration-heavy modules (walker/producer/packer/fabricator)
that PRs rarely touch at the test-contributing level. Close enough to truth
to be useful, and the nightly refresh catches drift within 24 hours.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ci.yml and codecov.yml were written when the e2e coverage upload was
still a TODO. That job now ships in coverage-nightly.yml and the
flag-merge via carryforward is already live. Update comments to
describe the current state instead of a past plan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ghtly

We already run the full e2e suite on every PR — 18 cells across
test_22/test_24/test_host × 3 OS × Node 22/24. Instead of re-running it
nightly under c8, wrap two canonical existing cells (ubuntu × matching
host/target) so they double as coverage producers:

  test_22 / (22.x, ubuntu-latest) → yarn coverage:e2e  → upload `flags: e2e`
  test_24 / (24.x, ubuntu-latest) → yarn coverage:e2e:24 → upload `flags: e2e`

Codecov merges same-flag uploads per commit (max per line), so both Node
versions contribute. Windows/macOS cells stay unchanged — they exercise
the same lib/ paths, so piping them through c8 would just duplicate data.

Benefits over the nightly approach:
  - e2e coverage is per-PR real-time, no 24h lag.
  - Merged unit+e2e shows on every PR natively (both flags on same commit),
    no reliance on carryforward to fake the merge.
  - No separate workflow to maintain.

Overhead: c8 adds ~10-20% to the wrapped cells' runtime (e.g. 4m → 5m).

carryforward stays enabled as a safety net for transient upload failures,
but it's no longer load-bearing for PR reports.

Changes:
  - package.json: add coverage:e2e:24 (Node 24 target variant).
  - test.yml: canonical-cell detection via a computed step output; run
    yarn coverage:e2e[:24] on canonical cells, yarn <npm_command> elsewhere,
    then upload with flags: e2e from canonical only.
  - codecov.yml: refreshed comments; both flags keep carryforward as a
    safety net rather than the primary merge mechanism.
  - Delete .github/workflows/coverage-nightly.yml — redundant.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.

Comment thread test/unit/detector.test.ts Outdated
Comment thread .github/workflows/ci.yml Outdated
robertsLando and others added 2 commits April 24, 2026 12:14
Two bugs in the prior run:

1. CODECOV_TOKEN was empty in test.yml — reusable workflows don't inherit
   secrets automatically. Codecov rejected the e2e upload with
   "Token required because branch is protected". Adding `secrets: inherit`
   to the three test_* callers in ci.yml forwards all repo/org secrets.

2. c8 produced an empty coverage table despite seeing 479 V8 dumps. Root
   cause: .c8rc.json had "lib-es5/**" in exclude, which (depending on the
   c8 version's default for `exclude-after-remap`) can drop V8 dumps for
   the executed compiled files BEFORE sourcemap-remapping them back to
   lib/*.ts source. Result: everything got filtered, 0 files reported.

   Fix: drop "lib-es5/**" from exclude (the `include: ["lib/**"]` already
   restricts output to TS source after remap, so this was redundant and
   actively harmful) and set `exclude-after-remap: true` explicitly so
   we don't depend on the default across c8 versions.

Unit coverage unaffected (still 37.42% locally) — remove+explicit-setting
is a pure no-op for the unit path because unit tests execute lib/*.ts
directly via esbuild-register; lib-es5 was never touched there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review feedback:
- detector.test.ts: visitor returns false on the root File node, not
  Program. Update wording, add a second test that descends one level
  (returns false at Program) so both branches are documented.
- ci.yml: stale comment referenced coverage-nightly.yml; e2e coverage now
  comes from the canonical Ubuntu cells in test.yml.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@robertsLando robertsLando merged commit 26f10a3 into main Apr 24, 2026
30 checks passed
@robertsLando robertsLando deleted the test/node-test-unit-suite-267 branch April 24, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: split unit tests into node:test suite and add coverage

2 participants