From 99227137d8de8556502a799b7ad8e2e5896c75ba Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Sat, 16 Nov 2019 10:59:49 +0100 Subject: [PATCH] [test] Improve coverage (#18385) --- .../src/SpeedDial/SpeedDial.js | 3 +- .../ToggleButtonGroup/ToggleButtonGroup.js | 3 +- packages/material-ui/scripts/rollup.config.js | 1 + .../src/BottomNavigation/BottomNavigation.js | 3 +- .../src/Breadcrumbs/Breadcrumbs.js | 3 +- .../src/ButtonGroup/ButtonGroup.js | 3 +- .../src/ExpansionPanel/ExpansionPanel.js | 3 +- .../src/ExpansionPanel/ExpansionPanel.test.js | 12 ++++-- packages/material-ui/src/GridList/GridList.js | 3 +- .../material-ui/src/GridList/GridList.test.js | 27 ++++++++++++ packages/material-ui/src/Menu/Menu.js | 3 +- packages/material-ui/src/Menu/Menu.test.js | 42 +++++++++++++++++++ packages/material-ui/src/MenuList/MenuList.js | 6 +-- packages/material-ui/src/Paper/Paper.js | 5 +-- packages/material-ui/src/Paper/Paper.test.js | 32 ++++++++++++++ .../material-ui/src/Select/SelectInput.js | 3 +- packages/material-ui/src/Step/Step.js | 3 +- packages/material-ui/src/Tabs/Tabs.js | 3 +- .../src/styles/createTypography.js | 4 +- .../src/styles/createTypography.test.js | 31 ++++++++++++++ 20 files changed, 168 insertions(+), 25 deletions(-) diff --git a/packages/material-ui-lab/src/SpeedDial/SpeedDial.js b/packages/material-ui-lab/src/SpeedDial/SpeedDial.js index 104ddf0ec7c3de..e2cd10f514d9b2 100644 --- a/packages/material-ui-lab/src/SpeedDial/SpeedDial.js +++ b/packages/material-ui-lab/src/SpeedDial/SpeedDial.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import { duration, withStyles } from '@material-ui/core/styles'; @@ -280,7 +281,7 @@ const SpeedDial = React.forwardRef(function SpeedDial(props, ref) { const allItems = React.Children.toArray(childrenProp).filter(child => { if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the SpeedDial component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui-lab/src/ToggleButtonGroup/ToggleButtonGroup.js b/packages/material-ui-lab/src/ToggleButtonGroup/ToggleButtonGroup.js index 10733329d58126..64a29b9b943dbc 100644 --- a/packages/material-ui-lab/src/ToggleButtonGroup/ToggleButtonGroup.js +++ b/packages/material-ui-lab/src/ToggleButtonGroup/ToggleButtonGroup.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import isValueSelected from './isValueSelected'; @@ -82,7 +83,7 @@ const ToggleButtonGroup = React.forwardRef(function ToggleButton(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the ToggleButtonGroup component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/scripts/rollup.config.js b/packages/material-ui/scripts/rollup.config.js index c39739f82c1463..ca0b09f215f89d 100644 --- a/packages/material-ui/scripts/rollup.config.js +++ b/packages/material-ui/scripts/rollup.config.js @@ -31,6 +31,7 @@ const commonjsOptions = { ], '../../node_modules/react-is/index.js': [ 'ForwardRef', + 'isFragment', 'isLazy', 'isMemo', 'isValidElementType', diff --git a/packages/material-ui/src/BottomNavigation/BottomNavigation.js b/packages/material-ui/src/BottomNavigation/BottomNavigation.js index c5e3f5b2332083..b76a1ff0fc2f8f 100755 --- a/packages/material-ui/src/BottomNavigation/BottomNavigation.js +++ b/packages/material-ui/src/BottomNavigation/BottomNavigation.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; @@ -33,7 +34,7 @@ const BottomNavigation = React.forwardRef(function BottomNavigation(props, ref) } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the BottomNavigation component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js index c3ba9a1607421a..18ea6a953b5ed1 100644 --- a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js +++ b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; @@ -86,7 +87,7 @@ const Breadcrumbs = React.forwardRef(function Breadcrumbs(props, ref) { const allItems = React.Children.toArray(children) .filter(child => { if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Breadcrumbs component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/ButtonGroup/ButtonGroup.js b/packages/material-ui/src/ButtonGroup/ButtonGroup.js index 38e3aec6b23d6d..9fb122ae6a4523 100644 --- a/packages/material-ui/src/ButtonGroup/ButtonGroup.js +++ b/packages/material-ui/src/ButtonGroup/ButtonGroup.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import capitalize from '../utils/capitalize'; @@ -148,7 +149,7 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the ButtonGroup component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js index 7e9898b8ba5bac..f770900ea614cf 100644 --- a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js +++ b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import { chainPropTypes } from '@material-ui/utils'; @@ -169,7 +170,7 @@ ExpansionPanel.propTypes = { */ children: chainPropTypes(PropTypes.node.isRequired, props => { const summary = React.Children.toArray(props.children)[0]; - if (summary.type === React.Fragment) { + if (isFragment(summary)) { return new Error( "Material-UI: the ExpansionPanel doesn't accept a Fragment as a child. " + 'Consider providing an array instead.', diff --git a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js index c5056cb2a1494d..bf5c3f3395b7b8 100644 --- a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js +++ b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js @@ -156,13 +156,17 @@ describe('', () => { PropTypes.resetWarningCache(); }); - /* works locally but doesn't catch the errors in test:karma it('requires at least one child', () => { - assert.throws(() => mount([])); - // 2 other errors are from accesing prop of undefined and react component stack + if (!/jsdom/.test(window.navigator.userAgent)) { + // errors during mount are not caught by try-catch in the browser + // can't use skip since this causes multiple errors related to cleanup of the console mock + return; + } + + assert.throws(() => mount({[]})); assert.strictEqual(consoleErrorMock.callCount(), 3); assert.include(consoleErrorMock.args()[0][0], 'Material-UI: expected the first child'); - }); */ + }); it('needs a valid element as the first child', () => { mount( diff --git a/packages/material-ui/src/GridList/GridList.js b/packages/material-ui/src/GridList/GridList.js index efb3f5b23728be..a44a7ee71e4989 100644 --- a/packages/material-ui/src/GridList/GridList.js +++ b/packages/material-ui/src/GridList/GridList.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; @@ -41,7 +42,7 @@ const GridList = React.forwardRef(function GridList(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the GridList component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/GridList/GridList.test.js b/packages/material-ui/src/GridList/GridList.test.js index 308c55f263f713..8d8f91a9068bfd 100644 --- a/packages/material-ui/src/GridList/GridList.test.js +++ b/packages/material-ui/src/GridList/GridList.test.js @@ -3,6 +3,8 @@ import { assert } from 'chai'; import { createMount, createShallow, getClasses } from '@material-ui/core/test-utils'; import describeConformance from '../test-utils/describeConformance'; import GridList from './GridList'; +import consoleErrorMock from 'test/utils/consoleErrorMock'; +import PropTypes from 'prop-types'; const tilesData = [ { @@ -186,4 +188,29 @@ describe('', () => { ); }); }); + + describe('warnings', () => { + before(() => { + consoleErrorMock.spy(); + }); + + after(() => { + consoleErrorMock.reset(); + PropTypes.resetWarningCache(); + }); + + it('warns a Fragment is passed as a child', () => { + mount( + + + , + ); + + assert.strictEqual(consoleErrorMock.callCount(), 1); + assert.include( + consoleErrorMock.args()[0][0], + "Material-UI: the GridList component doesn't accept a Fragment as a child.", + ); + }); + }); }); diff --git a/packages/material-ui/src/Menu/Menu.js b/packages/material-ui/src/Menu/Menu.js index 1a5a270f42cb6f..628674ceb2a2b8 100644 --- a/packages/material-ui/src/Menu/Menu.js +++ b/packages/material-ui/src/Menu/Menu.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; @@ -95,7 +96,7 @@ const Menu = React.forwardRef(function Menu(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Menu component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/Menu/Menu.test.js b/packages/material-ui/src/Menu/Menu.test.js index a6991f81e077b1..aa89638981f1fe 100644 --- a/packages/material-ui/src/Menu/Menu.test.js +++ b/packages/material-ui/src/Menu/Menu.test.js @@ -6,6 +6,8 @@ import describeConformance from '../test-utils/describeConformance'; import Popover from '../Popover'; import Menu from './Menu'; import MenuList from '../MenuList'; +import consoleErrorMock from 'test/utils/consoleErrorMock'; +import PropTypes from 'prop-types'; const MENU_LIST_HEIGHT = 100; @@ -212,4 +214,44 @@ describe('', () => { assert.strictEqual(onCloseSpy.callCount, 1); assert.strictEqual(onCloseSpy.args[0][1], 'tabKeyDown'); }); + + it('ignores invalid children', () => { + const wrapper = mount( + + {null} + hello + {/* testing conditional rendering */} + {false && hello} + {undefined} + foo + , + ); + + assert.lengthOf(wrapper.find('span[role="menuitem"]'), 1); + }); + + describe('warnings', () => { + before(() => { + consoleErrorMock.spy(); + }); + + after(() => { + consoleErrorMock.reset(); + PropTypes.resetWarningCache(); + }); + + it('warns a Fragment is passed as a child', () => { + mount( + + + , + ); + + assert.strictEqual(consoleErrorMock.callCount(), 2); + assert.include( + consoleErrorMock.args()[0][0], + "Material-UI: the Menu component doesn't accept a Fragment as a child.", + ); + }); + }); }); diff --git a/packages/material-ui/src/MenuList/MenuList.js b/packages/material-ui/src/MenuList/MenuList.js index 5183c1d20680db..79611a35dfde8e 100644 --- a/packages/material-ui/src/MenuList/MenuList.js +++ b/packages/material-ui/src/MenuList/MenuList.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import ReactDOM from 'react-dom'; import ownerDocument from '../utils/ownerDocument'; @@ -35,9 +36,6 @@ function textCriteriaMatches(nextFocus, textCriteria) { // jsdom doesn't support innerText text = nextFocus.textContent; } - if (text === undefined) { - return false; - } text = text.trim().toLowerCase(); if (text.length === 0) { return false; @@ -209,7 +207,7 @@ const MenuList = React.forwardRef(function MenuList(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Menu component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js index cf6c4126071498..9b1a9ee36c877f 100644 --- a/packages/material-ui/src/Paper/Paper.js +++ b/packages/material-ui/src/Paper/Paper.js @@ -2,7 +2,6 @@ import React from 'react'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; -import useTheme from '../styles/useTheme'; export const styles = theme => { const elevations = {}; @@ -38,9 +37,7 @@ const Paper = React.forwardRef(function Paper(props, ref) { } = props; if (process.env.NODE_ENV !== 'production') { - // eslint-disable-next-line react-hooks/rules-of-hooks - const theme = useTheme(); - if (!theme.shadows[elevation]) { + if (classes[`elevation${elevation}`] === undefined) { console.error(`Material-UI: this elevation \`${elevation}\` is not implemented.`); } } diff --git a/packages/material-ui/src/Paper/Paper.test.js b/packages/material-ui/src/Paper/Paper.test.js index 48b3079ca55e8f..a75f278bc63c60 100644 --- a/packages/material-ui/src/Paper/Paper.test.js +++ b/packages/material-ui/src/Paper/Paper.test.js @@ -3,12 +3,22 @@ import { assert } from 'chai'; import { createMount, createShallow, getClasses } from '@material-ui/core/test-utils'; import describeConformance from '../test-utils/describeConformance'; import Paper from './Paper'; +import { createMuiTheme, ThemeProvider } from '../styles'; +import consoleErrorMock from 'test/utils/consoleErrorMock'; describe('', () => { let mount; let shallow; let classes; + beforeEach(() => { + consoleErrorMock.spy(); + }); + + afterEach(() => { + consoleErrorMock.reset(); + }); + before(() => { mount = createMount({ strict: true }); shallow = createShallow({ dive: true }); @@ -59,4 +69,26 @@ describe('', () => { 'should have the 2 elevation class', ); }); + + it('warns if the given `elevation` is not implemented in the theme', () => { + mount(); + + assert.strictEqual(consoleErrorMock.callCount(), 1); + assert.include( + consoleErrorMock.args()[0][0], + 'Material-UI: this elevation `25` is not implemented.', + ); + }); + + it('allows custom elevations via theme.shadows', () => { + const theme = createMuiTheme(); + theme.shadows.push('20px 20px'); + const wrapper = mount( + + + , + ); + + assert.strictEqual(wrapper.find('div[data-testid="paper"]').hasClass('custom-elevation'), true); + }); }); diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js index e4496972bd267e..080cfbd675dfc4 100644 --- a/packages/material-ui/src/Select/SelectInput.js +++ b/packages/material-ui/src/Select/SelectInput.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import capitalize from '../utils/capitalize'; @@ -214,7 +215,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Select component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/Step/Step.js b/packages/material-ui/src/Step/Step.js index c3a5ab22929d38..5b362a2a4e9a45 100644 --- a/packages/material-ui/src/Step/Step.js +++ b/packages/material-ui/src/Step/Step.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import withStyles from '../styles/withStyles'; @@ -69,7 +70,7 @@ const Step = React.forwardRef(function Step(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Step component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/Tabs/Tabs.js b/packages/material-ui/src/Tabs/Tabs.js index 8134e21b662c8c..b4c64c0375579a 100644 --- a/packages/material-ui/src/Tabs/Tabs.js +++ b/packages/material-ui/src/Tabs/Tabs.js @@ -1,4 +1,5 @@ import React from 'react'; +import { isFragment } from 'react-is'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import { refType } from '@material-ui/utils'; @@ -383,7 +384,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) { } if (process.env.NODE_ENV !== 'production') { - if (child.type === React.Fragment) { + if (isFragment(child)) { console.error( [ "Material-UI: the Tabs component doesn't accept a Fragment as a child.", diff --git a/packages/material-ui/src/styles/createTypography.js b/packages/material-ui/src/styles/createTypography.js index 2e52cfaddeb495..33ab4821cd61c8 100644 --- a/packages/material-ui/src/styles/createTypography.js +++ b/packages/material-ui/src/styles/createTypography.js @@ -33,11 +33,11 @@ export default function createTypography(palette, typography) { if (process.env.NODE_ENV !== 'production') { if (typeof fontSize !== 'number') { - console.error(`Material-UI: 'fontSize' is required to be a number.`); + console.error('Material-UI: `fontSize` is required to be a number.'); } if (typeof htmlFontSize !== 'number') { - console.error(`Material-UI: 'htmlFontSize' is required to be a number.`); + console.error('Material-UI: `htmlFontSize` is required to be a number.'); } } diff --git a/packages/material-ui/src/styles/createTypography.test.js b/packages/material-ui/src/styles/createTypography.test.js index e2fdeaae807b0e..60f6688eb93590 100644 --- a/packages/material-ui/src/styles/createTypography.test.js +++ b/packages/material-ui/src/styles/createTypography.test.js @@ -1,6 +1,7 @@ import { assert } from 'chai'; import createPalette from './createPalette'; import createTypography from './createTypography'; +import consoleErrorMock from 'test/utils/consoleErrorMock'; describe('createTypography', () => { let palette; @@ -71,4 +72,34 @@ describe('createTypography', () => { assert.isDefined(createTypography(palette, {}).h1.letterSpacing); assert.isUndefined(createTypography(palette, { fontFamily: 'Gotham' }).h1.letterSpacing); }); + + describe('warnings', () => { + beforeEach(() => { + consoleErrorMock.spy(); + }); + + afterEach(() => { + consoleErrorMock.reset(); + }); + + it('logs an error if `fontSize` is not of type number', () => { + createTypography({}, { fontSize: '1' }); + + assert.strictEqual(consoleErrorMock.callCount(), 1); + assert.match( + consoleErrorMock.args()[0][0], + /Material-UI: `fontSize` is required to be a number./, + ); + }); + + it('logs an error if `htmlFontSize` is not of type number', () => { + createTypography({}, { htmlFontSize: '1' }); + + assert.strictEqual(consoleErrorMock.callCount(), 1); + assert.match( + consoleErrorMock.args()[0][0], + /Material-UI: `htmlFontSize` is required to be a number./, + ); + }); + }); });