test(core): enforce the dist-only test target across packages

[igorlukanin]

Summary

• jest.base.config.js declares no preset and no transform, so a package extending it cannot execute TypeScript — its suites have to come from dist/, and its config has to say so. Nothing checked that, and packages kept drifting: jest's default testMatch collects the untransformable sources, a bare jest dies on import, and the path argument in the unit script is the only thing keeping the suite runnable.
• Adds scripts/check-dist-test-target.js, run as its own step in the lint job. It flags a package when all three hold: the resolved jest config cannot transform TypeScript, the package has TypeScript test sources, and it does not confine collection to dist/.
• Reads the config from either jest.config.js or a jest key in package.json, since the defect is indifferent to which one a package uses.

Why the predicate is shaped this way. Two ways to get this check wrong, both of which flag packages that are already correct — and a guard that cries wolf gets deleted rather than fixed:

• Keying on testMatch alone fails forever on cubejs-backend-native, which expresses the same constraint as roots: ['<rootDir>/dist/test/']. Both spellings are accepted.
• Keying on "extends jest.base.config.js" fails forever on cubejs-base-driver, cubejs-backend-shared and cubejs-query-orchestrator. They reach the base config through jest.base-ts.config.js, which adds preset: ts-jest and a transform — so they run from source deliberately. Asking the resolved config whether it can compile a .ts file answers that directly.

Checking that a transform merely exists isn't enough either: transform: {} is jest's idiom for switching transformation off, and {'^.+\\.js$': 'babel-jest'} covers no .ts at all. Both are truthy, and both leave the package broken. The transform patterns are tested against a representative .ts filename instead.

Likewise, a declared testMatch that points at test/ rather than dist/test/ is still the bug, so the value has to target dist/, not just exist.

This is currently red, by design

Run against master it reports 24 packages. That is the defect it exists to find, not a fault in the check — but it does mean this cannot merge until those are fixed. It should go in last, after the in-flight PRs that fix them.

Five of the 24 are ones no in-flight PR covers — they configure jest through package.json rather than a config file, so they were never in scope. Verified directly rather than inferred: npx jest in cubejs-postgres-driver reports 2 failed / 2 passed suites and in cubejs-questdb-driver 3 failed / 1 passed, every failure Jest encountered an unexpected token. They're tracked separately; this PR deliberately doesn't widen to fix them.

Test plan

• Green on a tree with the in-flight fixes applied; red on master, naming each package.
• Both spellings accepted (testMatch into dist/, roots into dist/), including roots given as a bare string.
• A declared testMatch/roots pointing at the sources is still flagged; so is an empty array, and a list mixing a dist/ entry with a source one. A !-negated entry doesn't have to name dist/.
• transform: {}, a .js-only transform, and a non-TypeScript preset all fail to exempt a package with .ts tests; a ts-jest transform or preset does exempt it.
• .tsx suites, .spec. files, and tests outside test/ (e.g. under src/) all count.
• Packages with no TypeScript tests, or no jest config at all, are left alone.
• Config carried in package.json is read, and is subject to the same constraint.
• Adding a fresh package that extends the base config with unconstrained .ts tests is caught; adding either constraint clears it.
• A config that throws on load reports the package by name instead of a bare stack trace; a run that inspects no configs at all fails rather than passing vacuously.
• No build needed — reads source configs and test sources only, never dist/. Runs in ~0.1s.
• CI must pass (expected red until the packages above are fixed)

[claude]

Claude finished @igorlukanin's task in 3m 26s —— View job

---

Review: sound guard, one real coverage gap — 1 medium, 3 low, 2 nits

Full review (click to expand)

• Gather context (diff, script, workflow, package.json)
• Understand the request
• Review the checker logic
• Verify behaviour against the actual configs in the tree
• Check existing review threads (none present)
• Post review

What I checked

