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

expect(x).toBeTruthy() should properly narrow typescript types to be non-falsy #2883

Closed
4 tasks done
NateRadebaugh opened this issue Feb 17, 2023 · 11 comments
Closed
4 tasks done

Comments

@NateRadebaugh
Copy link

Clear and concise description of the problem

Currently, my tests must look like the following:

const bearerTokenApi1 = new BearerTokenApi(getApiConfig());
let response: GetTokenResponse | undefined;
try {
  response = await bearerTokenApi1.bearerTokenRequestToken({
    requestTokenRequest: {
      userName: env.TOKEN_USERNAME,
      password: env.TOKEN_PASSWORD,
    },
  });
} catch (error: unknown) {
  const errorMessage = await getErrorMessage(error);
  expect(errorMessage).toBe(undefined);
}

// Focus on these lines:
expect(response).toBeTruthy();
invariant(response);

The "invariant" function from tiny-invariant throws when falsy, similar to the .toBeTruthy() but has proper type narrowing.

I believe expect(response).toBeTruthy() should apply some sort of "asserts" to properly type narrow.

Suggested solution

expect(response).toBeTruthy();
invariant(response);

should become

expect(response).toBeTruthy();

Alternative

No response

Additional context

No response

Validations

@NateRadebaugh
Copy link
Author

I'm not a typescript wizard but this is my best effort and I'm not sure if it's even possible in typescript:

image

@gregor-mueller
Copy link

This would be so great! 👍

Problem

Workararounds

Right now we need to do one of the following (maybe there are other workarounds out there):

1. using non-null assertions

// ...
expect(foo).toBeDefined()
expect(foo!.bar).toBeDefined() // ouch, the linter cries and if we remove the line above, the test will crash
expect(foo!.bar!.baz).toBe(1337) // ouch, the linter cries and if we remove the line 2 lines above, the test will crash
// ...

2. returning after each assertion

// ...
expect(foo).toBeDefined()
if(!foo) return // satisfy TS - but will never be reached
expect(foo.bar).toBeDefined()
if(!foo.bar) return // satisfy TS - but will never be reached
expect(foo.bar.baz).toBe(1337)
if(!foo.bar.baz) return // satisfy TS - but will never be reached
// ...

3. throwing after each assertion

// ...
expect(foo).toBeDefined()
if(!foo) throw new Error('satisfy TS - but will never be reached')
expect(foo.bar).toBeDefined()
if(!foo.bar) throw new Error('satisfy TS - but will never be reached')
expect(foo.bar.baz).toBe(1337)
if(!foo.bar.baz) throw new Error('satisfy TS - but will never be reached')
// ...

4. writing our own assert functions

function expectToBeDefined<T>(val?: T): asserts val is NonNullable<T> {
  expect(val).toBeDefined()
}

export function expectToBe<T>(val?: unknown, compareVal?: T): asserts val is T {
  expect(val).toBe(compareVal)
}

// ...
expectToBeDefined(foo)
expectToBeDefined(foo.bar)
expectToBe(foo.bar.baz, 1337)
// ...

Proposed Solution

It would be awesome, if expect would directly assert the types of the checked element.
I figure, it might not be trivial for some of the assertions, but at least the checks for undefined, null, etc. could be done.
I'd love to support this endeavour but it's not easy for me to find time to work on a PR.

I tried to figure out where vitest gets the types from and searched for similar issues/prs in Jest (issue) and DefinitelyTyped (issue, PR (was reverted)). So maybe the PR should be done in one of those packages, but I'm not really sure right now.

Hope this helps! Thanks for the awesome package! ❤️

@stabai
Copy link

stabai commented Sep 19, 2023

@gregor-mueller it looks like the typings for this are part of vitest, if you're willing to see if a PR would be accepted:

export interface JestAssertion<T = any> extends jest.Matchers<void, T> {
// Jest compact
toEqual<E>(expected: E): void
toStrictEqual<E>(expected: E): void
toBe<E>(expected: E): void
toMatch(expected: string | RegExp): void
toMatchObject<E extends {} | any[]>(expected: E): void
toContain<E>(item: E): void
toContainEqual<E>(item: E): void
toBeTruthy(): void
toBeFalsy(): void
toBeGreaterThan(num: number | bigint): void
toBeGreaterThanOrEqual(num: number | bigint): void
toBeLessThan(num: number | bigint): void
toBeLessThanOrEqual(num: number | bigint): void
toBeNaN(): void
toBeUndefined(): void
toBeNull(): void
toBeDefined(): void
toBeInstanceOf<E>(expected: E): void
toBeCalledTimes(times: number): void
toHaveLength(length: number): void
toHaveProperty<E>(property: string | (string | number)[], value?: E): void
toBeCloseTo(number: number, numDigits?: number): void
toHaveBeenCalledTimes(times: number): void
toHaveBeenCalled(): void
toBeCalled(): void
toHaveBeenCalledWith<E extends any[]>(...args: E): void
toBeCalledWith<E extends any[]>(...args: E): void
toHaveBeenNthCalledWith<E extends any[]>(n: number, ...args: E): void
nthCalledWith<E extends any[]>(nthCall: number, ...args: E): void
toHaveBeenLastCalledWith<E extends any[]>(...args: E): void
lastCalledWith<E extends any[]>(...args: E): void
toThrow(expected?: string | Constructable | RegExp | Error): void
toThrowError(expected?: string | Constructable | RegExp | Error): void
toReturn(): void
toHaveReturned(): void
toReturnTimes(times: number): void
toHaveReturnedTimes(times: number): void
toReturnWith<E>(value: E): void
toHaveReturnedWith<E>(value: E): void
toHaveLastReturnedWith<E>(value: E): void
lastReturnedWith<E>(value: E): void
toHaveNthReturnedWith<E>(nthCall: number, value: E): void
nthReturnedWith<E>(nthCall: number, value: E): void
}

I agree that there are a lot of great assertions that would be ideal here. I constantly have to do stuff like this:

expect(queryDefinition.kind).toEqual(Kind.OPERATION_DEFINITION);
if (queryDefinition.kind !== Kind.OPERATION_DEFINITION) throw new Error();

But that second line should be unnecessary if this:

toEqual<E>(expected: E): void

were changed to this:

toEqual<E extends T>(expected: E): asserts expected is E

@sheremet-va
Copy link
Member

sheremet-va commented Sep 20, 2023

were changed to this

This will break valid code:

expect(1).toEqual('2')

This code throws, but it is expected to throw. I would recommend improving assert API instead maybe.

@gregor-mueller
Copy link

This code throws, but it is expected to throw.

Not sure if I understand correctly. The proposed changes would not change whether it throws or not. It would just make TypeScript understand that after successfully executing the function (in case it does not throw, so basically in the next line), what type the parameter has.

...some time later:

Having looked deeper into the issue, it seems that this is not really possible right now. expect would have to be changed as well, because something like toBeDefined does not take in any arguments (will throw TS2776: Assertions require the call target to be an identifier or qualified name.), and we would want to assert the type of the argument for expect.

I found a related TypeScript issue here:
microsoft/TypeScript#40562

@sheremet-va
Copy link
Member

Not sure if I understand correctly. The proposed changes would not change whether it throws or not. It would just make TypeScript understand that after successfully executing the function (in case it does not throw, so basically in the next line), what type the parameter has.

Throwing an error here is expected behavior. TypeScript should not fail here. But it will because E extends T which limits what you can put in toEqual.

@gregor-mueller
Copy link

But it will because E extends T which limits what you can put in toEqual.

Yes, I see. That's what I was going for simpler examples like .toBeFalsy, .toBeTruthy, .toBeDefined, etc...

Something like this would be needed:

toEqual<E>(expected: E): asserts T is E // not possible

or

toEqual<E>(expected: E): asserts this is E // assuming 'this' would be the argument of 'expect()'

Looks like it is just not possible right now, because of the limitations mentioned in the linked TS issues.

@sheremet-va
Copy link
Member

In short, what I am trying to say is that TypeScript will show an error in this example, but as a developer I expect this code to fail, so I don't need TypeScript interfering.

Vitest also provides assert API from chai. If people want this kind of functionality, it might be better to implement it there.

@stabai
Copy link

stabai commented Sep 20, 2023

That makes sense, I got mixed up on the assertion needing to be on the actual value that isn't an argument to the expect method.

It looks like the types for Chai assertions come from here:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b950930e2d11584030da2b1a685c4733626b01b7/types/chai/index.d.ts#L405

They do look like they'd get around the linked TS issues to me. But to avoid the compiler issues that @sheremet-va mentioned, it may require multiple overloads to be provided for some assertions methods.

@UncleSamSwiss
Copy link

UncleSamSwiss commented Jan 18, 2024

It would be awesome to have this directly in the built-in type definitions.

For now I use the following helper function:

export function expectToBeDefined<T>(
  value: T | undefined
): asserts value is T {
  expect(value).toBeDefined();
}

@AriPerkkio AriPerkkio mentioned this issue May 11, 2024
4 tasks
@sheremet-va
Copy link
Member

The team doesn't think this is something that we want to pursue in the Vitest core due to the edge case that will be introduced with this change (see oven-sh/bun#6934).

expect types are also provided in a way when it is currently impossible to implement type assertions because the argument is in another function. Having a helper expectToBeDefined is a valid user-land solution.

@sheremet-va sheremet-va closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants