chore: aws-cdk-lib tests were not being typechecked #36294
+432
−403
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
In preparation for something else I want to do (compiling the test files to `.js` again), I'm splitting the source filess inside `aws-cdk-lib` into 2 classes: - "Library" source files: all `.ts` files except those in `test` subdirectories. - "Test" source files: files in `test` subdirectories. In order to make this official, I'm making two `tsconfig.json` files with project references between them: - `tsconfig.lib.json`: addresses library source files. - `tsconfig.tests.json`: addresses test source files, and has a project reference to `tsconfig.lib.json`. Doing this introduces an additional problem: we now have two different tsconfig files in the same directory, and how does your IDE know which config to use based on which file you are currently looking at? Turns out the VSCode plugin will only look for `tsconfig.json` and nothing else. We're therefore also adding the following: - `tsconfig.json`: addresses *all* source files in the subdirectory. - `tsconfig.options.json`: all the other files `extend` this. Contains the TS compiler options in a single place so that we don't have to copy/paste them around. The upshot of doing all this is: **test files are now being type-checked by `tsc` and it turns out they had a ton of type errors in them!** In most cases we saw "unused variable" errors, but there were honest to goodness typing mistakes in there, that just happened to execute properly when runing through `ts-jest` (with type checking ignored). I'm submitting this as a PR by itself to get those changes out of the way.
aws-cdk-automation
added
the
pr/needs-maintainer-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In order to support some other performance-related changes, I was planning to split the sets of source files inside
aws-cdk-libinto 2 classes:.tsfiles except those intestsubdirectories.testsubdirectories, consuming the library as a separately compiled subproject.It turns out this does not work for 2 reasons:
stripInternal: true(TypeScript feature): there are some APIs that we strip out of the.d.tsfiles. If we put a discrete compilation step between the library and the tests, that means that tests can't use@internalAPIs either, which we test in a number of cases.--strip-deprecated(jsii feature): there are some legacy@deprecatedAPIs that we strip out of the.d.tsfiles as well, and the same applies: we have tests for a number of these that start failing once we separately compile the two source sets.Instead, we're just absorbing the test suite into a single
tsconfig.jsonfor this project again.The upshot of doing all this is: test files are now being type-checked by
tscand it turns out they had a ton of type errors in them!In most cases we saw "unused variable" errors, but there were honest to goodness typing mistakes in there, that just happened to execute properly when runing through
ts-jest(with type checking ignored).I'm submitting this as a PR by itself to get those changes out of the way.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license