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

Spied-on functions that return Promises change their mock return values when the Promise resolves #3634

Closed
beckjake opened this issue Jun 20, 2023 · 8 comments
Labels

Comments

@beckjake
Copy link
Contributor

Describe the bug

If a spied-upon function returns a promise, the .mock.results[...].value item starts off as a promise, but when the promise resolves it becomes the resolved item. This behavior doesn't seem to be documented anywhere.

I found this while migrating an existing test suite from jest, and I know this might not be the most desirable test pattern (I will simply change it to not expect this), but I found this behavior pretty surprising.

It looks like it might have been introduced in #2835.

Reproduction

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

Note that you can add any amount of indirection you want here, and it'll behave the same way. So the issue isn't that I'm calling bar and it's spied upon - I originally discovered this issue in a test that spied on bar, called a function that called bar, and examined the mock's result to make sure it was a promise (and then awaited it to examine the result, etc.).

System Info

System:
    OS: macOS 13.4
    CPU: (8) arm64 Apple M1 Pro
    Memory: 317.55 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.5.0 - ~/.nvm/versions/node/v18.5.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.5.0/bin/yarn
    npm: 8.12.1 - ~/.nvm/versions/node/v18.5.0/bin/npm
  Browsers:
    Chrome: 114.0.5735.133
    Safari: 16.5
  npmPackages:
    vitest: ^0.32.2 => 0.32.2


### Used Package Manager

yarn

### Validations

- [X] Follow our [Code of Conduct](https://github.com/vitest-dev/vitest/blob/main/CODE_OF_CONDUCT.md)
- [X] Read the [Contributing Guidelines](https://github.com/vitest-dev/vitest/blob/main/CONTRIBUTING.md).
- [X] Read the [docs](https://vitest.dev/guide/).
- [X] Check that there isn't [already an issue](https://github.com/vitest-dev/vitest/issues) 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](https://github.com/vitest-dev/vitest/discussions) or join our [Discord Chat Server](https://chat.vitest.dev).
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.
@stackblitz
Copy link

stackblitz bot commented Jun 20, 2023

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@sheremet-va
Copy link
Member

This behavior is different from jest's, but it is expected. Not sure exactly what you want to document, but feel free to open a PR.

@ITenthusiasm
Copy link

@sheremet-va Is there a way for a developer to inspect the original promise? I previously found it helpful to be able to inspect the original Promise.

@ITenthusiasm
Copy link

Eh. The use case is because I needed to check whether or not a Promise had resolved. I'm guessing in vitest land the way to do that is to verify that the value is a Promise before resolution and verify that it's a resolved value after resolution?

If that's the case, maybe it isn't necessary to preserve the original. That said, an update to the Migration guide could also be helpful. (Can you tell I'm in the middle of migrating from Jest yet? 😅)

@sheremet-va
Copy link
Member

I don't know. Maybe semantically it's better to not resolve a promise because this is what the actual function return value. So I would consider it a bug. To fix this, it's better to open a separate issue in tinyspy.

@ITenthusiasm
Copy link

ITenthusiasm commented Oct 2, 2023

I feel like you could argue either way. Originally I was a little upset with the implementation. But in the long run, this implementation could be considered advantageous -- as long as it's clearly documented.

The disadvantage of leaving returned Promises alone is that whenever developers want to inspect the details of the Promise, they have to create their own extension of the Promise and overwrite the global Promise object. Then, when the test is finished, they have to restore the original implementation of the global Promise so that unexpected accidents don't happen. This is... a more convoluted development experience. So the current way Vitest works seems like a better DX.

The current way Vitest works simplified my codebase for the aforementioned reason. (I don't have to inspect whether or not the Promise is already resolved. If I have the raw value, I know it resolved. If I still have the Promise, I know it didn't resolve.) If you decide you want to change things anyway to be more like Jest though, I would understand.

@sheremet-va
Copy link
Member

We now document this difference in Vitest documentation so I think it's safe to close this issue.

@ITenthusiasm
Copy link

Thanks for keeping the current implementation. 🙏🏾 I like what Vitest does so much better.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants