Skip to content
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

separate typecheck from runtime testing #3647

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Jul 17, 2024

See vitest-dev/vitest#6151

In short: tests pass even with assertions that should fail. Luckily there don't seem to be any of those right now, but you can reproduce this by checking out the v4 branch and adding expect(1).toEqual(2) to one of the tests, and note that pnpm test will still pass:

image

With this change it fails as expected:

image

It seems this is because when you specify typecheck.include, you're actually telling vitest to only test the types for that config. So you need two configs: one for runtime, and one for compile-time.

Note 1: I updated vitest too since I thought originally this was a bug in vitest.
Note 2: watch: false doesn't seem to be allowed in the workspace variant of a Config.

@mmkal mmkal force-pushed the vitest-workspace-typecheck branch from 59ca2aa to 6d5916c Compare July 17, 2024 19:58
@mmkal
Copy link
Contributor Author

mmkal commented Jul 18, 2024

@colinhacks looks like your change f7e7147 now makes the typecheck not work:

image

On the commit before that, it finds type errors:

image

It sounds like, according to @sheremet-va in vitest-dev/vitest#6151 (comment), vitest is either testing types or runtime (and not both?) so I don't think it's possible to get away with typecheck in a single config.

@colinhacks
Copy link
Owner

colinhacks commented Jul 18, 2024

Thanks Misha. I pushed a commit before understanding the full scope of the problem.

I ended up solving this in a simpler way by adding a types.test-d.ts. By existing, this file matches the default typecheck.include pattern and pulls all of the source files into the typechecker.

This is dumb as hell...I'm gonna have to write a blog post because I don't think many other people have figured out how to do this.

And the fact that setting typecheck.include disables runtime tests is super unexpected and frankly dangerous.

Amazing catch, thanks for this! 🙌

@colinhacks colinhacks merged commit 957da06 into colinhacks:v4 Jul 18, 2024
3 checks passed
@mmkal
Copy link
Contributor Author

mmkal commented Jul 18, 2024

Thanks Misha. I pushed a commit before understanding the full scope of the problem.

I ended up solving this in a simpler way by adding a types.test-d.ts. By existing, this file matches the default typecheck.include pattern and pulls all of the source files into the typechecker.

This is dumb as hell...I'm gonna have to write a blog post because I don't think many other people have figured out how to do this.

And the fact that setting typecheck.include disables runtime tests is super unexpected and frankly dangerous.

Amazing catch, thanks for this! 🙌

Yeah, this could affect a lot of setups. Pretty hard to test for false negatives, you have actually write broken code...

At work I wrote a saboteur GitHub Action which would create pull requests with type errors, broken source code, failing test assertions, etc. The PR would autoclose if CI resolved with a failure, and stay open with an angry comment if it was green 🙃

@mmkal mmkal deleted the vitest-workspace-typecheck branch July 18, 2024 13:27
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.

2 participants