search

vitest-dev / vitest

Next generation testing framework powered by Vite.
https://vitest.dev
MIT License
12.61k stars 1.13k forks source link

False negative with typecheck enabled #6151

Closed mmkal closed 3 weeks ago

mmkal commented 1 month ago

Describe the bug

The following test passes!

import { expect, test } from 'vitest';

test('huh', () => {
  expect(1).toBe(2);
});

With this config:

import { defineConfig } from 'vitest/config';

export default defineConfig({
  test: {
    include: ['**/*.test.ts'],
    typecheck: {
      enabled: true,
      ignoreSourceErrors: true,
      include: ['**/*.test.ts'],
    },
  },
});

Note: commenting out the whole typecheck prop makes the test fail as expected, as does commenting out typecheck.include.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-hw9uz6?file=test/huh.test.ts

You can also repro with the zod repo by putting a bad assertion in one of the tests and observing that they continue to pass.

(FYI @colinhacks because of this zod might have a bunch of broken tests on the v4 branch - that's where I found this, with vitest ^1.6.0, not sure about other branches)

Note: with "vitest": "latest" the "Tests" still pass but there's an unhandled error with no extra info.

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    vitest: 2.0.3 => 2.0.3

Used Package Manager

npm

Validations

sheremet-va commented 1 month ago

There is a reason why typecheck has a separate include field - the test can either be one or another. We should probably throw an error if they intersect, but the idea here is to use a separate workspace project if the files are the same (this is also how the browser mode works, for example):

export default defineWorkspace([
  {
    extends: './vitest.config.ts',
  },
  {
    extends: './vitest.config.ts',
    typecheck: {
      enabled: true,
      include: [sameInclude],
    }
  },
])
mmkal commented 1 month ago

I see, agreed would be good if there was an error to avoid false negatives. In the meantime, what would you recommend for https://github.com/colinhacks/zod/tree/v4?

That's using a vitest.root.ts: https://github.com/colinhacks/zod/blob/v4/vitest.root.ts And then each package has a vitest.config.ts which merges vitest.root.ts with its config: https://github.com/colinhacks/zod/blob/v4/packages/zod/vitest.config.ts

Should vitest.root.ts become vitest.workspace.ts? That's what I did in this PR on zod: https://github.com/colinhacks/zod/pull/3647 - does that look about right to you? (it does seem to resolve the problem)

colinhacks commented 1 month ago

I ended up fixing this problem in Zod v4 (https://github.com/colinhacks/zod/tree/v4) by adding a types.test-d.ts file that simply imports Zod's source code. This is a hack that was necessary to pull Zod's source code into the typechecker.

import * as z from "../src/index"

export {z};

I think the current behavior of typecheck.include is very surprising and possibly dangerous — my entire test suite wasn't even being executed, but there was no visual indication that this was happening.

It doesn't make sense to me that a file is either a "runtime test file" or "type test file". This doesn't actually seem to be true, because my test files (*.test.ts) are getting properly type checked when I have typecheck.enabled = true. But for some reason, the source files that are referenced by these (regular) test files don't get type checked.

It seems source files only get type checked when they're imported from a *.test-d.ts file. I'd strongly encourage you to reconsider that, if it's intentiional. (Correct me if I'm misunderstanding @sheremet-va)

The pattern that most library authors want is this:

Here's the set of steps I took In trying to achieve this:

  1. I naively set typecheck.enabled = true. But this didn't run typechecking on any of my source code.
  2. I assumed this was an issue with how files were getting included in the TS compilation, so I added a wide typecheck.include (**/*.ts)
  3. This means all my actual test files were now marked as "type test files" and my entire runtime test suite was disabled. I didn't realize this until @mmkal started digging into this, because the output still prints and it looks like everything's passing, but actually nothing is actually being executed :( That's pretty surprising, and I think it's worth reconsidering how these things interact.
sheremet-va commented 1 month ago

It seems source files only get type checked when they're imported from a *.test-d.ts file. I'd strongly encourage you to reconsider that, if it's intentiional. (Correct me if I'm misunderstanding @sheremet-va)

It's a bug: https://github.com/vitest-dev/vitest/issues/5868

The pattern that most library authors want is this

Here is the comment on why this is hard to achieve: https://github.com/vitest-dev/vitest/issues/5019#issuecomment-1917430500

The team agrees that it should be possible to run both at the same time:

But typechecking is low on our priorities at the moment. What I can do right now is throw an error if they intersect or introduce another bug where if the do, they will just run, but the errors will be reported as source errors.

AlexPaven commented 1 month ago

Since the consensus seems to be typecheck and runtime tests should be able to overlap, I would urge you to reconsider excluding test.each (and the rest of the APIs that error out at typecheck); if the main problem is test names, is there a way to just use the argument to the test method, regardless of what the actual runtime name of the generated tests would be? e.g.: test.each([1,2,3])('name %i', i => {}); would generate actual tests named name 1 etc., but for typecheck the reported test name could just be 'name %i'. I'm not familiar with vitest's code so I can't tell at a glance if that's viable or not.

I have a codebase that has a few test.each calls, and it uses types autogenerated from swagger files, it would be great if vitest could tell me immediately (as test results to keep everything under one umbrella) if any tests have typecheck errors when the types get updated - right now they pass even if vscode/typescript show type errors. But if test.each will still error out I can't enable typecheck for the entire project.

I also tried converting test.each to for loops, and that seems to work to some extent, but the tests are no longer discoverable for example in vscode. I can probably live with that for now, or maybe the vscode extension could be improved instead to detect these tests (but I'm guessing it's not that simple). Do you know if there's any other consequences to this?

Edit: I was slightly wrong, tests in a for loop are detected fine when typecheck is disabled, they only disappear from test explorer when typecheck is enabled... if I have type errors in those files - that must mean this approach could work fine when these changes are made!