-
Notifications
You must be signed in to change notification settings - Fork 74
chore(tests): switch to vitest MCP-65 #363
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
Conversation
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.
Pull Request Overview
This PR replaces Jest with Vitest as the test runner to simplify ESM/TypeScript support and reduce configuration overhead.
- Adds a
vitest.config.ts
for Vitest setup - Updates TS config and package dependencies to reference Vitest types and remove Jest
- Rewrites all tests to use Vitest APIs (
vi
,expect
,describe
, etc.)
Reviewed Changes
Copilot reviewed 43 out of 46 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
vitest.config.ts | New Vitest configuration |
tsconfig.jest.json | Switched "types" from Jest to Vitest |
package.json | Removed Jest deps, added vitest |
jest.config.cjs | Deleted (old Jest config) |
eslint.config.js | Updated ESLint to include vitest.config.ts |
tests/unit//*.test.ts, tests/integration/ | Replaced Jest imports, mocks, and assertions with Vitest |
import logger, { LogId } from "../../src/common/logger.js"; | ||
import { createHmac } from "crypto"; | ||
import type { MockedFunction } from "vitest"; |
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.
[nitpick] The manual as unknown as typeof mockApiClient
casting is complex. You can simplify typing by using const mockApiClient = vi.mocked(ApiClient, { shallow: true });
to get strongly typed mocks without workarounds.
Copilot uses AI. Check for mistakes.
Pull Request Test Coverage Report for Build 16275099623Details
💛 - Coveralls |
@@ -8,6 +8,7 @@ import { | |||
expectDefined, | |||
} from "../../../helpers.js"; | |||
import { IndexDirection } from "mongodb"; | |||
import { expect, it } from "vitest"; |
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.
these imports can actually be avoided by opting in to use globals which would make this PR look less scary. However I'm not a fan of globals in general as they're a bit too magical and hide real dependencies.
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 think using globals in this particular situation is kind of fine, but not a strong opinion.
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.
yeah another thing with globals was that every time it required eslint/typescript/etc. configuration to make it apply to some files but not others so this ended up being quicker
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.
Looks reasonable - no strong feelings on vi vs the rest. Looks like the coverage reporter is slightly different though and will now report 2 lines for each missed line (now seems to be counting the closing curly brace). Probably not a blocking issue.
@@ -8,6 +8,7 @@ import { | |||
expectDefined, | |||
} from "../../../helpers.js"; | |||
import { IndexDirection } from "mongodb"; | |||
import { expect, it } from "vitest"; |
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 think using globals in this particular situation is kind of fine, but not a strong opinion.
expect(items2).toHaveLength(2); | ||
expect(items2.map((item) => item.text)).toIncludeSameMembers([ | ||
'Name: "collection-1"', | ||
'Name: "collection-2"', | ||
]); | ||
expect(items2.map((item) => item.text)).toEqual( | ||
expect.arrayContaining(['Name: "collection-1"', 'Name: "collection-2"']) | ||
); |
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.
Can this be .strictEqual(...)
?
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.
a lot of these toIncludeSameMembers
tests are not order-sensitive but toStrictEqual
is so generally no
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.
Ah, true. Kind of annoying there's no built-in matcher for arrays ignoring the order.
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.
we can add custom matcher; honestly will be great to use that kind of stuff more, will add it now
@@ -20,7 +21,7 @@ describeWithMongoDB("aggregate tool", (integration) => { | |||
validateThrowsForInvalidArguments(integration, "aggregate", [ | |||
{}, | |||
{ database: "test", collection: "foo" }, | |||
{ database: test, pipeline: [] }, |
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 is actually best argument against globals, we had an accidental usage of jest's test
global here.... I'm actually surprised the test was passing...
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.
LGTM once eslint is happy. Also, if there's an easy fix for the coverage reports being akward, we should apply it, otherwise we'll have to live with it
This is a proposal to switch to vitest in light of continued issues with ESM modules and jest. Jest support for ESM modules is still experimental and it will likely require constant patching to keep it working as we end up adopting more ESM-based modules. This also applies to extra TypeScript processing.
vitest is tried and tested now and is essentially a drop-in replacement for jest (minus our minor usage of
jest-extended
, which is replaceable).Pros of vitest
vi.mocked
.Cons of vitest
Possible alternatives:
In my opinion minimal configuration is preferable and vitest is mature enough and well worth being adopted.