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

Migrate from Jest to Vitest #1202

Merged
merged 18 commits into from
Oct 22, 2023
Merged

Conversation

ChqThomas
Copy link
Contributor

@ChqThomas ChqThomas commented Oct 18, 2023

Q A
Bug fix? no
New feature? no
Tickets #1018 #610
License MIT

This PR is a draft to test the feasibility of migrating from jest to vitest

Jest doesn't support ESM out of the box, which makes it difficult to test some libraries like Chart.js and Svelte v4. Vitest supports typescript and ESM by default and is compatible with Jest API, it'll help us test those packages and maybe others in the future

Infos:

There is a global config file vitest.config.js which contains the default settings for every package, when needed it can be extended with a file in the package root (ex: src\React\assets\vitest.config.js)

I replaced the yarn test script with vitest src --run so it's run globally and the CI doesn't stop at the first error from a package, it still can be run for every/individual packages with yarn workspaces run vitest --run / yarn workspace "@symfony/ux-live-component" run vitest --run

Libraries like fetch-mock-jest and jest-canvas-mock need to be replaced
The require_context_poylfill.ts file used by ux-react, ux-vue and ux-svelte needs to be modified as it uses a dynamic require, replacing it with an import doesn't work as it is asynchronous

Resources:

TODO:

  • Packages:
    • @symfony/ux-autocomplete 0/14
      • replace fetch-mock-jest ?
    • @symfony/ux-chartjs 0/4
      • fix failing tests
      • replace jest-canvas-mock ?
    • @symfony/ux-cropperjs 1/1
    • @symfony/ux-dropzone 3/3
    • @symfony/ux-lazy-image 1/1
    • @symfony/ux-live-component 217/217 💯
      • Error: Uncaught [ReferenceError: Node is not defined] Just a warning but the test suite pass
    • @symfony/ux-notify 1/1
    • @symfony/ux-react 0/4
      • install @vitejs/plugin-react
      • migrate require_context_poylfill.ts
    • @symfony/stimulus-bundle 1/1
      • Error: Uncaught [ReferenceError: Node is not defined] Just a warning but the test suite pass
    • @symfony/ux-svelte 0/4
      • replace svelte-jester with @sveltejs/vite-plugin-svelte
      • migrate require_context_poylfill.ts
      • fix failing tests
    • @symfony/ux-swup 0/5
      • require() of ES Module node_modules/delegate-it/index.js from node_modules/swup/lib/index.js not supported.
    • @symfony/ux-toggle-password 1/1
    • @symfony/ux-translator 91/91
    • @symfony/ux-turbo 0/2
      • TypeError: Cannot read properties of undefined (reading 'prototype')
    • @symfony/ux-typed 1/1
    • @symfony/ux-vue 0/5
      • replace @vue/vue3-jest with @vitejs/plugin-vue
      • migrate require_context_poylfill.ts
  • Remove jest
  • Cleanup
  • Benchmark ?

I'll need help to resolve these issues 😁

@ChqThomas ChqThomas marked this pull request as draft October 18, 2023 19:57
@weaverryan
Copy link
Member

Thanks for this 💯! I'm going to try to find time to dig a bit this weekend.

@smnandre
Copy link
Member

Does it support parralel runs ? something like

npm workspace run -p 5 eslint

(not working, just to illustrate)

@ChqThomas
Copy link
Contributor Author

@smnandre Not in the current state as this project uses yarn 1.22 (classic) but with yarn v3+ you can:

yarn workspaces foreach --parallel run test

https://v3.yarnpkg.com/cli/workspaces/foreach

@smnandre
Copy link
Member

@ChqThomas ok thanks

So it's probably out of the scope of your current work..

I'd like to work the CI to speed things a bit (a lot?), because i must admin having to wait 2 minutes of JS scripts every time i touch a PHP file is not something i love :)

Capture d’écran 2023-10-21 à 15 14 04

And, as far as i know, it would not be a good idea to condition JS runs to .js files change (as php and js files can impact each other in our packages)

I'm looking at esbuild right now, it could help us

Should i create an issue to discuss this ?

WDYT ?

(cc @weaverryan )

@weaverryan
Copy link
Member

Should i create an issue to discuss this ?

Sure - I'd be interested to see what you have in mind 👍

@weaverryan
Copy link
Member

weaverryan commented Oct 21, 2023

Ok, checked into this. Fantastic work - it got me really excited and I think this is going to work! Using your detailed notes, I added a few more commits and resolved many of the remaining issues. So, we're actually quite close now. I also added a little script that will run all the workspace tests and continue going even if some fail. Just running yarn vitest from the root directory doesn't work, as it ignores the vitest.config.js that's inside some of the packages (it was only using the root). But the new script seems to work and have the desired effect of not stopping if one workspace has a failing test.

A few svelte tests are failing... the markup apparently changed that Svelte is outputting? I'm not sure what's going on there - but there's a good chance we can just change the assertions to match the new markup, though I can't explain the change.

EDIT: I've fixed the rest. I think this might be working! @ChqThomas do you want to remove jest and see if anything else needs to be done? I'm not worried about benchmarking, btw - the tests are fast. If we need to do something faster or run them in parallel, that's a separate thing.

@@ -5,6 +5,7 @@
"module": "dist/register_controller.js",
"version": "1.0.0",
"license": "MIT",
"type": "module",
Copy link
Member

Choose a reason for hiding this comment

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

@stof I'm wondering if I could get your advice. This was needed to get @sveltejs/vite-plugin-svelte to play nicely. In reality, we are already using import inside all packages and our users are using import. So, do you have any concern about this change? If not, we should apply it equally across all packages.

@ChqThomas
Copy link
Contributor Author

@weaverryan Wow that was fast!

A few svelte tests are failing... the markup apparently changed that Svelte is outputting? I'm not sure what's going on there - but there's a good chance we can just change the assertions to match the new markup, though I can't explain the change.

I can't explain it either, Svelte add HTML comments to identify components in dev mode, seems like they are not stripped out of the output with the Vitest plugin like they were with the Jest one. Updating the assertions is fine 👍

EDIT: I've fixed the rest. I think this might be working! @ChqThomas do you want to remove jest and see if anything else needs to be done? I'm not worried about benchmarking, btw - the tests are fast. If we need to do something faster or run them in parallel, that's a separate thing.

Yup I'll take a final look and completely remove jest config

Great work 😄

bin/run-vitest-all.sh Outdated Show resolved Hide resolved
@ChqThomas ChqThomas marked this pull request as ready for review October 22, 2023 10:29
@weaverryan
Copy link
Member

Thank you very very much Thomas!

@weaverryan weaverryan merged commit 77b916a into symfony:2.x Oct 22, 2023
35 of 37 checks passed
@smnandre
Copy link
Member

smnandre commented Nov 1, 2023

I'm not sure (never heard of vitest until last week so i'm no expert.. at all)... but it seems that we are not using the vitests workspaces ? Is it not worth it ? Or not a need for this repo ?

@weaverryan
Copy link
Member

I wasn't aware of them - so maybe something can be improved

@boedah boedah mentioned this pull request Nov 15, 2023
4 tasks
weaverryan added a commit that referenced this pull request Jan 29, 2024
This PR was merged into the 2.x branch.

Discussion
----------

[ChartJs] Add support for chart.js:^4.0

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Issues        | Fix #607
| License       | MIT

Since #1202 has been merged, which solved the blockage of #610, the support for ChartJs v4 should be as simple as this PR :) and would replace #610.

🤞🏼 on the CI-flow

Commits
-------

56d925a [ChartJs] Add support for chart.js:^4.0
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.

3 participants