chained methods do not have correct ExtraContext

everett1992 commented 1 year ago

Describe the bug

Chaining methods reset ExtraContext to {}. it<Context>.skip will use {} instead of Context. While it.skip<Context> will work it's not as ergonomic as it<Context>.skip especially when assigning a context'ed it to a variable for reuse.

const cit = it<Context>;
cit('', ({now}) => {})
// @ts-expect-error :( I don't want to have to repeat Context, or even know what the context is.
cit.skip('', ({now}) => {})

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-pwreuv?file=test/context.test.ts

import { beforeEach, expect, it } from "vitest"

beforeEach<Context>((ctx) => { ctx.now = new Date() })

// 1.
it<Context>('context works without chains', ({now}) => expect(now).toBeDefined())

// 2.
//  @ts-expect-error Now does not exist on TestContext. TestContext is {}
it<Context>.skip('context type is missing with chain', ({now}) => expect(now).toBeDefined())

// 3. This works, however it is not ergonomic. only and skip should be quick to add.
it.skip<Context>('context works when type is given to chained method', ({now}) => expect(now).toBeDefined())

// Option 3 does not work for my use case, where I assign `it<TestContext>` to a variable so I don't have to repeat
// the type in every test. Here's an example
const cit = it<Context>;
cit('', ({now}) => {})
// @ts-expect-error :( I don't want to have to repeat Context, or even know what the context is.
cit.skip('', ({now}) => {})

System Info

$ npx envinfo --system --npmPackages '{vitest,@vitest/*,vite,@vitejs/*}' --binaries --browsers
success Install finished in 1.3s

  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: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    @vitest/ui: latest => 0.28.5 
    vite: latest => 4.1.3 
    vitest: latest => 0.28.5

Used Package Manager

npm

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

everett1992 commented 1 year ago

I think it's caused by this line

https://github.com/vitest-dev/vitest/blob/482b72fc5f02da87498a18e0ed844ecb6bc772ca/packages/runner/src/types/tasks.ts#L148

and it appears to be fixed by changing it to

(name: string, fn?: TestFunction<ExtraContext>, options?: number | TestOptions): void

But I'm not sure if that has other downsides.

jsaelhof commented 2 weeks ago

This issue appears to affect it.for in the same way.

// Referencing the example context by OP, `now` exists and can be used but TypeScript does not see the extended context
it.for<{ b: boolean }>([{ b: true }, { b: false }])("it should", ({ b }, { now }) => {...})

Given that it<Context> works, one would expect this to work but it doesn't it.for<ForType>([ ... ])<Context>("it should", () => {}). The function called on each for doesn't accept a T generic.

hi-ogawa commented 2 weeks ago

This could be some limitation on typescript's it<Context> expression.

Instead of it<Context>, using it as CustomAPI<Context> seems to work better. I guess understanding this difference would require a solid typescript hacker.

https://stackblitz.com/edit/vitest-dev-vitest-mvn39n?file=test%2Fcontext.test.ts

import { it } from 'vitest';
import type { CustomAPI } from '@vitest/runner';

// const cit = it<{ now: Date }>;
const cit = it as CustomAPI<{ now: Date }>;

cit('', ({ now }) => {
  now satisfies Date;
});

cit.skip('', ({ now }) => {
  now satisfies Date;
});

vitest-dev / vitest