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

[core] Replace enzyme in describeConformance #42447

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion docs/pages/joy-ui/api/tooltip.json
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@
"isGlobal": false
}
],
"spread": true,
"spread": false,
"themeDefaultProps": true,
"muiName": "JoyTooltip",
"forwardsRefTo": "HTMLButtonElement",
Expand Down
2 changes: 1 addition & 1 deletion docs/pages/material-ui/api/switch.json
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
"isGlobal": false
}
],
"spread": false,
"spread": true,
"themeDefaultProps": false,
"muiName": "MuiSwitch",
"forwardsRefTo": "HTMLSpanElement",
Expand Down
160 changes: 66 additions & 94 deletions packages-internal/test-utils/src/describeConformance.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
/* eslint-env mocha */
import * as React from 'react';
import { expect } from 'chai';
import { ReactWrapper } from 'enzyme';
import ReactTestRenderer from 'react-test-renderer';
import createMount from './createMount';
import createDescribe from './createDescribe';
import findOutermostIntrinsic from './findOutermostIntrinsic';
import { MuiRenderResult } from './createRenderer';

function capitalize(string: string): string {
Expand Down Expand Up @@ -46,53 +43,30 @@ export interface ConformanceOptions {
after?: () => void;
inheritComponent?: React.ElementType;
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
render: (node: React.ReactElement<any>) => MuiRenderResult;
mount?: (node: React.ReactElement<any>) => ReactWrapper;
only?: Array<keyof typeof fullSuite>;
skip?: Array<keyof typeof fullSuite | 'classesRoot'>;
testComponentsRootPropWith?: string;
/**
* A custom React component to test if the component prop is implemented correctly.
*
* It must either:
* - Be a string that is a valid HTML tag, or
* - A component that spread props to the underlying rendered element.
*
* If not provided, the default 'em' element is used.
*/
testComponentPropWith?: string | React.ElementType;
testDeepOverrides?: SlotTestOverride | SlotTestOverride[];
testRootOverrides?: SlotTestOverride;
testStateOverrides?: { prop?: string; value?: any; styleKey: string };
testCustomVariant?: boolean;
testVariantProps?: object;
testLegacyComponentsProp?: boolean;
wrapMount?: (
mount: (node: React.ReactElement<any>) => ReactWrapper,
) => (node: React.ReactElement<any>) => ReactWrapper;
slots?: Record<string, SlotTestingOptions>;
ThemeProvider?: React.ElementType;
createTheme?: (arg: any) => any;
}

/**
* @param {object} node
* @returns
*/
function assertDOMNode(node: unknown) {
// duck typing a DOM node
expect(typeof (node as HTMLElement).nodeName).to.equal('string');
}

/**
* Utility method to make assertions about the ref on an element
* The element should have a component wrapped in withStyles as the root
*/
function testRef(
element: React.ReactElement<any>,
mount: ConformanceOptions['mount'],
onRef: (instance: unknown, wrapper: import('enzyme').ReactWrapper) => void = assertDOMNode,
) {
if (!mount) {
throwMissingPropError('mount');
}

const ref = React.createRef();

const wrapper = mount(<React.Fragment>{React.cloneElement(element, { ref })}</React.Fragment>);
onRef(ref.current, wrapper);
}

/**
* Glossary
* - root component:
Expand All @@ -102,18 +76,6 @@ function testRef(
* - has the type of `inheritComponent`
*/

/**
* Returns the component with the same constructor as `component` that renders
* the outermost host
*/
export function findRootComponent(wrapper: ReactWrapper, component: string | React.ElementType) {
const outermostHostElement = findOutermostIntrinsic(wrapper).getElement();

return wrapper.find(component as string).filterWhere((componentWrapper) => {
return componentWrapper.contains(outermostHostElement);
});
}

export function randomStringValue() {
return `s${Math.random().toString(36).slice(2)}`;
}
Expand All @@ -134,16 +96,20 @@ export function testClassName(
getOptions: () => ConformanceOptions,
) {
it('applies the className to the root component', () => {
const { mount } = getOptions();
if (!mount) {
throwMissingPropError('mount');
const { render } = getOptions();

if (!render) {
throwMissingPropError('render');
}

const className = randomStringValue();
const testId = randomStringValue();

const wrapper = mount(React.cloneElement(element, { className }));
const { getByTestId } = render(
React.cloneElement(element, { className, 'data-testid': testId }),
aarongarciah marked this conversation as resolved.
Show resolved Hide resolved
);

expect(findOutermostIntrinsic(wrapper).instance()).to.have.class(className);
expect(getByTestId(testId)).to.have.class(className);
});
}

Expand All @@ -157,45 +123,57 @@ export function testComponentProp(
) {
describe('prop: component', () => {
it('can render another root component with the `component` prop', () => {
const { mount, testComponentPropWith: component = 'em' } = getOptions();
if (!mount) {
throwMissingPropError('mount');
const { render, testComponentPropWith: component = 'em' } = getOptions();
if (!render) {
throwMissingPropError('render');
}

const wrapper = mount(React.cloneElement(element, { component }));
const testId = randomStringValue();

expect(findRootComponent(wrapper, component).exists()).to.equal(true);
if (typeof component === 'string') {
const { getByTestId } = render(
React.cloneElement(element, { component, 'data-testid': testId }),
);
expect(getByTestId(testId)).not.to.equal(null);
expect(getByTestId(testId).nodeName.toLowerCase()).to.eq(component);
} else {
const componentWithTestId = (props: {}) =>
React.createElement(component, { ...props, 'data-testid': testId });
const { getByTestId } = render(
React.cloneElement(element, {
component: componentWithTestId,
}),
);
expect(getByTestId(testId)).not.to.equal(null);
}
});
});
}

/**
* MUI components can spread additional props to a documented component.
* MUI components spread additional props to its root.
*/
export function testPropsSpread(
element: React.ReactElement<any>,
getOptions: () => ConformanceOptions,
) {
it(`spreads props to the root component`, () => {
// type def in ConformanceOptions
const { inheritComponent, mount } = getOptions();
if (!mount) {
throwMissingPropError('mount');
}
const { render } = getOptions();

if (inheritComponent === undefined) {
throw new TypeError(
'Unable to test props spread without `inheritComponent`. Either skip the test or pass a React element type.',
);
if (!render) {
throwMissingPropError('render');
}

const testProp = 'data-test-props-spread';
const value = randomStringValue();
const testId = randomStringValue();

const wrapper = mount(React.cloneElement(element, { [testProp]: value }));
const root = findRootComponent(wrapper, inheritComponent);
const { getByTestId } = render(
React.cloneElement(element, { [testProp]: value, 'data-testid': testId }),
);

expect(root.props()).to.have.property(testProp, value);
expect(getByTestId(testId)).to.have.attribute(testProp, value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, we lose the aspect of testing that the inherited component is being used and only maintain that the props are spread into whatever the component considers its root.

This follows RTL's principle of not testing implementation details. There might be a way for us to test that the inherited component is the one used, but I think it would be better to test that on each component's tests, by testing the expected functionality, classes, or whatever the inherited component provides.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this is mostly used by the doc gen to add a message about additional props available on a component. I'm strongly in favor of removing this message and listing all available props on each component, so it's not necessary to jump between the pages to learn about available props. Afterwards, this test may be unnecessary.

Not in this PR, of course :)

});
}

Expand All @@ -212,16 +190,17 @@ export function describeRef(
describe('ref', () => {
it(`attaches the ref`, () => {
// type def in ConformanceOptions
const { inheritComponent, mount, refInstanceof } = getOptions();
const { render, refInstanceof } = getOptions();

if (!render) {
throwMissingPropError('render');
}

testRef(element, mount, (instance, wrapper) => {
expect(instance).to.be.instanceof(refInstanceof);
const ref = React.createRef();

if (inheritComponent !== undefined && (instance as HTMLElement).nodeType === 1) {
const rootHost = findOutermostIntrinsic(wrapper);
expect(instance).to.equal(rootHost.instance());
}
});
render(React.cloneElement(element, { ref }));

expect(ref.current).to.be.instanceof(refInstanceof);
});
});
}
Expand Down Expand Up @@ -589,14 +568,18 @@ function testComponentsProp(
) {
describe('prop components:', () => {
it('can render another root component with the `components` prop', () => {
const { mount, testComponentsRootPropWith: component = 'em' } = getOptions();
if (!mount) {
throwMissingPropError('mount');
const { render, testComponentsRootPropWith: component = 'em' } = getOptions();
if (!render) {
throwMissingPropError('render');
}

const wrapper = mount(React.cloneElement(element, { components: { Root: component } }));
const testId = randomStringValue();

expect(findRootComponent(wrapper, component).exists()).to.equal(true);
const { getByTestId } = render(
React.cloneElement(element, { components: { Root: component }, 'data-testid': testId }),
);
expect(getByTestId(testId)).not.to.equal(null);
expect(getByTestId(testId).nodeName.toLowerCase()).to.eq(component);
});
});
}
Expand Down Expand Up @@ -1081,7 +1064,6 @@ function describeConformance(
only = Object.keys(fullSuite),
slots,
skip = [],
wrapMount,
} = getOptions();

let filteredTests = Object.keys(fullSuite).filter(
Expand All @@ -1096,21 +1078,11 @@ function describeConformance(
filteredTests = filteredTests.filter((testKey) => !slotBasedTests.includes(testKey));
}

const baseMount = createMount();
const mount = wrapMount !== undefined ? wrapMount(baseMount) : baseMount;

after(runAfterHook);

function getTestOptions(): ConformanceOptions {
return {
...getOptions(),
mount,
};
}

filteredTests.forEach((testKey) => {
const test = fullSuite[testKey];
test(minimalElement, getTestOptions);
test(minimalElement, getOptions);
});
}

Expand Down
20 changes: 19 additions & 1 deletion packages/mui-base/test/describeConformanceUnstyled.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
SlotTestingOptions,
describeRef,
randomStringValue,
testClassName,
testComponentProp,
testReactTestRenderer,
} from '@mui/internal-test-utils';
Expand Down Expand Up @@ -269,6 +268,25 @@ function testSlotPropsProp(
});
}

function testClassName(element: React.ReactElement, getOptions: () => ConformanceOptions) {
Copy link
Member Author

@DiegoAndai DiegoAndai May 30, 2024

Choose a reason for hiding this comment

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

This was required as Base UI's describeConformance accepts async render functions. We could support those in the basic describeConformance test, but I wanted to keep the changes minimal, especially considering this base code is stale. Let me know if making the original testClassName async would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Async render functions will be required when Material UI uses Floating UI. Certain interactions require flushing the microtask queue, which is an async operation. This is not strictly required now, so I agree it's ok to minimize the number of changes in this PR.

it('applies the className to the root component', async () => {
const { render } = getOptions();

if (!render) {
throwMissingPropError('render');
}

const className = randomStringValue();
const testId = randomStringValue();

const { getByTestId } = await render(
React.cloneElement(element, { className, 'data-testid': testId }),
);

expect(getByTestId(testId)).to.have.class(className);
});
}

interface TestOwnerState {
'data-testid'?: string;
}
Expand Down
11 changes: 8 additions & 3 deletions packages/mui-joy/src/Autocomplete/Autocomplete.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -632,9 +632,14 @@ const Autocomplete = React.forwardRef(function Autocomplete(
},
});

const defaultRenderOption = (optionProps: any, option: unknown) => (
<SlotOption {...optionProps}>{getOptionLabel(option)}</SlotOption>
);
const defaultRenderOption = (optionProps: any, option: unknown) => {
const { key, ...rest } = optionProps;
return (
<SlotOption key={key} {...rest}>
{getOptionLabel(option)}
</SlotOption>
);
};

const renderOption = renderOptionProp || defaultRenderOption;

Expand Down
6 changes: 0 additions & 6 deletions packages/mui-joy/src/Menu/Menu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,6 @@ describe('Joy <Menu />', () => {
<DropdownContext.Provider value={testContext}>{node}</DropdownContext.Provider>,
);
},
wrapMount: (mount) => (node: React.ReactNode) => {
const wrapper = mount(
<DropdownContext.Provider value={testContext}>{node}</DropdownContext.Provider>,
);
return wrapper.childAt(0);
},
ThemeProvider,
muiName: 'JoyMenu',
refInstanceof: window.HTMLUListElement,
Expand Down
6 changes: 0 additions & 6 deletions packages/mui-joy/src/MenuButton/MenuButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ describe('<MenuButton />', () => {
describeConformance(<MenuButton />, () => ({
classes,
inheritComponent: 'button',
wrapMount: (mount) => (node: React.ReactNode) => {
const wrapper = mount(
<DropdownContext.Provider value={testContext}>{node}</DropdownContext.Provider>,
);
return wrapper.childAt(0);
},
muiName: 'JoyMenuButton',
refInstanceof: window.HTMLButtonElement,
render: (node) => {
Expand Down
4 changes: 0 additions & 4 deletions packages/mui-joy/src/MenuItem/MenuItem.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ describe('Joy <MenuItem />', () => {
classes,
inheritComponent: ListItemButton,
render: (node) => render(<MenuProvider value={testContext}>{node}</MenuProvider>),
wrapMount: (mount) => (node) => {
const wrapper = mount(<MenuProvider value={testContext}>{node}</MenuProvider>);
return wrapper.childAt(0);
},
ThemeProvider,
refInstanceof: window.HTMLLIElement,
testComponentPropWith: 'a',
Expand Down
1 change: 0 additions & 1 deletion packages/mui-joy/src/Tab/Tab.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ describe('Joy <Tab />', () => {
classes,
inheritComponent: 'button',
render: (node) => render(<TabsProvider defaultValue={0}>{node}</TabsProvider>),
wrapMount: (mount) => (node) => mount(<TabsProvider defaultValue={0}>{node}</TabsProvider>),
ThemeProvider,
muiName: 'JoyTab',
refInstanceof: window.HTMLButtonElement,
Expand Down
1 change: 0 additions & 1 deletion packages/mui-joy/src/TabList/TabList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ describe('Joy <TabList />', () => {
classes,
inheritComponent: 'div',
render: (node) => render(<TabsProvider defaultValue={0}>{node}</TabsProvider>),
wrapMount: (mount) => (node) => mount(<TabsProvider defaultValue={0}>{node}</TabsProvider>),
ThemeProvider,
muiName: 'JoyTabList',
refInstanceof: window.HTMLDivElement,
Expand Down
Loading
Loading