colinhacks / zod

TypeScript-first schema validation with static type inference
https://zod.dev
MIT License
33.09k stars 1.15k forks source link

separate typecheck from runtime testing #3647

Closed mmkal closed 2 months ago

mmkal commented 2 months ago

See https://github.com/vitest-dev/vitest/issues/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 commented 2 months ago

@colinhacks looks like your change https://github.com/colinhacks/zod/pull/3647/commits/f7e7147f29f6620ed127eccf2843790a9a2b1fda 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 https://github.com/vitest-dev/vitest/issues/6151#issuecomment-2232737150, 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 commented 2 months ago

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! 🙌

mmkal commented 2 months ago

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 🙃