Skip to content

ci: type-check the test files a pull request changes (#1364) - #1366

Open
prx-my wants to merge 6 commits into
OWASP:mainfrom
prx-my:test/issue-1364-typecheck-tests
Open

prx-my wants to merge 6 commits into
OWASP:mainfrom
prx-my:test/issue-1364-typecheck-tests

Conversation

@prx-my

@prx-my prx-my commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What changed and why

Jest transpiles tests without type-checking them, so a test can assert a shape the compiler would reject and stay green. tsconfig.test.json also sets isolatedModules, but the missing gate is the point: no workflow ran the compiler over the test tree.

Decision: gate new and changed files first, rather than fixing the whole backlog in this PR. The full test tree currently reports 261 errors, concentrated in a handful of files (scanner-cache 83, parent-upgrade 33, scanner-npm-advisory 31). Fixing them all here would turn the gate review into a large, risky refactor; gating changed files stops new debt immediately.

How it works

  • scripts/typecheck-changed-tests.mjs <base-ref> lists the tests/**/*.ts files the branch added or modified with git diff -z, so spaces, Unicode and newlines survive.
  • It drives TypeScript through the compiler API, gives it the changed files as roots, and matches diagnostics on resolved absolute paths.
  • Diagnostics attached to unchanged files are counted but do not fail the PR. Configuration errors, diagnostics with no file, and compiler exceptions always fail.
  • The build job runs it on pull_request events. The base commit is fetched shallowly by SHA, so it works for fork PRs too.
  • CONTRIBUTING.md documents the local command.

Verification

  • tests/scripts/typecheck-changed-tests.test.ts runs the real script against throwaway git repositories and covers: a normal type error, a Unicode filename, a filename with spaces plus errors in an imported unchanged file, a valid changed file with unchanged-file debt, no changed tests, and an unparseable TypeScript config.
  • actionlint clean; npm run build and npm run lint:tests pass.
  • CI probe completed: a committed type error in a changed test made build fail at Type-check changed tests with TS2322 (https://github.com/OWASP/cve-lite-cli/actions/runs/39554341092), while the suite itself still passed. The probe was reverted.
  • The gate now type-checks the new regression test file itself on this PR: Type-checked 1 changed test file(s): no errors.

Part of #1364 - the gate is in place; the remaining pre-existing errors are backlog work.

Nothing type-checked the test tree: tsconfig.test.json sets
isolatedModules, the main config includes only src, and CI never ran the
compiler over tests. A test could assert a shape the compiler rejects and
still pass.

Gate new and changed test files first while the existing 261 errors are
worked down. The gate feeds tsc a config containing only the files the PR
touches, then reports diagnostics against those files; pre-existing errors
in unchanged files are counted but do not fail the build.

Part of OWASP#1364
@prx-my
prx-my requested a review from sonukapoor as a code owner October 21, 2026 06:15
Copilot AI balanced review requested due to automatic review settings October 21, 2026 06:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@prx-my

prx-my commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

CI probe, as promised:

So the gate catches exactly the failure mode the issue describes, and a changed test cannot ship untyped.

Read paths from git diff -z so spaces, Unicode and newlines survive.
Drive tsc through the compiler API and match diagnostics on resolved
absolute paths, so only diagnostics attached to unchanged files are
ignored. Configuration and compiler failures now fail the gate.

Add regression coverage for the failure paths, including a changed file
that imports an unchanged file with its own errors.
@prx-my

prx-my commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Review points addressed:

  • Safe filenames: git diff -z, split on NUL, no trim(). Paths with spaces, Unicode, or newlines survive.
  • Structured diagnostics: replaced the output regex with the TypeScript compiler API. Diagnostics are matched on resolved absolute paths against the changed set.
  • Global failures: configuration errors, diagnostics with no file, and compiler exceptions now fail the gate. Only diagnostics explicitly attached to unchanged files are ignored.
  • Regression tests: tests/scripts/typecheck-changed-tests.test.ts covers normal errors, Unicode names, spaces with unchanged imported errors, passing with unchanged-file debt, and the no-changes skip. Each test runs the real script against a throwaway git repo.

CI is green, and the gate type-checked the new test file on this PR (Type-checked 1 changed test file(s): no errors). Still Part of #1364; the backlog remains open.

Remove the unused result bookkeeping and exports now that the tests drive
the CLI, correct the comment to say Jest only transpiles tests, and keep
configuration failures on the fatal path. Add regression coverage for an
unparseable test config and make the messages refer to errors in
unchanged files rather than pre-existing debt.
@prx-my

prx-my commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Cleanup applied: shorter header with the corrected Jest/type-checking explanation, unused ok/exports/import guard removed, noEmitOnError dropped, messages now say "errors in unchanged files" and "Test type-check failed", plus a sixth regression test for an unparseable tsconfig.test.json. PR description updated; CI is green and the gate type-checked the new test file.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The scoped gate, CI integration, documentation, and regression tests have no identified blocking issues.

No open issues 🎉

Review effort: Balanced

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving this. The gap it closes is real and the shape of the fix is right.

Jest transpiles the test files without type-checking them, so a type error in a test never surfaces: the suite goes green and nothing tells you. On current main there are 264 such errors across 51 test files. That is not a backlog this pull request has to clear, and it correctly does not try.

What makes it workable is that it only looks at the test files a pull request actually changes, so those 264 stay where they are and only new ones block. I planted a type error in a test file and the gate exits 1 while the suite on that same file passes, which is exactly the behaviour that could not happen before.

Two things I liked that you were not asked for. The workflow passes the base commit through the environment and quotes it rather than interpolating it into the shell, which is the same pattern we had to retrofit onto the release workflow last week. And there are no casts, no any and no suppression comments anywhere in it, which would have been the easy way to make a type-check gate go green.

Merge result is clean against current main at 178 suites and 2,470 tests. Replacing the failing exit with a successful one fails 4 of your 6 tests, so the coverage discriminates.

One number for the record: your body says 261 pre-existing errors and I measure 264 today. That is main moving under you rather than a miscount, and it is an argument for the gate rather than against it.

This branch has not been deployed

No deployments
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.

3 participants