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

Broken types for extended test context #6660

Closed
6 tasks done
dosolkowski-work opened this issue Oct 7, 2024 · 17 comments · Fixed by #6707
Closed
6 tasks done

Broken types for extended test context #6660

dosolkowski-work opened this issue Oct 7, 2024 · 17 comments · Fixed by #6707

Comments

@dosolkowski-work
Copy link

Describe the bug

When upgrading from vitest 2.0.5 to 2.1.2, something has changed with the types for custom test contexts (export const myTest = test.extend(...)) that results in tsc reporting all test function parameters as having any type, when it previously used to be able to identify their types. An example error:

src/utils/useFrozenState.test.ts:41:63 - error TS7006: Parameter 'initialState' implicitly has an 'any' type.

41     it.each([null, undefined])("refuses to freeze %o", async (initialState) => {
                                                                 ~~~~~~~~~~~~

In this case, it should be able to tell that initialState has a type of null | undefined. Vitest was the only library changed, and reverting back to 2.0.5 restores the prior behavior where types were identified correctly.

Reproduction

Our repository is set up similar to this arrangement: https://github.com/qidydl/vite-workspace-experiment where we use paths and references in tsconfig.json to refer to shared code. The test context in the shared code is not complex:

export interface CustomTestContext {
    user: UserEvent;
}

export const customTest = test.extend<CustomTestContext>({
    user: async ({}, use) => await use(userEvent.setup()),
});

The usage is also not complex:

import { customTest as it } from "@shared/_test/testUtils";

it.each([null, undefined])("refuses to freeze %o", async (initialState) => {
    ...test using initialState
});

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (32) x64 13th Gen Intel(R) Core(TM) i9-13950HX
    Memory: 13.43 GB / 31.61 GB
  Binaries:
    Node: 20.18.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.8.2 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.12.1 - C:\Program Files\nodejs\pnpm.CMD
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    @vitest/coverage-istanbul: ^2.1.2 => 2.1.2
    @vitest/eslint-plugin: ^1.1.7 => 1.1.7
    @vitest/ui: ^2.1.2 => 2.1.2
    vite: ^5.4.8 => 5.4.8
    vitest: ^2.1.2 => 2.1.2

Used Package Manager

pnpm

Validations

@sheremet-va
Copy link
Member

sheremet-va commented Oct 7, 2024

Looks like it fails even with a regular .each and .for:

it.extend<{ userId: number }>({ userId: 100 }).each([null, undefined])('test', async (_initialState) => {
  //
})

it.for([null, undefined])('test', async (_initialState) => {
  //
})

it.each([null, undefined])('test', async (_initialState) => {
  //
})

Seems like undefined and null are always ignored. If I add a different type, then only its type is inferred:

it.each([100, undefined]).each('test', value => {
  // vlaue is a "number"
})

@sheremet-va sheremet-va added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Oct 7, 2024
@dosolkowski-work
Copy link
Author

We do have errors reported even when null and undefined are not involved, for example:

it.each(["Wait", "WaitForProceed"] as const)("deals with wait state %s", async (state) => {
    // tsc -b reports error TS7006: Parameter 'state' implicitly has an 'any' type.
});

But it only does this in the application projects importing the test context from the shared code, not within the shared code, so I'm not sure if there's some interaction with using TypeScript's paths/references to get to the context? The application projects still reference the same version of Vitest.

If I write a basic test:

import { customTest as it } from "@shared/_test/testUtils";

it.each(["foo", "bar", "baz"])("tests string %s", (str) => {
    expect(str).toHaveLength(3);
});

it reports a compiler error, but if I comment out the import so we use the default it implementation from Vitest, the error goes away.

@dosolkowski-work
Copy link
Author

Okay I may have a lead: the shared code generates a .d.ts file where we can see the custom context is declared like this:

export declare const customTest: import("@vitest/runner").CustomAPI<{
    user: UserEvent;
}>;

We don't have a reference to @vitest/runner, so there's an error "Cannot find module '@vitest/runner' or its corresponding type declarations. ts(2307)".

@dosolkowski-work
Copy link
Author

Confirmed that adding a package reference to @vitest/runner to the shared code project fixes the issue. Not sure if this was a breaking change we missed somehow? I think the scenario is scoped to TypeScript projects that export a custom test context, which admittedly is probably not super common.

@sheremet-va
Copy link
Member

Not sure if this was a breaking change we missed somehow?

There was no breaking change. I don't know why your TypeScript code generates a d.ts with an import('@vitest/runner'). Vitest re-exports this type.

I generally don't understand why your code is bundled. Vitest is meant to run on unbundled code.

@dosolkowski-work
Copy link
Author

It's not bundled, the shared code is just run through tsc to generate type definitions, which some tools require (I think ESLint would explode with tons of errors because it would not properly follow the paths/references in tsconfig.json). I'd be happy to find a better alternative to our setup that still works, and it's possible other upgrades mean we don't actually need a tsc transformation any more, but Vitest's re-exporting of the type may not be working correctly.

@dosolkowski-work
Copy link
Author

dosolkowski-work commented Oct 7, 2024

Narrowed it down further: I completely removed the tsc step (maybe a recent upgrade to ESLint v9 fixed that), so there are no .d.ts files and no build output for the shared library, but I still receive build errors for the application project if I do not include @vitest/runner as a dependency of the shared library.

@sheremet-va
Copy link
Member

Do you have a reproducible repository? Vitest ships the @vitest/runner package, so the type error shouldn't happen.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 8, 2024

Seems like undefined and null are always ignored. If I add a different type, then only its type is inferred:

undefined and null are working fine here https://stackblitz.com/edit/vitest-dev-vitest-etm6wp?file=test%2Frepro.test.ts This seems to depend on tsconfig strict, but I guess we can assume that nowadays.

image

When upgrading from vitest 2.0.5 to 2.1.2

@dosolkowski-work Can you check if you don't have multiple versions of vitest installed? That could potentially mess up tsc. You can search vitest@ through your pnpm lockfile.

@dosolkowski-work
Copy link
Author

I can confirm we don't have multiple versions installed (we actually enforce this using syncpack). As part of the upgrade I completely deleted node_modules and the pnpm lockfile, and then reviewed after upgrading, to ensure there wasn't anything strange left over (e.g. @vitest/eslint-plugin somehow still using the old version).

It's possible that pnpm is a factor: because it does not include transient dependencies in node_modules (whereas I think npm did the last time I used it?), @vitest/runner can't be imported unless explicitly listed in package.json, but maybe for other package managers or in different configurations, it can be.

Correction/further information on my prior comment: the shared code project being compiled to .d.ts files is actually just part of how TypeScript project references work--they will be generated by running tsc --build in the application project even if we don't explicitly run tsc in the shared project. It seems intentional that you have to go through this step to share TypeScript code between projects.

I'll attempt to fork and update the repository I linked to in order to reproduce more clearly, sorry it's taking some time.

@dosolkowski-work
Copy link
Author

I forked the repository to https://github.com/dosolkowski-work/vite-workspace-experiment and updated it to show the bug and the workaround (before and after the most recent commit).

The bug is seen in this workflow run: https://github.com/dosolkowski-work/vite-workspace-experiment/actions/runs/11239311536/job/31246077526

whereas with the workaround, the run is clean: https://github.com/dosolkowski-work/vite-workspace-experiment/actions/runs/11239342625/job/31246177713

The return type of test.extend() is CustomAPI<> from here in @vitest/runner: https://github.com/vitest-dev/vitest/blob/main/packages/runner/src/types/tasks.ts#L414 rather than a type from vitest itself. This is also where VSCode (and I assume TypeScript itself) thinks test.extend() is defined.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 9, 2024

Thanks for the repro. It's still hard to believe Vitest would affect this behavior. Can you test how v2.0.5 looks then?
My impression is that your setup is fairly opinionated, so needing to add @vitest/runner to help tsc dts output (also with vitest/globals) sounds reasonable.

Also it would be great if you can strip down the repro further without all those dependencies. If it can be reproduced with just vitest and typescript, then that would help seeing the issue better.

@hi-ogawa hi-ogawa added pending triage and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 9, 2024
@dosolkowski-work
Copy link
Author

I made a separate branch with vitest 2.0.5 for comparison: https://github.com/dosolkowski-work/vite-workspace-experiment/tree/vitest-2.0

In this branch we can see that custom contexts used to depend on vitest directly:

export declare const myTest: import("vitest").TestAPI<{
    user: UserEvent;
}>;

@sheremet-va
Copy link
Member

The TestAPI is exported in the exact same way as it did before:

https://www.npmjs.com/package/vitest/v/2.0.5?activeTab=code
https://www.npmjs.com/package/vitest/v/2.1.3?activeTab=code

@dosolkowski-work
Copy link
Author

That's not the type that TypeScript thinks is returned by .extend(). It doesn't even think .extend() is in vitest, it thinks it's in @vitest/runner, as I noted two comments above. I'm not sure if the definition of it or test changed, so that .extend() is now being found elsewhere?

@sheremet-va
Copy link
Member

Export of test and it also didn't change between those two versions:

- export { CancelReason, DoneCallback, ExtendedContext, HookCleanupCallback, HookListener, OnTestFailedHandler, RunMode, Custom as RunnerCustomCase, Task as RunnerTask, Test as RunnerTestCase, File as RunnerTestFile, Suite as RunnerTestSuite, RuntimeContext, SuiteAPI, SuiteCollector, SuiteFactory, SuiteHooks, TaskBase, TaskContext, TaskCustomOptions, TaskMeta, TaskResult, TaskResultPack, TaskState, TestAPI, TestContext, TestFunction, TestOptions, afterAll, afterEach, beforeAll, beforeEach, describe, it, onTestFailed, onTestFinished, suite, test } from '@vitest/runner';
+ export { CancelReason, ExtendedContext, HookCleanupCallback, HookListener, OnTestFailedHandler, OnTestFinishedHandler, RunMode, Custom as RunnerCustomCase, Task as RunnerTask, TaskBase as RunnerTaskBase, TaskResult as RunnerTaskResult, TaskResultPack as RunnerTaskResultPack, Test as RunnerTestCase, File as RunnerTestFile, Suite as RunnerTestSuite, SuiteAPI, SuiteCollector, SuiteFactory, TaskContext, TaskCustomOptions, TaskMeta, TaskState, TestAPI, TestContext, TestFunction, TestOptions, afterAll, afterEach, beforeAll, beforeEach, describe, it, onTestFailed, onTestFinished, suite, test } from '@vitest/runner';

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 15, 2024

I made a minimal repro here https://github.com/hi-ogawa/reproductions/tree/main/vitest-6660-test-extend-dts
I guess technically Vitest changed the type export for type TestAPI<..> = CustomAPI<...> in @vitest/runner #6068

Previously test.extend returns TestAPI, which is re-exported from vitest, but now test.extend returns CustomAPI, which is only exported from @vitest/runner, so dts has to use import("@vitest/runner").

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants