fix(api-gateway): test the compiled output, and enforce it

[igorlukanin]

Summary

• packages/cubejs-api-gateway's unit script targets dist/test, but its jest config never said so — it extends jest.base.config, which sets no testMatch, so jest's default was in effect and collected the TypeScript/ESM sources under test/. Nothing transforms them, so each one dies with SyntaxError: Cannot use import statement outside a module; the script's path argument was the only thing keeping the run green. Two of those sources were plain .js (test/date-parser.test.js, test/normalize-query-filters-dates.test.js), which made them look editable when in fact only their compiled copies in dist/test ever run.
• Worth being precise about the mechanism, because it is wider than the two .js files suggest: the .ts tests fail from source in exactly the same way. No test in this package runs from source. So the fix is not "convert two files" — it is to make the config express the dist target it always had, and enforce it. testMatch is now pinned to <rootDir>/dist/test/**/*.{test,spec}.js, and the two .js files are converted to .ts so the directory is uniform. .spec. is in the pattern because jest's default testMatch accepted it: pinning without it would silently drop any future spec file, which is the same defect in a new costume.
• test/test-collection.test.ts guards it: every test file on disk must have a compiled counterpart that testMatch actually collects, and no source may be collected directly. It evaluates the real testMatch through micromatch — the glob library jest itself uses — rather than restating the pattern, so it stays honest if someone edits it. The walk covers the whole package, not just test/, because a test added under src/ compiles to dist/src/ and would be silently uncollected there too. It also asserts that no narrowing key (testPathIgnorePatterns, modulePathIgnorePatterns, roots, testRegex) has been added behind testMatch — the routine "temporarily ignore the flaky suite" edit would otherwise reintroduce exactly this bug past a guard that reasons from testMatch alone. And it closes the reverse direction: a compiled test whose source is gone. tsc is incremental and only build does rm -rf dist, so renaming or deleting a test leaves its old dist copy collected forever, running code that no longer exists — this file's own subject, mirrored. Matching is done on package-relative posix paths so the guard doesn't silently match nothing on Windows (micromatch reads \ as an escape), which would have failed asymmetrically: one assertion passing vacuously while the other named all 14 files.
• Side effect of the conversion, called out because it changes behaviour: date-parser.test.js did Date.now = jest.fn()... then Date.now.mockRestore(), and mockRestore() on a bare jest.fn() restores nothing — so Date.now stayed mocked for the rest of the file, returning a Date where Date.now() must return a number. Both are now jest.spyOn(Date, 'now'), restored from a single afterEach rather than inline after each expect. Inline was still a leak on the failure path only: measured, one injected failure took 3 tests red (two of them unrelated ones reading the real clock); from afterEach the same injection takes exactly 1.
• micromatch moves into devDependencies; it was already in the tree as a transitive dep of jest, and ^4.0.8 resolves to an existing yarn.lock entry, so --frozen-lockfile needs no lockfile change.

Testing dist/ is deliberate rather than incidental — 14 packages do it, test/snapshotResolver.js is built around it, and package.json ships main: dist/src/index.js, so the compiled copies are the artifact users get. Migrating this package to ts-jest so its tests run from source is a real alternative and a much larger change; it is not attempted here.

One consequence to know about: npx jest date-parser works, but naming the source path with its extension (npx jest test/date-parser.test.ts) reports "No tests found", because the path that exists at run time is the compiled .js. The jest config documents this.

Test plan

