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.

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

[test] Convert SwipeableDrawer tests to testing-library #26916

Merged
merged 4 commits into from
Jun 25, 2021
Merged

[test] Convert SwipeableDrawer tests to testing-library #26916

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19d7bf9
[test] Convert SwipeableDrawer tests to testing-library
eps1lon Jun 23, 2021
3d5b0f5
StrictMode comaptible
eps1lon Jun 23, 2021
08c989b
Create actual Touch objects
eps1lon Jun 24, 2021
0e4b072
Match browser event order
eps1lon Jun 24, 2021
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
123 changes: 42 additions & 81 deletions packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import * as React from 'react';
import { expect } from 'chai';
import { spy } from 'sinon';
import { spy, useFakeTimers } from 'sinon';
import {
act,
createMount,
fireEvent,
createClientRender,
Expand All @@ -13,45 +12,8 @@ import PropTypes, { checkPropTypes } from 'prop-types';
import SwipeableDrawer from '@material-ui/core/SwipeableDrawer';
import Drawer, { drawerClasses } from '@material-ui/core/Drawer';
import { backdropClasses } from '@material-ui/core/Backdrop';
import SwipeArea from './SwipeArea';
import useForkRef from '../utils/useForkRef';

function fireMouseEvent(name, element, properties = {}) {
act(() => {
const event = document.createEvent('MouseEvents');
event.initEvent(name, true, true);
Object.keys(properties).forEach((key) => {
event[key] = properties[key];
});
if (element.dispatchEvent) {
element.dispatchEvent(event);
} else {
element.getDOMNode().dispatchEvent(event);
}
});
}

function fireBodyMouseEvent(name, properties = {}) {
fireMouseEvent(name, document.body, properties);
}

function fireSwipeAreaMouseEvent(wrapper, name, properties = {}) {
let event;
act(() => {
event = document.createEvent('MouseEvents');
event.initEvent(name, true, true);
Object.keys(properties).forEach((key) => {
event[key] = properties[key];
});
const swipeArea = wrapper.find(SwipeArea);
if (swipeArea.length >= 1) {
// if no SwipeArea is mounted, the body event wouldn't propagate to it anyway
swipeArea.getDOMNode().dispatchEvent(event);
}
});
return event;
}

const FakePaper = React.forwardRef(function FakeWidthPaper(props, ref) {
const { style, ...other } = props;
const paperRef = React.useRef(null);
Expand Down Expand Up @@ -102,9 +64,19 @@ const NullPaper = React.forwardRef(function NullPaper(props, ref) {
});

describe('<SwipeableDrawer />', () => {
// test are mostly asserting on implementation details
const mount = createMount({ strict: null });
const render = createClientRender({ strict: false });
/**
* @type {ReturnType<typeof useFakeTimers>}
*/
let clock;
beforeEach(() => {
clock = useFakeTimers();
});
afterEach(() => {
clock.restore();
});

const mount = createMount();
const render = createClientRender();

describeConformance(<SwipeableDrawer onOpen={() => {}} onClose={() => {}} open />, () => ({
classes: {},
Expand Down Expand Up @@ -474,8 +446,9 @@ describe('<SwipeableDrawer />', () => {
describe('disableSwipeToOpen', () => {
it('should not support swipe to open if disableSwipeToOpen is set', () => {
const handleOpen = spy();
const wrapper = mount(
render(
<SwipeableDrawer
disableSwipeToOpen
onOpen={handleOpen}
onClose={() => {}}
open={false}
Expand All @@ -485,53 +458,48 @@ describe('<SwipeableDrawer />', () => {
</SwipeableDrawer>,
);

// simulate open swipe
wrapper.setProps({ disableSwipeToOpen: true });
expect(wrapper.find('[role="presentation"]').exists()).to.equal(false);
fireBodyMouseEvent('touchstart', { touches: [{ pageX: 10, clientY: 0 }] });
fireBodyMouseEvent('touchmove', { touches: [{ pageX: 150, clientY: 0 }] });
fireBodyMouseEvent('touchend', { changedTouches: [{ pageX: 250, clientY: 0 }] });
fireEvent.touchStart(document.body, { touches: [{ pageX: 10, clientY: 0 }] });
fireEvent.touchMove(document.body, { touches: [{ pageX: 150, clientY: 0 }] });
fireEvent.touchEnd(document.body, { changedTouches: [{ pageX: 250, clientY: 0 }] });

expect(handleOpen.callCount).to.equal(0);
expect(wrapper.find('[role="presentation"]').exists()).to.equal(false);
wrapper.unmount();
});

it('should support swipe to close if disableSwipeToOpen is set', () => {
const handleClose = spy();
const wrapper = mount(
render(
<SwipeableDrawer
disableSwipeToOpen
onOpen={() => {}}
onClose={handleClose}
open
PaperProps={{ component: FakePaper }}
PaperProps={{ component: FakePaper, 'data-testid': 'paper' }}
>
<div>SwipeableDrawer</div>
</SwipeableDrawer>,
);

// simulate close swipe
wrapper.setProps({ disableSwipeToOpen: true });
expect(wrapper.find('[role="presentation"]').exists()).to.equal(true);
fireMouseEvent('touchstart', wrapper.find(FakePaper), {
fireEvent.touchStart(screen.getByTestId('paper'), {
touches: [{ pageX: 250, clientY: 0 }],
});
fireBodyMouseEvent('touchmove', { touches: [{ pageX: 150, clientY: 0 }] });
fireBodyMouseEvent('touchend', { changedTouches: [{ pageX: 10, clientY: 0 }] });
fireEvent.touchMove(document.body, { touches: [{ pageX: 150, clientY: 0 }] });
fireEvent.touchEnd(document.body, { changedTouches: [{ pageX: 10, clientY: 0 }] });

expect(handleClose.callCount).to.equal(1);
wrapper.unmount();
});
});

describe('lock', () => {
it('should handle a single swipe at the time', () => {
const handleOpen = spy();
const wrapper = mount(
render(
<div>
<SwipeableDrawer
onOpen={handleOpen}
onClose={() => {}}
open={false}
PaperProps={{ component: FakePaper }}
SwipeAreaProps={{ 'data-testid': 'swipearea' }}
>
<div>Drawer1</div>
</SwipeableDrawer>
Expand All @@ -540,47 +508,40 @@ describe('<SwipeableDrawer />', () => {
onClose={() => {}}
open={false}
PaperProps={{ component: FakePaper }}
SwipeAreaProps={{ 'data-testid': 'swipearea' }}
>
<div>Drawer2</div>
</SwipeableDrawer>
</div>,
);

// use the same event object for both touch start events, one would propagate to the other swipe area in the browser
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to do some manual testing to understand what was replayed here.

  1. events are never re-used
  2. if this is about propagating then the DOM takes care of it (not dispatching multiple times). The original idea was probably replaying about hit detection

Copy link
Member Author

@eps1lon eps1lon Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Verified in https://codesandbox.io/s/single-swipearea-lock-ksyss (monitorEvents(document.body, ['touchstart', 'touchmove', 'touchend'])) that the way the test is setup a touchstart would only be fired at the last rendered SwipeableDrawer. The SwipeAreas are stacked on top of each other and hit detection never "pierces" elements i.e. if a touch hits an element then the event is targetted at that element not at all the ones the touch pierces. Then the event bubbles up but since the SwipeAreas are siblings, only one registers the touchstart.

I also remembered that subsequent touchmove and touchend target the same element as the touchstart.

But I don't know how I could get this test to fail. I thought https://github.com/mui-org/material-ui/blob/5f882fefadbdd72e950a4940a550c1d3287dd150/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L300-L303 was responsible for this behavior but removing the early return does not fail any test.

@oliviertassinari Do you know what implementation this test was ensuring or how one would test https://github.com/mui-org/material-ui/blob/5f882fefadbdd72e950a4940a550c1d3287dd150/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L300-L303

Also please review the full test diff (https://github.com/mui-org/material-ui/pull/26916/files#diff-2e95d46f49f995c596826a9b8b12a84dc9de890ac9ffb65d208dd2fc23b2ccbeR508-R551) to make sure I didn't miss something.

Copy link
Member

@oliviertassinari oliviertassinari Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The new test case looks correct. Regarding getting it to fail. We added more defensive logic, later on, to avoid two swipeable drawers responding to the same event. It seems that this logic is winning over the previous solutions. For instance, event.defaultMuiPrevented now only seems to make sense if there is a custom component.

claimedSwipeInstance now seems to be solved by testing the target of the event. We should be able to remove this logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm somewhat leaning towards keeping the implementation to be safe. Touch related interactions are hard to test automatically so it may take a while before people realize we broken something.

const touchStartEvent = fireSwipeAreaMouseEvent(
wrapper.find(SwipeableDrawer).at(0),
'touchstart',
{
touches: [{ pageX: 0, clientY: 0 }],
},
);
wrapper
.find(SwipeableDrawer)
.at(1)
.find(SwipeArea)
.getDOMNode()
.dispatchEvent(touchStartEvent);
fireBodyMouseEvent('touchmove', { touches: [{ pageX: 20, clientY: 0 }] });
fireBodyMouseEvent('touchmove', { touches: [{ pageX: 180, clientY: 0 }] });
fireBodyMouseEvent('touchend', { changedTouches: [{ pageX: 180, clientY: 0 }] });
fireEvent.touchStart(screen.getAllByTestId('swipearea')[0], {
touches: [{ pageX: 0, clientY: 0 }],
});
fireEvent.touchMove(document.body, { touches: [{ pageX: 20, clientY: 0 }] });
fireEvent.touchMove(document.body, { touches: [{ pageX: 180, clientY: 0 }] });
fireEvent.touchEnd(document.body, { changedTouches: [{ pageX: 180, clientY: 0 }] });

expect(handleOpen.callCount).to.equal(1);
});
});

it('does not crash when updating the parent component while swiping', () => {
const wrapper = mount(
render(
<SwipeableDrawer
onOpen={() => {}}
onClose={() => {}}
open={false}
PaperProps={{ component: NullPaper }}
SwipeAreaProps={{ 'data-testid': 'swipearea' }}
>
<div>SwipeableDrawer</div>
</SwipeableDrawer>,
);
fireSwipeAreaMouseEvent(wrapper, 'touchstart', { touches: [{ pageX: 0, clientY: 0 }] });

fireEvent.touchStart(screen.getByTestId('swipearea'), { touches: [{ pageX: 0, clientY: 0 }] });
// simulate paper ref being null because of the drawer being updated
fireBodyMouseEvent('touchmove', { touches: [{ pageX: 20, clientY: 0 }] });
fireEvent.touchMove(document.body, { touches: [{ pageX: 20, clientY: 0 }] });
});

describe('no backdrop', () => {
Expand Down