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

test: migrate slow tests to use vitest #3802

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Jan 16, 2025

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

This PR moves Electron Forge's slow test suite to use Vitest as a follow-up to #3797. A few notes:

Test migration

Migrating these tests was a lot more straightforward because they run raw commands via child process instead of asserting against mocks. These tests didn't use sinon/proxyquire at all, so I just had to change the Mocha/Chai test harness and assertion calls over to Vitest.

Fixture migration

I moved all files over to /spec/ including test fixtures, so the actual code diff is smaller than it seems.

package.json cleanup

  • I also removed all test npm scripts from each individual package's package.json because we run a single instance of vitest from the root of the package rather than using lerna to execute each package's individual scripts.
  • chai, mocha, and associated packages are gone!

Removal of unnecessary test utils

I removed expectProjectPathExists and expectProjectPathNotExists from @electron-forge/test-utils since these helper functions were essentially calling fs.existsSync with extra steps.

Workspace organization

The fast and slow suites are now assigned as Vitest workspace projects based on the test file names.

  • vitest.config.mts is the base file.
  • vitest.workspace.mts contains separate configs for slow and fast tests (mostly just increased test timeouts for the slow test suite).

Note that I had to turn off file parallelism for the time being because there were issues with running multiple test files at once (potentially around FS collisions?). Once this lands, I'll open an issue to track the problem.

@erickzhao erickzhao changed the title Slow tests vitest test: migrate slow tests to use vitest Jan 16, 2025
@erickzhao erickzhao marked this pull request as ready for review January 17, 2025 18:21
@erickzhao erickzhao requested a review from a team as a code owner January 17, 2025 18:21
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another awesome PR! 🎉

I think we could also drop the cross-env dependency in this PR? Looks like it was only used with the Mocha test script commands. Should be able to remove it from package.json and packages/api/core/package.json.

@@ -28,11 +21,8 @@
"@types/interpret": "^1.1.1",
"@types/progress": "^2.0.5",
"@types/rechoir": "^0.6.1",
"chai": "^4.3.3",
"chai-as-promised": "^7.0.0",
"cross-env": "^7.0.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this dependency.

@@ -1,3 +1,3 @@
{
"recommendations": ["dbaeumer.vscode-eslint", "esbenp.prettier-vscode", "maty.vscode-mocha-sidebar"]
"recommendations": ["dbaeumer.vscode-eslint", "esbenp.prettier-vscode"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"recommendations": ["dbaeumer.vscode-eslint", "esbenp.prettier-vscode"]
"recommendations": ["dbaeumer.vscode-eslint", "esbenp.prettier-vscode", "vitest.explorer"]

The Vitest extension works well and is maintained by the Vitest team, let's swap it in here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants