-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
test: refactor fast suite to use vitest #3797
base: main
Are you sure you want to change the base?
Conversation
@@ -131,15 +126,14 @@ | |||
"minimist": "^1.2.6", | |||
"mocha": "^9.0.1", |
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.
Note that Mocha and Chai are still needed for our slow test suite. I'll work on removing those in a follow-up PR.
return spawn('npx', args); | ||
} | ||
|
||
describe('cli', { timeout: 30_000 }, () => { |
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.
how was 30s determined for this test? I'm wondering if we can revisit this and adjust as necessary
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 the default and kept increasing it until it stopped flaking 😓
I think a future task would maybe be if we can move this to the slow test suite as well.
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.
Awesome! Excellent PR description, and thanks for trudging through the transition here.
Unfortunately GitHub didn't do a great job consolidating the diff for some of the renamed files, but not sure what caused it to work on some but not others. 🤔 I wonder if renaming both a parent directory and the filename makes it more likely to lose the connection?
@@ -0,0 +1,8 @@ | |||
/// <reference types="vitest/config" /> |
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.
Is this necessary? I'd have expected the types to work correctly without it.
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, I added it because it was recommended in the docs: https://vitest.dev/config/file.html#managing-vitest-config-file
IIRC it giving a type check error without it.
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.
Code LGTM. This PR description is textbook-level!
This PR moves our
test:fast
suite from Mocha/Chai/Proxyquire/Sinon to Vitest.This migration was mostly a syntactical change. I tried to preserve the actual contents of the tests as much as possible.
File renames
By default, Vitest looks for
Module.test.ts
rather thanModule_test.ts
. This seems to be a common standard, so all files are renamed.The new test suite is now under
spec/
instead oftest/
so that there's a clear delineation between the old Mocha tests and the newer Vitest tests.All imports are now explicit
Mocha (like Jest) includes its functions (e.g.
describe
,it
,before
, etc.) as globals when executing tests.On the other hand, Vitest requires you to explicitly import its test API functions.
+ import { describe, expect, it } from 'vitest';
Preferring Jest-like matchers over Chai-like matchers
Previously, this test suite used Mocha for running tests, which paired with Chai for assertions.
Vitest supports both Chai-like and Jest-like assertion syntaxes, which means that we technically didn't have to change any assertions. However, we use Jest across most of our other Electron ecosystem repos, so I migrated all tests to Jest-like syntax.
See comparison below:
Use
vi
API for mockingThis test suite previously used a combination of libraries to mock external dependencies for unit tests:
proxyquire
was used to mock The Node.jsrequire
module loader.sinon
was used to create function stubs.Vitest comes with its own
vi
mocking API (note: it uses Sinon under the hood as well). It provides a more ergonomic approach thanproxyquire
(and supports ESM) and resemblesjest.mock
for those who are familiar with that API.For example, take the mocking of imports in the
start
command tests:vi.fn()
replaces the need to storesinon
stub variables for each mocked function.vi.mock
now applies to each import directly rather than needing to rewire the function we're testing against (in this case, thestart
command).vi.mock
with a dynamic import is type-safe.vi.mock
provides the ability to only partially mock modules with theimportOriginal
helper function.A note on mocking
require
We use ES Module import syntax (i.e.
import { foo } from 'bar'
) in most places in Forge's code. The TypeScript compiler will translate this intorequire
calls under the hood because we output CommonJS in the build process. This is howproxyquire
is still able to work despite us not usingrequire
directly.vi.mock
is easily able to work with this syntax as well, and also grants us the ability to mock pure ESM modules as well if the need arises. However, it does struggle with mocking dynamicrequire
calls that we have in certain maker installers that we include inoptionalDependencies
.For example:
forge/packages/maker/deb/src/MakerDeb.ts
Lines 34 to 36 in 6b74e1a
To properly mock
require
calls, I needed to add a new util to@electron-forge/test-utils
that works around this issue withvi.hoisted
:msw
overfetch-mock
Mock Service Worker (MSW) is the testing utility recommended by the Vitest documentation for mocking network requests.
It was a bit of a pain to get working because it doesn't provide an easy way to assert against calls (had to collect calls manually), but I made it happen.