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

chained methods do not have correct ExtraContext #2892

Open
6 tasks done
everett1992 opened this issue Feb 20, 2023 · 4 comments
Open
6 tasks done

chained methods do not have correct ExtraContext #2892

everett1992 opened this issue Feb 20, 2023 · 4 comments
Labels
help wanted Extra attention is needed p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@everett1992
Copy link
Contributor

everett1992 commented Feb 20, 2023

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

@everett1992
Copy link
Contributor Author

I think it's caused by this line

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

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.

@sheremet-va sheremet-va added pr welcome p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 4, 2023
@sheremet-va sheremet-va added help wanted Extra attention is needed p2-edge-case Bug, but has workaround or limited in scope (priority) and removed bug pr welcome p3-minor-bug An edge case that only affects very specific usage (priority) labels Feb 16, 2024
@jsaelhof
Copy link

jsaelhof commented Oct 4, 2024

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
Copy link
Contributor

hi-ogawa commented Oct 7, 2024

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;
});

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Nov 3, 2024

Now that TestAPI became same as CustomAPI #6707, using @vitest/runner is not necessary. https://stackblitz.com/edit/vitest-dev-vitest-kfsrmj?file=test%2Fcontext.test.ts

import { it, type TestAPI } from 'vitest';

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

No branches or pull requests

4 participants