Read scripts/check-dist-test-target.js in full, both base jest configs, and every packages/*/jest.config.js plus the jest keys in package.json. Direct node execution wasn't permitted in this environment, so the behavioural claims below come from reading the predicates against the real config values rather than from a run — I've said which are live defects and which are drift-proofing.

The three-part predicate is the right shape, and the reasoning in the PR body for why it isn't keyed on testMatch alone or on "extends jest.base.config.js" holds up: cubejs-backend-native does express the constraint as roots: ['<rootDir>/dist/test/'], and cubejs-base-driver / cubejs-backend-shared / cubejs-query-orchestrator do reach the base config through jest.base-ts.config.js with preset: 'ts-jest' and a real .ts transform. Testing transform patterns against a representative filename instead of checking key presence is the correct call — transform: {} and {'^.+\\.js$': 'babel-jest'} are both live in this tree (cubejs-client-vue3, cubejs-client-dx), so a presence check would have exempted them. inspected.length === 0 as a guard-the-guard is a good instinct and the kind of thing usually missing.

Findings

#	Sev	Where	Issue
1	Medium	:153-155	"No jest config" is read as "does not run jest", but cubejs-athena-driver, cubejs-crate-driver, cubejs-jdbc-driver, cubejs-mongobi-driver and cubejs-trino-driver all invoke jest from scripts with no config and have TS test sources — the exact defect, silently unenforced
2	Low	:168-172	.some across testMatch/roots accepts roots → dist/ with testMatch → sources, which collects zero tests and reports green
3	Low	:118-120	A config exporting a function (jest supports this) is treated as a plain object → false positive on an otherwise-correct package
4	Low	:210-215	One unloadable config aborts the whole scan, masking every offender found so far

Finding 1 is the one worth acting on before merge — not because it breaks the check, but because it makes the check's clean exit mean less than it appears to. Everything else is drift-proofing; no package in the tree trips 2 or 3 today.

Nits

• No automated test for the checker itself. All six commits are prefixed test(core): and the PR carries a 10-item test plan, but the diff adds no test file — every one of those scenarios was verified by hand and none of them regress-protect the script. Given that this file is itself the assertion, a small suite (fixture package dirs → expected offender list) would be cheap and would let the next person change targetsDist without re-deriving the whole matrix. The .some-vs-every question in finding 2 is exactly the kind of thing such a suite would pin down.
• CONFIG_EXTENSIONS ordering (:102) is js, cjs, mjs, ts, json; jest resolves .mjs ahead of .cjs. A package carrying both would have this script inspect a config jest ignores. Purely hypothetical today, but the list is presented as mirroring jest's resolution, so matching the order costs nothing.
• Scope is packages/ only — rust/package.json (which lint:npm does cover) and the repo root are outside the walk. Probably intentional; a one-line comment saying so would stop a future reader wondering.

Non-blocking observations

• Wiring: lint:dist-test-target is added to root lint and as its own CI step, but since CI runs lint:npm and lerna run lint separately rather than root yarn lint, there's no double execution. Placing it before lerna run lint means it fails fast, which is right for a ~0.1s check.
• .github/workflows/push.yml adds jest.base-ts.config.js and the script itself to the paths filters — correct, since a change to either can flip this check's outcome. (I can't modify files under .github/workflows myself; noting only that the change looks right.)
• targetsDist normalising \ → / and exempting !-negated entries are both correct readings of jest semantics, and the (^|\/)dist(\/|$) anchoring correctly declines to match a dist-helpers/ style directory.
• The "currently red by design, merge last" framing is the right call — a guard landed before its subjects are fixed trains people to ignore a red lint job.

· branch [`igor/core-741-enforce-dist-only-test-target`](https://github.com/cube-js/cube/tree/igor/core-741-enforce-dist-only-test-target)

[claude]

Claude finished @igorlukanin's task in 4m 41s —— View job

---

Re-review: all four prior findings addressed, plus a real test suite — 1 low, 3 nits

Full review (click to expand)

• Read the new commits (14fe2a0, 7456a6d, 8f7bfe7, a61073f, 8512261)
• Re-read scripts/check-dist-test-target.js in full
• Read the new scripts/check-dist-test-target.test.js (40 cases)
• Verify predicates against the real configs in the tree
• Check prior threads; resolved all four
• Post review

What changed since the last pass

Every finding from the previous review is closed, and two of the three nits with it:

prior finding	resolution
"no jest config" ≠ "does not run jest"	invokesJest reads the scripts strings; a config-less package that runs jest goes through the same predicate with config = {} rather than a parallel copy. Resolved.
unloadable config aborts the whole scan	collected into uninspectable and reported alongside the offenders, non-zero either way. Resolved.
config exporting a function misread as empty	refuses to guess and reports by name (:192-197). Resolved.
some across testMatch/roots	Declined, with a reason I accept — see below. Resolved.
nit: scope was packages/ only	widened to workspaceDirs() off the root manifest, which picks up rust/*. Genuinely load-bearing: rust/cubestore has js-wrapper/test/process.test.ts and a package.json jest block, so it was previously unenforced.
nit: no automated test for the checker	40 dependency-free fixture cases, run in the same lint step.

Three things in the new code are better than what I'd have suggested:

• invokesJest reads scripts, not devDependencies, with the empirical reason written down (:216-219): thirteen packages invoke jest off the hoisted root copy, two declare it without running it. That's the kind of justification that keeps someone from "simplifying" it back to the wrong signal.
• transformsTypeScript probes the extensions the package actually has — a ^.+\.ts$ transform beside a .tsx suite no longer exempts anything, and the test at :210-220 pins that an exemption must mean every suite here can run.
• The test runner rejects a case that returns a promise (:421-423) rather than silently printing it as passing. Small, and exactly the failure mode a hand-rolled sync runner usually has.

On the some-vs-every question I raised last time: the counter-argument in :286-292 is correct and I withdraw the suggestion. Jest ANDs the constraint keys, so roots: ['<rootDir>/dist/test/'] beside a broad testMatch: ['**/*.test.js'] is a correct package that every would report — the false positive that gets a guard deleted. The residual case (roots → dist/, testMatch → sources → zero tests collected, green) survives, but the only predicate that catches it without the false positive is "some key targets dist/ and no key targets exclusively outside it," which is more machinery than the case deserves. Leaving it is the right call; the test at :150-161 documents the choice.

Verified, not inferred

Node execution isn't permitted in this environment, so — same caveat as last time — the behavioural claims below come from reading the predicates against real values, plus these git-level checks:

• git ls-files 'packages/*/jest.config.*' 'rust/*/jest.config.*' → 29 files, all .js. No config in the tree is uninspectable today, so the new uninspectable.length > 0 → exit 1 path won't fire spuriously.
• No packages/*/jest.config.js exports a function (grepped for module.exports = ( / = async / = function).
• Only cubejs-backend-native invokes jest with a custom --config (jest-bridge.config.js, jest-bench.config.js), and it also carries a default jest.config.js with roots: ['<rootDir>/dist/test/'] — so the "custom config only" false positive described in the nits is hypothetical.
• Root workspaces is the object spelling { packages: ['rust/*', 'packages/*'] }, which workspaceDirs handles and the suite asserts against the real manifest (:410-412).
• rust/cubestore's package.json jest block declares roots: ['<rootDir>/dist'] and a dist/ testMatch, so widening the walk to rust/* adds an exempt package, not a 25th offender.

Finding

Low — CONFIG_EXTENSIONS order doesn't match the constant it cites (:158). Jest's JEST_CONFIG_EXT_ORDER is .js, .mjs, .cjs, .ts, .mts, .cts, .json; this list is js, ts, mjs, cjs, json. A package with jest.config.ts beside jest.config.cjs gets the .ts reported as uninspectable (lint fails) while jest would load the readable .cjs; a jest.config.mts/.cts matches nothing, falls through to the package.json branch, and — if the package invokes jest — is reported as a dist-target offender, naming the wrong defect. Both are false failures, so this is hardening rather than a live bug.

Nits

• main() is the one part with no coverage. The suite exercises violations() and the unit predicates thoroughly, but the exit codes, the offender message, the vacuous-pass guard (inspected.length === 0), and the deliberate ordering at :308-314 (uninspectable errors printed before the vacuous-pass exit, so they aren't swallowed) are all untested — and that ordering is precisely the subtle thing a test should pin, since nothing about the code makes it look load-bearing. Extracting a report({offenders, inspected, uninspectable}) that returns an exit code would make all four assertable at the same near-zero cost as the rest of the suite.
• Offender output is in readdirSync order, which isn't sorted on most filesystems. With ~24 names in CI output, a .sort() before printing makes two runs diffable.
• scripts/check-dist-test-target.test.js is named *.test.js but is run by node, not jest. Nothing globs scripts/ today, so it's harmless — but a file matching jest's default testMatch that calls process.exit at module scope and contains no test() jest recognises is a small trap for whoever eventually points a root-level jest at the repo. Also worth knowing: root yarn lint is lint:npm + per-package lerna run lint, so scripts/ isn't linted by anything — the eslint-disable-next-line comments in both files are aspirational.

Non-blocking observations

• The two CI steps are ordered test-then-check, which is right: if the checker's own suite is broken, its verdict on the tree means nothing.
• .github/workflows/push.yml now globs scripts/check-dist-test-target*.js, so the test file re-triggers the job too. Correct. (I can't modify files under .github/workflows; noting only that the change reads right.)
• IGNORED_DIRS including target matters more now that the walk covers rust/* — rust/cubestore/target and rust/cube/target would otherwise dominate the runtime.
• label() producing <workspace>/<package> keeps fixture-tree output from reading as paths outside the repo, which is what lets the suite assert on offender names at all.
• "Currently red by design, merge last" still stands, and the added coverage strengthens it: the checker's own suite is green independently of the 24 packages it reports, so test:dist-test-target passing is a meaningful signal even while lint:dist-test-target is red.

Resolved all four of my prior threads: three fixed in code, one answered with a rationale I accept.

· branch igor/core-741-enforce-dist-only-test-target ·

[igorlukanin]

Took all four findings and the three nits, in 14fe2a00d1. The one I didn't take as written is #2 — every is the wrong rule and would false-positive on a correct config; details in that thread.

The nits, since they turned out to be the more interesting half:

• No test for the checker. Added scripts/check-dist-test-target.test.js — 37 fixture-driven cases, dependency-free, wired into lint ahead of the check itself so a broken checker reports as a broken checker rather than as tree drift. Every fix above is red-checked: reverting any one of them fails at least one case.
• CONFIG_EXTENSIONS ordering. Matched to jest's actual JEST_CONFIG_EXT_ORDER (js, ts, mjs, cjs, json) — .ts precedes .mjs too, not just .mjs before .cjs. Pinned by a fixture carrying both.
• Scope is packages/ only. Widened rather than commented: rust/cubestore runs jest too. The roots are now derived from the root manifest's workspaces instead of hardcoded, because losing a tree is the one failure the inspected guard can't catch — the walk still resolves plenty of configs from the trees it does visit.

Two more that came out of reviewing the fix:

• A .ts-only transform beside a .tsx suite was exempting a package whose tests can't actually run. The transform is now probed against the extensions the package really has.
• target/ wasn't in IGNORED_DIRS, so the walk was recursing 7.5 GB of Cargo output under cubejs-backend-native. 0.27s → 0.08s, which matters because the ~0.1s runtime is why this can sit in the lint job with no build step.

Offender count is 25, up one: cubejs-trino-driver, newly caught by the no-config fix. CORE-742 covers it.

[claude]

Claude finished @igorlukanin's task in 2m 3s —— View job

---

Re-review of b9427c5: .mts/.cts gap closed, one new edge from the fix — 1 low, 2 standing nits

Full review (click to expand)

• Read the new commit (b9427c5)
• Re-read scripts/check-dist-test-target.js and its suite in full
• Verify the resolver against every jest.config.* in the tree
• Check prior threads (4 resolved, 1 open and still partly applicable)
• Post review

What b9427c5 changes

resolveConfig no longer resolves only from a fixed extension list. After the known list misses, it globs jest.config.* off readdirSync and reports anything unloadable by name. That closes the worse half of my prior finding: a jest.config.mts no longer falls through to the package.json branch and get reported as a dist-target offender — it's named as uninspectable, which is the honest failure. The new case at :320-334 pins exactly that, including the filename in the assertion.

LOADABLE_EXTENSIONS + path.extname in place of the two endsWith calls is also a straight readability win, and it's the piece that makes the catch-all safe to add at all.

Finding

Low — the catch-all matches by prefix, so it claims --config-only variants too (:170-173). jest.config.unit.js / jest.config.integration.js / jest.config.base.js all start with jest.config. and all have extname === '.js', so they pass the loadable guard and get required as the resolved config — in readdirSync order, which isn't sorted, so two such files make the verdict machine-dependent. Jest loads neither by default; those are --config targets, and the package's real default collection is jest's unconstrained one. /^jest\.config\.[^.]+$/ keeps this commit's .mts win without reading a config that isn't in play. Nothing in the tree trips it — all 29 configs under packages/+rust/ are plain jest.config.js, and cubejs-backend-native's extras are jest-bridge.config.js / jest-bench.config.js, outside the prefix — so hardening, not a live bug.

Standing thread

The CONFIG_EXTENSIONS ordering thread is half-addressed and left open. The .mts/.cts misattribution is gone, but the list is still js, ts, mjs, cjs, json while jest's JEST_CONFIG_EXT_ORDER is .js, .mjs, .cjs, .ts, .mts, .cts, .json — .ts after both ESM/CJS spellings, not before. So the comment at :152-153 ("Resolution follows jest's own JEST_CONFIG_EXT_ORDER") overstates what the list does, and a package carrying jest.config.cjs beside jest.config.ts still resolves the .ts here → reported uninspectable → lint red, while jest reads the .cjs this check could have read. Also a false failure, so still hardening. Not re-posting on the thread; the unresolved state carries it.

Nits, both carried over from the last pass

• main() remains the untested part. The exit codes, the offender message, inspected.length === 0, and — most of all — the deliberate ordering at :317-334 (uninspectable errors printed before the vacuous-pass exit, so they aren't swallowed) have no coverage. That ordering is the one thing in the file whose load-bearingness isn't visible from the code, and it's the thing a test should pin. Extracting report({offenders, inspected, uninspectable}) → exitCode makes all four assertable at the same near-zero cost as the other 40 cases.
• Offender output is unsorted (readdirSync order). With ~25 names in CI, .sort() before printing makes two runs diffable.

Verified, not inferred

node_modules isn't installed and direct node execution wasn't permitted here, so I could not run the suite or confirm jest's constants against the installed copy — the JEST_CONFIG_EXT_ORDER value above is stated from jest 29/30's jest-config source, and worth a glance locally before acting on it. Everything else is a file-level check:

• Glob **/jest.config.* → 29 files, all .js, all directly under a packages/* dir. No config in the tree is uninspectable, so the new uninspectable.length > 0 → exit 1 path won't fire spuriously, and the prefix-match edge above stays hypothetical.
• No jest.config.<name>.js variant exists anywhere; cubejs-backend-native's --config targets are named jest-*.config.js.
• path.extname('jest.config.unit.js') === '.js' — which is what makes the finding above reachable rather than theoretical.

Non-blocking observations

• The suite's writePackage gained alsoConfig/alsoConfigExt and the resolution-order case at :296-309 — pinning precedence with a fixture rather than a comment is the right instinct, and it's what would catch the ordering divergence above if the pair were cjs+ts instead of js+ts.
• Reasoning-in-comments density stays high and stays load-bearing (some vs every at :296-302, invokesJest reading scripts at :226-229, target in IGNORED_DIRS at :119-121). These are the comments that stop a future "simplification" back to the wrong signal.
• "Currently red by design, merge last" still holds. The checker's own suite is green independently of the 25 packages it reports, so test:dist-test-target remains a meaningful signal while lint:dist-test-target is red.
• I can't modify .github/workflows/push.yml; the paths additions read correct.

· branch [`igor/core-741-enforce-dist-only-test-target`](https://github.com/cube-js/cube/tree/igor/core-741-enforce-dist-only-test-target)

[claude]

Claude finished @igorlukanin's task in 1m 32s —— View job

---

Re-review of f86b478: prior finding closed cleanly — 0 new findings, 1 judgment call, 2 standing nits

Full review (click to expand)

• Read the new commit (f86b478)
• Re-read scripts/check-dist-test-target.js and its 41-case suite in full
• Verify the resolver against every jest.config.* in the tree
• Check prior threads (all six resolved; nothing to re-open)
• Post review

What f86b478 changes

The catch-all is now /^jest\.config\.[^.]+$/ instead of a startsWith prefix, which is exactly the narrowing I suggested. jest.config.unit.js / jest.config.integration.js no longer get required as the resolved config, so the machine-dependent readdirSync-order verdict is gone while the .mts/.cts win from b9427c5 is kept. The comment at :162-165 writes down why single-extension is the load-bearing part, which is the thing that stops it being "simplified" back. The new case at :320-334 pins it with a fixture that would pass under the old prefix match and fails under it now.

Six threads, all resolved, none re-opened. Nothing in the current file trips a finding I'd hold the merge for.

One judgment call worth naming (not a finding)

The new case asserts that a package whose only jest invocation is jest --config jest.config.unit.js is reported as an offender — even when that variant config is dist-constrained. That's a defensible reading (invokesJest at :236-245 matches the --config form, and a bare jest or an IDE runner in that package really would collect the sources), but it's the one place the check now names a defect the package's own scripts can't reach. The remedy the message prints — add a default jest.config.js with a dist/ testMatch — does fix the real hazard, so the output isn't misleading, just broader than the scripts warrant. Nothing in the tree is in that shape (cubejs-backend-native's --config targets are jest-bridge.config.js / jest-bench.config.js, outside the prefix, and it carries a default config anyway). If it ever bites, invokesJest skipping invocations that carry --config/-c is the narrow fix. Fine as-is; flagging so the choice is on the record rather than discovered later.

Standing nits, both carried from earlier passes

• main() is still the only untested part. The exit codes, the offender message, inspected.length === 0, and — most of all — the deliberate ordering at :325-339 (uninspectable errors printed before the vacuous-pass exit, so they aren't swallowed) have no coverage. That ordering is the one thing in the file whose load-bearingness isn't visible from the code. Extracting report({offenders, inspected, uninspectable}) → exitCode makes all four assertable at the same near-zero cost as the other 41 cases.
• Offender output is unsorted (readdirSync order, :262-266). With ~25 names in CI, a .sort() before printing makes two runs diffable.

Two sub-trivial things I noticed and am not asking for: with two unknown-extension configs (jest.config.mts beside jest.config.cts) the uninspectable message names whichever readdirSync yields first — exit code is 1 either way; and a broken symlink named jest.config.x would surface as a bare ENOENT in uninspectable without the package label, since statSync throws before label() is applied.

Verified, not inferred

node_modules isn't installed and direct node execution wasn't permitted in this environment, so I could not run the suite — the behavioural claims come from reading the predicates against real values plus these file-level checks:

• Glob **/jest.config.* → 29 files under packages/+rust/, all plain jest.config.js. No jest.config.<name>.<ext> variant exists anywhere, so this commit changes the verdict on zero packages in the tree — it's hardening, which is what it claims to be.
• path.extname('jest.config.unit.js') === '.js' is what made the old prefix match reachable; /^jest\.config\.[^.]+$/ rejects it on the segment count instead, which is the right axis.
• No config in the tree is uninspectable or exports a function, so neither the uninspectable.length > 0 → exit 1 path nor the function-export throw fires spuriously.
• jest-config@29.7.0 in this tree has JEST_CONFIG_EXT_ORDER = ['.js', '.ts', '.mjs', '.cjs', '.json'], matching CONFIG_EXTENSIONS at :167 — my earlier ordering claim was from jest's newer source, and the resolved thread has it right.

Non-blocking observations

• The suite is now 41 cases and every fix in this PR is red-checked by at least one — including this commit's, which fails if the regex reverts to startsWith. That's the property that makes "the checker is itself the assertion" survivable.
• Comment density stays high and stays load-bearing: some vs every at :301-307, invokesJest reading scripts at :230-234, target in IGNORED_DIRS at :119-121, and now single-extension resolution at :162-165. Each one records a reason someone would otherwise have to re-derive.
• "Currently red by design, merge last" still holds, and test:dist-test-target remains green independently of the 25 packages lint:dist-test-target reports.
• I can't modify .github/workflows/push.yml; the paths additions read correct.

· branch [`igor/core-741-enforce-dist-only-test-target`](https://github.com/cube-js/cube/tree/igor/core-741-enforce-dist-only-test-target)