-
Notifications
You must be signed in to change notification settings - Fork 77
test: replace tsd with vitest type checking #6607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
vitest type checking works with source files directly and tsd works with compiled output
To draw your attention, the type testing feature is marked experimental in Vitest. I can’t find any further detail on that. Looking few the issues, these might explain:
I understand it is tempting to use the same tool for everything, but note that Further more, assignability matchers do not exist in the Vitest implementation. A test like:
gets turned into: expectTypeOf(noop).toExtend<OnEnd>() The problem here is readability. Is the type of Seems like the idea behind the At first glance the
Here
is rewritten to: // @ts-expect-error - testing invalid
expectTypeOf<RunFunction>().toBeCallableWith('command', ['arg'], { unknownOption: false }) Note that In the other hand,
What is good about As an alternative, I would like to suggest trying out TSTyche (https://tstyche.org). It is a type testing tool created by me. TSTyche can do more than any other similar tool (and does not have issues mentioned above):
To say it short, TSTyche is simply the same old “do one thing and do it well” type of toll. This is rather lengthy comment. Thanks for reading. |
@mrazauskas Thanks for your detailed review. Just want to clarify that the final decision on tooling and codebase direction is up to the Netlify team - they can reject this PR and stick with tsd, or consider TSTyche as you suggested. I'll briefly address the technical concerns below. For broader discussions about alternative tooling approaches, we might want to continue that conversation elsewhere to keep this PR focused on the specific changes.
There's conflicting information in Vitest's docs. The testing-types guide presents it as standard, while the features page has an experimental badge. Looking at the implementation, it's essentially a wrapper around
Not applicable. They don't use project references for that package - the base config has
Root
👍🏻
👍🏻
👍🏻 Regarding code review, I appreciate the feedback and will iterate on these points. |
await utils.run('command', ['arg']) | ||
await utils.run('command', ['arg'], { preferLocal: false }) | ||
|
||
// @ts-expect-error - TypeScript has no negative-call assertion; this error is the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// @ts-expect-error - TypeScript has no negative-call assertion; this error is the test | |
// @ts-expect-error - 'expect-type' has no negative-call assertion; this error is the test |
By the way, adding some obviously failing assertion: expectTypeOf<string>().toEqualTypeOf<number>() or a line with an error: const a: string = 123 to any of the test files? I see errors in the editor, but Vitest lists all the tests files as passing. Seems like the tests are always passing despite obvious failures, or? |
@mrazauskas You might be mixing up Vitest’s type tests with tsd. In Vitest, type tests are statically checked (not executed) and only run when you enable typechecking—use |
It's the tsconfig. build's package tsconfig uses files/include that didn’t cover test-d, so vitest --typecheck ran but tsc never saw the failing assertions. This could be fixed by adding a dedicated tsconfig for type tests and pointing Vitest to it. It's a leftover from the previous setup. Good catch |
Don’t you think this is Vitest’s responsibility to notice this problem? Listing test files as passing without checking them, sounds rather misleading. |
Summary
Part of the effort to move all your tests to a single test runner. Note, that currently the script runs both type tests and the single existing vitest runtime test together, which is not a big deal now. Renamed files to
*.test-d.ts
following vitest convention.For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)