• yarn tsc && yarn unit in the package: 14 suites / 240 tests / 23 snapshots green — the 13/235 pre-change baseline plus the guard's 5 tests. Also green with no path argument, which is the point: the config now carries the target instead of the script's positional compensating for it. The positional stays in unit as fail-safe redundancy and for uniformity with the other 13 dist-testing packages — it is a superset of what testMatch collects, so it filters nothing today. It is a second, config-invisible route to narrowing collection, and the guard says so rather than pretending testMatch is the only one.
• Guard mutation-tested rather than trusted: removing testMatch (which restores the original bug), deleting a compiled counterpart, adding a test under src/, adding a .test.mts, adding a testPathIgnorePatterns, and leaving a compiled test with no source each turn it red, naming the offending file or key. Widening the pattern to dist/**/*.{test,spec}.js keeps it green, confirming it follows the config instead of a hardcoded copy.
• The converted date-parser tests verified to still bite, not merely pass: mutating src/date-parser.js (end.add(-1, match[3]) → -2, the last N <unit> branch) turns 5 of them red, including the Date.now-mocking ones, and green again on restore. Mutating the singular last <unit> branch instead turns nothing red — that line is genuinely uncovered, which is a pre-existing coverage gap rather than evidence the tests are inert.
• eslint 0 errors / 21 warnings, identical to master. The two converted files enter lint scope for the first time (package lint is eslint "**/*.{ts,tsx}") and add no warnings of their own.
• yarn install --frozen-lockfile succeeds with no yarn.lock change.
• CI must pass.

[claude]

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

---

Review complete — approach is sound; 0 blocking, 1 medium, 4 low (5 inline comments)

Full review

• Gather context (diff, jest config, base branch state)
• Review jest.config.js + package.json changes
• Review converted test files for behavioural drift
• Review the new test-collection.test.ts guard
• Check for stale prior review threads (none exist)
• Post review

Verdict

The diagnosis is right and the fix is the right shape. jest.base.config.js sets no testMatch, so jest's default (**/?(*.)+(spec|test).[jt]s?(x)) was in effect and the dist/test positional in the unit script was doing all the work — pinning testMatch: ['<rootDir>/dist/test/**/*.test.js'] moves the target into the config where it belongs. I confirmed the pattern loses nothing: there are no .spec.* files and no __tests__ directories in the package, so the narrower glob collects the same 14 suites the default would have.

Claims I was able to verify statically:

Claim	Result
--frozen-lockfile needs no lockfile change	✅ micromatch@^4.0.8 is already a key at yarn.lock:17817 (alongside ^4.0.2, ^4.0.4, ^4.0.5)
Test semantics unchanged by the conversion	✅ diffed both files against origin/master — normalize-query-filters-dates is types-only; date-parser is types + the Date.now change
The Date.now bug was real	✅ master did Date.now = jest.fn().mockReturnValue(now) (a Date, not a number) at 10 sites and mockRestore() on a bare jest.fn() at each — so the mock leaked cumulatively from line 77 onward
Guard won't spuriously fail in CI	✅ .github/workflows/push.yml:135 runs yarn tsc before yarn lerna run unit
Converted files newly enter lint scope	✅ package lint is eslint "**/*.{ts,tsx}"

I could not run the suite — node_modules is not installed in this environment and a full monorepo yarn install is out of budget for a review pass. The 238-tests-green figure is unverified here; CI is the check.

Praise

• The guard is better-designed than most meta-tests. Deriving from the real jestConfig.testMatch through micromatch — jest's own glob library — rather than restating the pattern is the detail that makes it survive edits, and expect(SOURCE_TEST_FILES.length).toBeGreaterThan(5) guarding against a vacuous walk is the kind of thing that's usually missing. Extending the walk to the whole package so a test under src/ is caught too is a good call.
• The mutation testing in the PR description (removing testMatch, deleting a counterpart, adding a .test.mts, and separately confirming the date-parser tests still bite) is the right standard of evidence for a change like this.
• Calling out the Date.now behaviour change explicitly rather than burying it in a rename diff.

Findings

Medium

1. date-parser.test.ts:83 and 9 other sites — restore still leaks on a failing assertion. jest.restoreAllMocks() is the last statement inside each test body, after the expect. A throwing assertion skips it and Date.now stays mocked for the rest of the file. That's the same leak class the PR fixes, narrowed to the failure path — and it bites here because Date.now-mocking tests are interleaved with tests that need the real clock (today, last 6 hours, from 23 hours ago to now), so one real failure cascades into unrelated red. The sibling file already uses afterEach correctly. (inline)

Low

2. test-collection.test.ts:49-52 — fails for every file on Windows. micromatch treats \ as an escape, not a separator, so isMatch returns false unconditionally on win32: test 2 passes vacuously, test 3 fails listing all 14 files as uncollected — while jest itself ran them fine. Same root cause makes glob metacharacters in the checkout path (/Users/me/cube (fork)/) break it too. (inline, with suggestion)

3. test-collection.test.ts:76-89 — only checks source → compiled, not the reverse. dist is only cleaned by build, not by the tsc developers actually run, so a renamed or deleted test leaves an orphaned dist/test/*.test.js that keeps getting collected forever, running code whose source is gone. That's this file's own failure mode mirrored. Cheap to close with the existing helpers. (inline, with suggestion)

4. test-collection.test.ts:35 — /\.test\.[^.]+$/ skips graphql.test.ts.snap only incidentally (snapshots always carry two extensions). Adding __snapshots__ to IGNORED_DIRS states the intent. (inline)

5. date-parser.test.ts:63 — '2020-09-22T13:03:20.518Z' as unknown as Date plus three lines of comment justifying it, where new Date('2020-09-22T13:03:20.518Z') is behaviour-identical (moment() resolves both to the same instant for a Z-suffixed ISO string). (inline, with suggestion)

One note not attachable inline

package.json:18 is outside the diff, so this couldn't be an inline comment: unit is still jest --coverage --forceExit dist/test. With testMatch now set, that positional is a redundant testPathPattern regex. It's harmless — the guard still runs from dist/test and would catch a broadened testMatch even with the positional filtering sources out — but the PR body says the suite is "now green with no path argument, so the config carries the target rather than the script compensating for it", and the script wasn't actually changed to match. Dropping dist/test from the script would make the code say what the description says.

Not concerns

• Security, performance, auth: nothing in scope — this is test-infrastructure only, no runtime code paths touched.
• micromatch as a direct devDependency: fine, and not published (files is ["README.md", "dist/src/*"]).
• The require() + eslint-disable for micromatch: justified, there's no @types/micromatch in the tree.
• Docs: no user-facing surface, so no /docs-mintlify change needed. The jest.config.js comment documenting the jest date-parser vs. jest test/date-parser.test.ts gotcha is the right place for it.
• Not migrating to ts-jest: agreed this is out of scope, and the PR is explicit about it.

· Branch [`igor/core-735-api-gateway-source-js-tests-never-execute`](https://github.com/cube-js/cube/tree/igor/core-735-api-gateway-source-js-tests-never-execute)

[claude]

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

---

Re-review after push — 0 blocking, 1 medium, 5 low; 5 prior threads re-affirmed, 1 new comment

Full review

• Diff current HEAD against the previously reviewed state
• List existing review threads, check each against current content
• Re-review changed files
• Post updated review

What changed since the last review

The push tightened the guard in two ways, both good:

• testMatch is now <rootDir>/dist/test/**/*.{test,spec}.js rather than *.test.js, and the walk's filter is /\.(test|spec)\.[^.]+$/ — so pinning the pattern provably can't drop a .spec. suite that jest's default would have collected. There are no .spec.* files in the package today, so this is forward-insurance, not a fix; still the right call, since the whole premise of the PR is that a silently-uncollected test is invisible.
• A new test, nothing narrows collection behind testMatch (lines 72-79), asserts testPathIgnorePatterns / modulePathIgnorePatterns / roots are unset. Verified against jest.base.config.js: it sets none of the three, so this passes rather than tripping on an inherited value. Good instinct — the other three tests reason from testMatch alone, and that reasoning is only sound if nothing downstream filters.

Also re-verified in the current tree:

Check	Result
PACKAGE_ROOT resolution	✅ __dirname is <pkg>/dist/test at run time, so ../.. is the package root
compiledCounterpart mapping	✅ matches tsconfig.json (rootDir: ".", outDir: "dist"), so test/x.test.ts → dist/test/x.test.js
Brace pattern under micromatch	✅ micromatch supports {test,spec}, so deriving from the config still works with the widened glob
micromatch devDep, frozen lockfile	✅ ^4.0.8 is an existing yarn.lock key

I could not run the suite — node_modules isn't installed here and a monorepo yarn install is out of budget for a review pass. CI is the check on the 238-green figure.

Findings

Every finding from the previous review survives in the current content — I checked each against the file rather than assuming, and left the threads unresolved rather than replying on them:

Medium

1. date-parser.test.ts:83 — restore still leaks on a failing assertion; jest.restoreAllMocks() is the last statement inside each test body, after the expect. Still present at all 10 sites. (thread)

Low

2. test-collection.test.ts:54 — isCollected fails for every file on Windows (micromatch treats \ as an escape); test 2 passes vacuously, test 3 fails listing all 14 files. Unchanged. (thread)
3. test-collection.test.ts:100 — only source → compiled is checked; an orphaned dist/test/*.test.js from a renamed test keeps being collected forever, since tsc doesn't clean dist. Unchanged. (thread)
4. test-collection.test.ts:37 — the walk excludes __snapshots__/*.test.ts.snap only incidentally (two extensions defeat [^.]+$). Widening to (test|spec) didn't change this; adding __snapshots__ to IGNORED_DIRS still states the intent directly. (thread)
5. date-parser.test.ts:63 — '…Z' as unknown as Date plus three lines of comment where new Date('…Z') is behaviour-identical. Unchanged. (thread)

New, on the code added in this push

6. test-collection.test.ts:72-79 — the new narrowing check enumerates config keys, but the narrowing actually in effect is the dist/test positional still in package.json:18's unit script, which require('jest.config.js') can't see. Benign today (it's a superset of testMatch), but jest … dist/test/graphql would leave this suite green with 13 of 14 suites unrun — the same edit the test exists to catch, made one file over. Dropping the positional also reconciles the code with the PR body, which already says there's no path argument. (new comment, with the toBeUndefined() message nit and testRegex)

Praise

• Widening to {test,spec} in both the config and the walk, and saying in the comment why, is the detail that makes the pin provably lossless rather than probably lossless. Getting that in on a second pass is easy to skip.
• The new nothing narrows test is the correct thing to have noticed: the other assertions are only as strong as the claim that testMatch is the sole gate. Finding 6 is about completing it, not about it being wrong to add.
• Still the case that deriving from the real jestConfig.testMatch through micromatch — jest's own glob library — rather than restating the pattern is what makes this guard survive edits, and that toBeGreaterThan(5) keeps the walk from going vacuous.

Not concerns

• Security, performance, auth: nothing in scope — test infrastructure only, no runtime code paths touched.
• micromatch as a direct devDependency: fine, and not published (files is ["README.md", "dist/src/*"]).
• require() + eslint-disable for micromatch: justified, no @types/micromatch in the tree.
• Docs: no user-facing surface. The jest.config.js comment documenting the jest date-parser vs. jest test/date-parser.test.ts gotcha is the right place for it.
• Not migrating to ts-jest: agreed, out of scope, and the PR is explicit.

No threads resolved — all five prior concerns are still applicable in the current diff.

· Branch [`igor/core-735-api-gateway-source-js-tests-never-execute`](https://github.com/cube-js/cube/tree/igor/core-735-api-gateway-source-js-tests-never-execute)

[claude]

This test enumerates config keys, but the narrowing actually in effect today lives in the unit script, where the guard can't see it. package.json:18 is still:

"unit": "CUBE_JS_NATIVE_API_GATEWAY_INTERNAL=true jest --coverage --forceExit dist/test"

That trailing dist/test is a positional testPathPattern regex — functionally the same class of narrowing as testPathIgnorePatterns, just applied from the other side and invisible to require('jest.config.js'). So the exact edit this test is written to catch ("temporarily ignore the flaky suite") still lands unguarded if someone makes it in the script instead of the config: jest … dist/test/graphql leaves this suite green while 13 of the 14 don't run.

It's benign as written — the pattern is a superset of what testMatch collects, so today it filters nothing. Two things worth doing:

1. Drop the positional. It's now redundant with testMatch, and the PR body already claims it's gone ("now green with no path argument, so the config carries the target rather than the script compensating for it") — the script wasn't actually changed to match, so code and description disagree.
2. Either way, note the limitation here. The comment at lines 73-75 reads as though testMatch is the only thing that decides collection, and a reader will believe it; a line saying "CLI arguments can still narrow this, and are out of scope" keeps the guard honest about its own reach.

Separately, minor: toBeUndefined() on three keys gives a failure message ("expected undefined, received [...]") that doesn't say why the key is forbidden. A .toBeUndefined() with the reason in the assertion — or expect({ testPathIgnorePatterns, roots }).toEqual({}) with a comment — costs nothing and saves the next person a git log. And testRegex belongs in the list: it's mutually exclusive with testMatch, so setting it makes jest throw rather than silently narrow, but that's a different failure to reason about, not a covered one.

[igorlukanin]

You're right that the PR body and the code disagreed, and the body was the wrong one. Corrected there.

Kept the positional, though, and took your point 2. It's deliberate: it's a superset of what testMatch collects so it filters nothing, and all 13 sibling dist-testing packages carry the same argument — dropping it here alone makes this package the odd one out right as CORE-736 goes to make them uniform. The redundancy is the point: if someone deletes testMatch, the script still narrows to dist/test instead of collecting untransformable sources.

But your actual finding stands regardless of which way that goes: a CLI positional is a second route to narrowing that the guard cannot see, and the comment implied testMatch was the only one. The guard now says so explicitly, and points at the unit script.

Also folded in the rest: testRegex is in the list (with a note that it makes jest throw rather than narrow — verified, jest refuses to start with both set, so it's named for the reader rather than caught), and the three toBeUndefined() calls are now one filter producing the offending key names, so the failure says which key and not just expected undefined.

[igorlukanin]

Took the reword, which is the arm of your "either drop it or reword" that matches the scope decision. The body bullet now states that the positional stays, why, and that it is a config-invisible narrowing route — so the six-months-later reader auditing the claim finds the reasoning instead of an unfinished fix.

Thanks for checking .test.d.ts / .test.js.map against the dist walk — that over-collection hazard is real for the new assertion and I hadn't verified it from that direction.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.10%. Comparing base (f7ff67a) to head (8c558aa).

❗ There is a different number of reports uploaded between BASE (f7ff67a) and HEAD (8c558aa). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (f7ff67a)	HEAD (8c558aa)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11482       +/-   ##
===========================================
- Coverage   79.46%   59.10%   -20.36%     
===========================================
  Files         480      223      -257     
  Lines       98810    17891    -80919     
  Branches     3636     3636               
===========================================
- Hits        78515    10574    -67941     
+ Misses      19774     6796    -12978     
  Partials      521      521               

Flag	Coverage Δ
cube-backend	59.10% <ø> (-0.01%)	⬇️
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

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

---

Re-review after push — 0 blocking, 0 medium, 1 low; 5 of 6 prior findings fixed, 5 threads resolved

Full review

• Diff current HEAD against the previously reviewed state
• List existing review threads, check each against current content
• Re-review changed files
• Resolve stale threads / post updated review

What changed in f08972f

Every finding from the last pass is addressed, and each fix is the one I'd have picked:

Prior finding	Status in current tree
Medium — Date.now restore leaks on a failing assertion	✅ Fixed. A single afterEach(() => jest.restoreAllMocks()) at the top of the describe (date-parser.test.ts:8-10), all 10 inline calls dropped. Verified all 10 mocking sites still use jest.spyOn(Date, 'now'), so restore actually restores, and the comment states why the placement matters.
Low — isCollected fails for every file on win32	✅ Fixed, and fixed the better way of the two I suggested: matching happens package-relative (toPosix(relativeToPackage(file)) vs. p.replace('<rootDir>/', '')), which immunizes against the separator issue and the glob-metacharacter-in-checkout-path issue at once, rather than only the first.
Low — only source → compiled checked; orphaned dist tests	✅ Fixed. no collected test has lost its source (lines 124-136) closes the reverse direction. Confirmed the dist walk can't over-collect: .test.d.ts and .test.js.map both carry a second dot after .test., so /\.(test|spec)\.[^.]+$/ rejects them and only .test.js is considered.
Low — __snapshots__ excluded only incidentally	✅ Fixed, added to IGNORED_DIRS with the reasoning inline.
Low — '…Z' as unknown as Date	✅ Fixed, now new Date('2020-09-22T13:03:20.518Z'); the three-line justification comment is gone with it.
Low (new last pass) — CLI narrowing invisible to the config guard	🟡 Partially. Points 2 and 3 done: the comment at lines 94-96 now says explicitly that the check "bounds the config, not every route in" and names the unit script, and narrowingKeys/toEqual([]) gives a failure message that names the offending key instead of expected undefined, received [...]. testRegex is in the list with the mutual-exclusivity note. Point 1 — dropping the positional — was not done; see below.

Also re-verified in the current tree: PACKAGE_ROOT (__dirname is <pkg>/dist/test, so ../.. is right), compiledCounterpart against tsconfig.json (rootDir: ".", outDir: "dist"), micromatch's brace support for {test,spec}, and micromatch: "^4.0.8" at package.json:67 matching an existing yarn.lock key.

I could not run the suite — node_modules isn't installed in this environment and a monorepo yarn install is out of budget for a review pass. CI is the check on the 238-green figure.

Remaining finding

Low — package.json:18 still passes dist/test positionally.

"unit": "CUBE_JS_NATIVE_API_GATEWAY_INTERNAL=true jest --coverage --forceExit dist/test"

Now that the guard documents this as out of scope, the code and the comment agree — so this is no longer a correctness gap in the test, just a leftover. It's still worth dropping, for one reason that isn't about the guard: the PR body says the suite is "now green with no path argument, so the config carries the target rather than the script compensating for it", and the script does still carry the argument. Either drop it or reword that bullet, so a reader auditing the claim in six months doesn't conclude the fix was never finished. Functionally it filters nothing today — the positional is a superset of testMatch. Fix this →

Praise

• Choosing the relative-path form of the win32 fix over the absolute-path one closes two failure modes with one change, and the comment explains the asymmetry (vacuous pass on one assertion, all-files-red on the other) rather than just asserting the fix. That asymmetry is the part a future reader would otherwise have to rediscover.
• The afterEach comment says why body-local restore is wrong — "one genuine failure would leave Date.now mocked for every test after it" — which is the durable form. Someone re-adding an inline restoreAllMocks() now has to argue with a stated reason.
• Answering the CLI-narrowing point by scoping the guard's claim in a comment, rather than pretending the check is total, is the right call. A guard that overstates its reach is worse than one that's honest about its edge.
• Still the case that deriving from the real jestConfig.testMatch through micromatch — jest's own glob library — rather than restating the pattern is what makes this survive edits, and that toBeGreaterThan(5) keeps the walk from going vacuous.

Not concerns

• Security, performance, auth: nothing in scope — test infrastructure only, no runtime code paths touched.
• micromatch as a direct devDependency: fine, and not published (files is ["README.md", "dist/src/*"]).
• require() + eslint-disable for micromatch: justified, no @types/micromatch in the tree.
• The local-time new Date(2021, 1, 15, …) constructions in date-parser.test.ts are TZ-dependent, but that's unchanged from master — out of scope here.
• Docs: no user-facing surface. The jest.config.js comment documenting the jest date-parser vs. jest test/date-parser.test.ts gotcha is the right place for it.
• Not migrating to ts-jest: agreed, out of scope, and the PR is explicit.

Resolved 5 of my own threads (the medium plus four lows) as no longer applicable. Left the CLI-narrowing thread unresolved, since the positional is still in the script.

· Branch [`igor/core-735-api-gateway-source-js-tests-never-execute`](https://github.com/cube-js/cube/tree/igor/core-735-api-gateway-source-js-tests-never-execute)