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] Use vite and vitest for e2e and regression tests #1127

Merged
merged 40 commits into from
Jan 4, 2025

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Dec 17, 2024

  • Test apps built with Vite instead of Webpack
  • Tests are run with Vitest instead of Mocha
  • Removed a bunch of unused devDependencies - in particular webfontloader (and some font loading code) is no longer needed as we are loading local font files

Part of #883

@mui-bot
Copy link

mui-bot commented Dec 17, 2024

Netlify deploy preview

https://deploy-preview-1127--base-ui.netlify.app/

Generated by 🚫 dangerJS against d0bf4b1

@mj12albert mj12albert force-pushed the infra/test-vite branch 6 times, most recently from f21bd83 to c14ce9e Compare December 17, 2024 16:29
@mj12albert mj12albert changed the title [test] Use vite and vitest for regression tests [test] Use vite and vitest for e2e and regression tests Dec 17, 2024
@mj12albert mj12albert force-pushed the infra/test-vite branch 2 times, most recently from 1b7ee44 to 76d77af Compare December 17, 2024 17:23
@mj12albert mj12albert marked this pull request as ready for review December 17, 2024 17:32
@mj12albert mj12albert force-pushed the infra/test-vite branch 2 times, most recently from 61a95b6 to 35cd7ff Compare December 18, 2024 05:29
"test:jsdom": "cross-env NODE_ENV=test VITEST_ENV=jsdom vitest",
"test:chromium": "cross-env NODE_ENV=test VITEST_ENV=chromium vitest",
"test:firefox": "cross-env NODE_ENV=test VITEST_ENV=firefox vitest",
"test:jsdom": "cross-env NODE_ENV=test VITEST_ENV=jsdom vitest --project @base-ui-components/react --project docs",
Copy link
Member Author

@mj12albert mj12albert Dec 18, 2024

Choose a reason for hiding this comment

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

These all need to be filtered with --project now that test also includes Vitest workspaces

I noticed that tests in docs weren't being run at all before explicitly specifying the project flag

@mj12albert mj12albert force-pushed the infra/test-vite branch 2 times, most recently from c8868eb to 3298810 Compare December 18, 2024 06:13
@mj12albert mj12albert marked this pull request as draft December 18, 2024 06:27
@mj12albert mj12albert marked this pull request as ready for review December 18, 2024 06:41
@mj12albert mj12albert force-pushed the infra/test-vite branch 2 times, most recently from f3cf49a to d721946 Compare December 18, 2024 12:44
@mj12albert mj12albert requested a review from mnajdova December 18, 2024 13:00
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2024
Comment on lines -165 to -180
const [fontState, setFontState] = React.useState('pending');
React.useEffect(() => {
webfontloader.load({
custom: {
families: ['Unica 77'],
urls: ['../../docs/src/styles.css'],
},
timeout: 20000,
active: () => {
setFontState('active');
},
inactive: () => {
setFontState('inactive');
},
});
}, []);
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need this as we are not loading webfonts, and the inner TestViewer already waits for font load events

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It looks ok to me, everything seem to work as expected, but I would wait for @michaldudak for a final review before merging. I don't expect lot of conflicts, so it would probably be ok sitting for few weeks.

.eslintrc.js Outdated
files: ['test/**/*{.ts,.tsx}'],
rules: {
'@typescript-eslint/no-use-before-define': 'off',
'import/no-relative-packages': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

Why switching this off?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought it was needed to import the shared vitest config but it's not!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 2, 2025
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

I'm having issues running these tests on Windows - no tests are detected. I suspect this could be related to path separators in globbedRegressionFixtures(test/regressions/main.tsx)

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mj12albert
Copy link
Member Author

suspect this could be related to path separators in globbedRegressionFixtures(test/regressions/main.tsx)

@michaldudak When you run pnpm test:regressions on windows, are the public demos (in docs/src/app/(public)/... picked up by this glob? (or nothing is detected?)

(globbedRegressionFixtures points to an empty directory now, but also it uses a relative path unlike docs/.. which is aliased in the vitest config, not sure if there is any relation)

@michaldudak
Copy link
Member

I managed to fix the Windows issue.

There are a couple of warnings on the console when running tests:

(!) Some chunks are larger than 500 kB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/configuration-options/#output-manualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.

We can hide it by increasing the chunkSizeWarningLimit. We don't care about the chunk size here.

The CJS build of Vite's Node API is deprecated. See https://vite.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

I believe the Vite config file must be an ESM module.

@mj12albert
Copy link
Member Author

I managed to fix the Windows issue.

Thanks ~ @michaldudak

The console warnings are fixed: https://app.circleci.com/pipelines/github/mui/base-ui/6840/workflows/02371255-ba7b-46e5-9d9f-9155ff899ed7/jobs/59536/parallel-runs/0/steps/0-106

I've also moved the vite config to a shared config as they are identical

@mj12albert mj12albert requested a review from michaldudak January 3, 2025 11:43
.eslintignore Outdated
@@ -7,5 +7,6 @@
/packages/react/**/*.min.*
build
build-tests
dist
Copy link
Member

Choose a reason for hiding this comment

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

nit: could this directory be named build to be consistent with the rest of the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! (dist was just the vite default)

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

LGTM!

@mj12albert mj12albert merged commit 9fb1b40 into mui:master Jan 4, 2025
23 checks passed
@mj12albert mj12albert deleted the infra/test-vite branch January 4, 2025 08:37
onehanddev pushed a commit to onehanddev/base-ui that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants