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

Feature/jest to vitest migration #6605

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Abhishek-17h
Copy link
Contributor


If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.

Closes #6326

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit fd6396b
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/67ac4a8a1195ff000817efbb

@Abhishek-17h
Copy link
Contributor Author

I’ve created this draft PR to get feedback on my current progress for this issue. Here’s what I’ve completed so far and what remains to be done:

Remaining Work:
Configure Vitest to make it the default unit test runner for Volto.
Write documentation on how to migrate projects and add-ons to use Vitest.
Create general documentation about Vitest for new users.
Migrate the remaining test files to Vitest.
I’d like to confirm that I’m heading in the right direction with my work so far. If someone could guide me on configuring Vitest as Volto’s unit test runner, it would be really helpful, as I want to ensure there are no mistakes in the configuration files.

Looking forward to your feedback!

@Abhishek-17h
Copy link
Contributor Author

@tisto @sneridagh

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

I added some preliminary comments, please take a look at them. I know it's a WIP still, but I hope they help.
I haven't gone through all the changes, but the few I went look good.
It's an humongous work, so thank you for taking care of it!

You already changed the scripts commands, so it should work, CI points at them:
pnpm --filter @plone/volto test

CI is RED because you need to update the lockfile (pnpm install).

packages/volto/.i18nrc.js Outdated Show resolved Hide resolved
packages/volto/package.json Outdated Show resolved Hide resolved
packages/volto/package.json Outdated Show resolved Hide resolved
packages/volto/package.json Outdated Show resolved Hide resolved
packages/volto/package.json Outdated Show resolved Hide resolved

jest.mock('../Form/Form', () => jest.fn(() => <div className="Form" />));
vi.mock('../Form/Form', () => ({
Copy link
Member

Choose a reason for hiding this comment

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

We should include some docs about how mocks work, and the recommended way in usual cases, like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add documentation about how mocks work and the recommended approach for common cases like this. That will make it easier for others to follow.

packages/volto/vitest.config.js Outdated Show resolved Hide resolved
packages/volto/vitest.config.js Outdated Show resolved Hide resolved
@Abhishek-17h
Copy link
Contributor Author

@sneridagh Thank you for the helpful review! I’ll implement the suggestions.

@Abhishek-17h Abhishek-17h force-pushed the feature/jest-to-vitest-migration branch from a617af1 to ac8055c Compare January 25, 2025 05:54
@Abhishek-17h
Copy link
Contributor Author

Abhishek-17h commented Feb 11, 2025

Hey @sneridagh, I’m in the final phase of this work and need your help with an issue I’ve been stuck on for the past few days.

In this file https://github.com/plone/volto/blob/main/packages/volto/__tests__/create-addons-loader.test.js, perticuarly in makeAddonLoader function, CommonJS syntax require is used, which is not compatible with Vitest. I replaced it with async/await'and import, but it’s still not working.

Could you help me out with this?

@Abhishek-17h Abhishek-17h force-pushed the feature/jest-to-vitest-migration branch from 7bc629f to f9c3d3c Compare February 11, 2025 04:25
@Abhishek-17h
Copy link
Contributor Author

Abhishek-17h commented Feb 11, 2025

Only two files are remaining to migrate to vitest
1)https://github.com/plone/volto/blob/main/packages/volto/__tests__/create-addons-loader.test.js
2)https://github.com/plone/volto/blob/main/packages/volto/__tests__/volto-slate/deserialize.test.js
All other test files works well with vitest and migrated to use vitest

@sneridagh
Copy link
Member

sneridagh commented Feb 11, 2025

@Abhishek-17h This is amazing!
I know that these two tests are quite complex. 1) is not that important, we have other means of testing it, 2) definetely is important. Although having both, of course would be desirable. Go for 2) if possible, you can leave the other.

PD: I don't know if using jiti would be useful (a code file loader and parser in runtime) for 1) never tried.

@Abhishek-17h
Copy link
Contributor Author

@sneridagh Thanks for your suggestion! Jiti worked perfectly and solved the problem that had me stuck for a week.

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

Successfully merging this pull request may close these issues.

Vitest
2 participants