Skip to content

[infra] Improve test setup #18228

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 5, 2025
Merged

[infra] Improve test setup #18228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58d8ea9
Test an even listener reloading page on error
LukasTy Jun 4, 2025
fdc4033
Remove duplicate `isJsdom` checks
LukasTy Jun 4, 2025
ba57787
Move Pickers setup only to Pickers test config
LukasTy Jun 4, 2025
4465504
Merge branch 'master' into try-a-browser-tests-fix
JCQuintas Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/x-date-pickers-pro/vitest.config.browser.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export default mergeConfig(sharedConfig, {
test: {
name: getTestName(import.meta.url),
environment: 'browser',
setupFiles: [new URL('../../test/utils/setupPickers.js', import.meta.url).pathname],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure, but I'd expect this could be broken on windows. Perhaps you want fileURLToPath?

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 were already using this pattern new URL() in other places. 🤔
Maybe @noraleonte could try running pnpm test:unit:browser to check this assumption? 🙏

Copy link
Member

@Janpot Janpot Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd say you always want to use fileURLToPath, even if it works for @noraleonte. Maybe not on your systems, but .pathname is going to give incorrect results. e.g. when someone has their project in a folder with a space or something.

Different case, but I remember someone having their projects in a folder with a dot and this breaking a script that assumes only paths for files can contain a dot. Naturally most people won't have this dot, so the issue remains invisible for a long time and will be hard to debug once it appears. In this case it even affected publishing of stable releases. Conclusion: always use the built-in functions for manipulating urls and paths, even if it looks trivial and seems to work. This is something you can be religious about in my experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

#18444

browser: {
enabled: true,
instances: [
Expand Down
1 change: 1 addition & 0 deletions packages/x-date-pickers-pro/vitest.config.jsdom.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ export default mergeConfig(sharedConfig, {
test: {
name: getTestName(import.meta.url),
environment: 'jsdom',
setupFiles: [new URL('../../test/utils/setupPickers.js', import.meta.url).pathname],
},
});
1 change: 1 addition & 0 deletions packages/x-date-pickers/vitest.config.browser.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default mergeConfig(sharedConfig, {
test: {
name: getTestName(import.meta.url),
environment: 'browser',
setupFiles: [new URL('../../test/utils/setupPickers.js', import.meta.url).pathname],
browser: {
enabled: true,
instances: [
Expand Down
1 change: 1 addition & 0 deletions packages/x-date-pickers/vitest.config.jsdom.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ export default mergeConfig(sharedConfig, {
test: {
name: getTestName(import.meta.url),
environment: 'jsdom',
setupFiles: [new URL('../../test/utils/setupPickers.js', import.meta.url).pathname],
},
});
5 changes: 1 addition & 4 deletions test/setupVitest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { beforeAll, afterAll } from 'vitest';
import 'test/utils/addChaiAssertions';
import 'test/utils/setupPickers';
import 'test/utils/licenseRelease';
import { generateTestLicenseKey, setupTestLicenseKey } from 'test/utils/testLicense';
import { configure } from '@mui/internal-test-utils';
Expand Down Expand Up @@ -58,10 +57,8 @@ if (!globalThis.after) {
(globalThis as any).after = afterAll;
}

const isJsdom = typeof window !== 'undefined' && window.navigator.userAgent.includes('jsdom');

// Only necessary when not in browser mode.
if (isJsdom) {
if (isJSDOM) {
class Touch {
instance: any;

Expand Down
7 changes: 7 additions & 0 deletions vitest.shared.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ export default defineConfig({
provider: 'playwright',
headless: true,
screenshotFailures: false,
orchestratorScripts: [
{
id: 'vitest-reload-on-error',
content: `window.addEventListener('vite:preloadError', (event) => { window.location.reload(); });`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Taken from https://vite.dev/guide/build#load-error-handling

async: true,
},
],
},
// Disable isolation to speed up the tests.
isolate: false,
Expand Down