Skip to content

ci(types): type-check test files via tsconfig.tests.json, non-blocking first (#3962)#3985

Merged
Yeraze merged 1 commit into
mainfrom
feature/3962-p1-typecheck-tests
Jul 7, 2026
Merged

ci(types): type-check test files via tsconfig.tests.json, non-blocking first (#3962)#3985
Yeraze merged 1 commit into
mainfrom
feature/3962-p1-typecheck-tests

Conversation

@Yeraze

@Yeraze Yeraze commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

Task 1.2 of the remediation plan (#3962, Phase 1).

~557 test files currently get zero type-checking (tsconfig.json excludes **/*.test.ts*), so mock shapes silently drift from real signatures. This adds:

  • tsconfig.tests.json — extends the root config, adds src/**/*.test.ts(x) to the program, relaxes only noUnusedLocals/noUnusedParameters.
  • npm run typecheck:tests script.
  • Non-blocking CI steps in ci.yml (gated to the 24.x leg to avoid 4× matrix cost) and pr-tests.yml, each with an inline comment stating the flip-to-blocking condition (error count = 0).
  • Epic status doc updated through Task 1.2.

Baseline: 283 errors, all in test files, zero in production sources — real drift (e.g. mock checkPermission signatures that no longer match, properties read off mocks that the real type doesn't have). Top ~8 files hold ~150 of them; burndown + flip-to-blocking is follow-up work.

Deviations from the plan (measured, documented in the epic doc)

  1. Full strict kept — noImplicitAny stays ON. The plan suggested turning it off initially; measured, that adds ~50 false-positive errors in production files (disabling evolving-any inference) while hiding real test drift. Strict-on is better on every axis: 283/0 test/prod vs 345/50.
  2. Top-level tests/ dir excluded — including it pulls @types/node into the whole program and poisons frontend inference (~60 spurious implicit-any errors in src/components). Documented in the config header; it can get its own config later if wanted.

Verification

  • Baseline reproducible: npm run typecheck:tests → 283 errors, 0 in prod files.
  • Negative test: a deliberate string-into-number mock error was caught at its exact location, then reverted (283 → 284 → 283).
  • npm run typecheck (prod) unaffected: clean. Builds and vitest untouched.
  • Full suite: 8068 passed, 0 failed, 0 suite failures (success: true).

Refs #3962

🤖 Generated with Claude Code

https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n

…g) (#3962 task 1.2)

283 pre-existing mock/signature-drift errors baselined; flip the CI steps
to blocking when the count reaches 0. Full strict kept (see epic doc for
why noImplicitAny stays on and tests/ dir is excluded).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job


PR Review: ci(types): type-check test files via tsconfig.tests.json

Tasks:

  • Gather context and understand the request
  • Read changed files
  • Analyze for code quality, bugs, security, performance
  • Post review feedback

Overall Assessment

This is a well-structured, pragmatic addition. The approach (non-blocking first, document the flip condition explicitly, gate to one matrix leg) is the right way to introduce a new quality gate against an existing error backlog. The deviations from the original plan are well-reasoned and documented.


tsconfig.tests.json

Minor issue — redundant noEmit:
noEmit: true appears both in the config file (line 13) and in the npm script (tsc -p tsconfig.tests.json --noEmit). This is harmless but slightly noisy; one or the other is sufficient. Convention in this repo is to put it in the config (npm run typecheck does tsc --noEmit while the root config also sets "noEmit": true), so the flag on the CLI is redundant. Worth cleaning up when convenient.

include has a redundant glob:

"include": ["src", "src/**/*.test.ts", "src/**/*.test.tsx"]

"src" already covers src/**/*.test.ts(x) — the additional globs are redundant (though harmless). The meaningful work here is the exclude not filtering them out (unlike tsconfig.json). This is fine, just a nit.

*.spec.ts(x) pattern not included:
tsconfig.json excludes *.spec.ts and *.spec.tsx alongside *.test.ts*. tsconfig.tests.json includes src/**/*.test.ts and src/**/*.test.tsx but doesn't add the spec variants. If any spec files exist under src/ they'd be silently skipped. Worth confirming the codebase uses only *.test.* naming — if so, a comment noting that would make the intent clearer.


.github/workflows/ci.yml (lines 79–85)

The gate to 24.x only is clearly justified in the PR description (avoid 4× matrix cost). The inline comment explaining the flip condition is exactly right. No issues.


.github/workflows/pr-tests.yml (lines 93–97)

Consistent with ci.yml. One very minor observation: pr-tests.yml runs typecheck:tests unconditionally (no if: matrix.node-version filter needed since this job already pins to 24.x). This is correct as-is.

generate-test-summary step has a stale $? check (pre-existing, not introduced here):
Line 109 if [ $? -eq 0 ] always evaluates the exit code of echo, not the test runner, because run: blocks don't chain that way. This is pre-existing and out of scope for this PR, but worth flagging for follow-up.


package.json

Script addition is clean and follows existing convention (typechecktypecheck:tests). No issues.


Summary

Area Status
tsconfig.tests.json correctness ✅ Sound
noEmit duplication ⚠️ Minor nit (harmless)
spec file coverage gap ⚠️ Worth confirming
CI non-blocking gate ✅ Well-structured
Flip-to-blocking documentation ✅ Clear and actionable
Production typecheck unaffected ✅ Verified per PR description

The PR achieves its stated goal cleanly. The nits above are all non-blocking — the core implementation is solid and a good step toward the remediation epic goal.


@Yeraze
Yeraze merged commit f1fcf57 into main Jul 7, 2026
19 checks passed
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.

1 participant