-
Notifications
You must be signed in to change notification settings - Fork 27
[code-infra] Adopt @mui/internal-test-utils
#927
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
base: master
Are you sure you want to change the base?
Conversation
Bundle size report
Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces the @mui/internal-test-utils package to the code-infra repository, consolidating testing utilities to enable faster iteration and development. The package provides a comprehensive suite of test utilities including renderers, matchers, conformance testing, and helper functions for React component testing.
Key Changes
- New test-utils package with complete TypeScript and Vitest configuration
- Custom chai matchers for accessibility, focus, and style assertions
- Conformance testing framework for component consistency
- React Testing Library wrapper with enhanced functionality
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-utils/package.json | Package definition with dependencies and exports for the new test utilities |
| packages/test-utils/tsconfig.json | TypeScript configuration for the test-utils package |
| packages/test-utils/tsconfig.build.json | Build-specific TypeScript configuration |
| packages/test-utils/vitest.config.mts | Vitest test configuration with jsdom environment setup |
| packages/test-utils/src/index.ts | Main entry point exporting all test utilities |
| packages/test-utils/src/setupVitest.ts | Vitest setup with console error/warning suppression |
| packages/test-utils/src/setupVitestBrowser.ts | Browser environment setup with Touch API polyfill |
| packages/test-utils/src/initMatchers.ts | Initializes chai matchers for tests |
| packages/test-utils/src/initMatchers.test.js | Test suite for custom error/warning matchers |
| packages/test-utils/src/initPlaywrightMatchers.ts | Playwright-specific matchers for browser testing |
| packages/test-utils/src/chaiPlugin.ts | Custom chai assertions for accessibility and styling |
| packages/test-utils/src/chai.types.ts | TypeScript type definitions for custom chai matchers |
| packages/test-utils/src/createRenderer.tsx | Enhanced React Testing Library renderer with emotion cache |
| packages/test-utils/src/createRenderer.test.jsx | Tests for the custom renderer functionality |
| packages/test-utils/src/createDescribe.ts | Utility for creating describe blocks with skip/only support |
| packages/test-utils/src/describeConformance.tsx | Comprehensive conformance testing framework for components |
| packages/test-utils/src/components.tsx | Test helper components (ErrorBoundary, RenderCounter) |
| packages/test-utils/src/focusVisible.ts | Focus management utilities for testing |
| packages/test-utils/src/flushMicrotasks.ts | Async utility for flushing microtasks |
| packages/test-utils/src/fireDiscreteEvent.ts | Discrete event firing utilities for React 17/18 compatibility |
| packages/test-utils/src/env.ts | Environment detection utilities |
| packages/code-infra/src/eslint/material-ui/config.mjs | ESLint configuration update to support .jsx extension |
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
| @@ -0,0 +1,20 @@ | |||
| # On update, sync references where "#stable-snapshot" is mentioned in the codebase. | |||
| [stable] | |||
| baseline widely available | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed we were shipping regenerator runtime because of a missing browserslistrc.
@brijeshb42 Would it be possible to have a more modern default when browserslistrc is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add that, or fail directly asking to create a browserlistrc file if it doesn't exist. We shouldn't assume the supported browsers IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 4 comments.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
| emotion: false, | ||
| }; | ||
|
|
||
| export const config: Configuration = defaultConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be readonly?
| @@ -0,0 +1,13 @@ | |||
| export interface Configuration { | |||
| emotion: boolean; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document the usage of the prop?
| emotion: boolean; | |
| /** | |
| * The emotion wrapper is optional. | |
| * If you package uses emotion, install ... and set this to true. | |
| */ | |
| emotion: boolean; |
| skip: (...args: P) => void; | ||
| only: (...args: P) => void; | ||
| }; | ||
| export default <P extends any[]>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document this function so if someone who is thinking of using it understands possible drawbacks?
I understand it is used in many places in order to hide complexity. But it makes testing errors more opaque and harder to understand.
|
|
||
| export type ClockConfig = undefined | number | Date; | ||
|
|
||
| function createClock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In X I've moved all tests that used fake clock from this implementation to the actual vi.useFakeTimers/vi.vi.setSystemTime as this was creating a lot of unnecessary errors.
Since the createRenderer is usually called once for each file, it then became a flag that changed all the tests in a file to use fake timers or not.
When used in conjunction with createDescribe, which could have a lot of tests inside, it created a mess in which some components were being tested with fake timers, while other weren't.
I would be in favour of completely removing it as there are only about 18 uses in material-ui
| import * as ReactDOMServer from 'react-dom/server'; | ||
| import { useFakeTimers } from 'sinon'; | ||
| import { beforeEach, afterEach, beforeAll, vi } from 'vitest'; | ||
| import type { EmotionCache } from '@emotion/cache'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this causes issues if emotion is not installed?
| return originalFireEventKeyUp(element, options); | ||
| }; | ||
|
|
||
| export function fireTouchChangedEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot going on on this file that could be split into different files, but we can do it in a different pass
| * @param {() => void} callback | ||
| * @returns {void} | ||
| */ | ||
| function withMissingActWarningsIgnored(callback: () => void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we remove the implementation in this file altogether?
|
|
||
| chai.use(chaiPlugin); | ||
|
|
||
| if (failOnConsoleEnabed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
| if (failOnConsoleEnabed) { | |
| if (failOnConsoleEnabled) { |
Makes it possible to iterate faster on this module, the goal is to thin it out and remove unnecessary wrapper APIs.
Had to make a bunch of updates as well, most notable
emotionwrapper is now optional and can be initialized when calling setupVitest. This allows us to make these emotion peer dependencies optional toosetupVitestand call the function. We can't rely on module side effects because we useisolate:falseand that caches modules imported by the setup script between suitestoErrorDevto handle both sync and async functions. (There was a broken test in X relying on this)Integrations:
@mui/internal-test-utilsto code infra repo material-ui#47422