-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: return a disposable from doMock() #9332
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
| } | ||
| }) | ||
|
|
||
| test.skipIf(Symbol.dispose)('doMock works with using', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems weird, because Symbol.dispose in _incrementMock?.[Symbol.dispose] is always undefined and test always checks for _incrementMock[undefined]
Maybe it should instead check Object.keys(_incrementMock)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had lifted the pattern from here:
vitest/test/core/test/jest-mock.test.ts
Lines 758 to 762 in d3afa60
| describe.skipIf(Symbol.dispose)('in environments not supporting it', () => { | |
| it('does not have dispose property', () => { | |
| expect(vi.fn()[Symbol.dispose]).toBeUndefined() | |
| }) | |
| }) |
I wasn't sure if Symbol.dispose was patched in between evaluating the skipIf condition and the test body or something, for example, by something like
vitest/test/core/test/esnext.test.ts
Line 22 in e922e92
| Symbol.dispose ??= Symbol('dispose') |
If Symbol.dispose really is undefined, then maybe we just skip the test altogether? There are already tests that assert that doMock()'s functionality works, so it doesn't really matter what the return value is in a non-using-supporting environment 🤷 Or, like you say, we could examine the keys of the object, but it's just a little weird to try to do that when we can't name the key that we're interested in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if there is no reason to test something, then don't test it
Description
This makes
doMock()return a disposable that calls the correspondingdoUnmock()upon being disposed.Resolves #8425
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.