setup: type-check test/ during npm run build - #13
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tsconfig.jsonexcludedtest/, sonpm run buildnever type-checked test files, and CI never caught type errors in tests (Vitest transpiles without type-checking). This PR makesnpm run buildtype-checktest/under the exact same strict settings assrc/— without emitting test files intodist/— and fixes the real type errors that the stricter check surfaced.What changed
tsconfig.test.json(new): extendstsconfig.json, so it inheritsstrict,noUncheckedIndexedAccess,noImplicitOverride,noUnusedLocals,noUnusedParameters, andmoduleResolution: "NodeNext". It setsnoEmit: trueand includestest/**/*.ts(plussrc/**/*.tsso imports resolve in one program).tsconfig.json: dropped"test"fromexclude. Itsincludeis alreadysrc/**/*.ts, so nothing else changes; keeping"test"there would have documented exactly the stance this PR reverses.package.json:"build": "tsc -p tsconfig.json && tsc -p tsconfig.test.json". The source emit step is unchanged; the second step only type-checks tests.test/ttl.test.ts: annotated two key arrays as[xdr.LedgerKey, xdr.LedgerKey]tuples sokeys[0]/keys[1]keep their element types undernoUncheckedIndexedAccess.test/discover.test.ts: constructscontractSpec.Errwith{ message: "LimitTooLarge" }, matching the installed SDK'sErr<E extends ErrorMessage>whereErrorMessage = { message: string }— verified against the pinned@stellar/stellar-sdk@16.2.0copy, not against documentation.CONTRIBUTING.md: removed the "test/is not type-checked" warning.Key design decisions
&&in the build script rather than project references:tsc -brequirescompositeprojects, which cannot setnoEmit, so references would have forced test files to emit somewhere. The two-config approach keepsdist/byte-for-byte identical (verified by md5sum) and makes a test-file type error failnpm run build.any/non-null assertions: the existing errors are fixed properly; both constructs are lint errors in this repository.as const:as constproduces a readonly tuple, which is not assignable tobuildReadings'xdr.LedgerKey[]parameter.Acceptance criteria
test/is type-checked under the same strict settings assrc/—tsconfig.test.jsonextendstsconfig.json; no test file is emitted intodist/(noEmit, andfind distshows no test output).noUncheckedIndexedAccessor usingany/non-null assertions — tuple annotations intest/ttl.test.ts; typedErrconstruction intest/discover.test.ts.npm run build— plantedconst _bad: number = "definitely a string", build exited 2 with TS2322, then reverted.npm run buildproduces exactly the samedist/contents — md5sum of all 46 dist files identical before/after.Test output
No coverage threshold is configured in this repository; CI runs the same four commands on Node 22 and 24.
Note on the issue's error listing
The issue names
test/ttl.test.ts:59, but enabling the check surfaced three real errors, not one:test/ttl.test.ts:59and:73(bothnoUncheckedIndexedAccessindexing) andtest/discover.test.ts:89(Errconstructor typing). All three are fixed; none contradicts runtime behavior.Follow-ups (not in scope)
tsconfig.jsoninclude, so an editor falls back to an inferred project for them rather than these strict settings.npm run buildenforces the check; wiring the editor project is a separate concern.Security
No signing path, key handling, or transaction logic was touched. No secrets involved.
Closes #12