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: CollectionControl #13656

Merged
merged 3 commits into from
Apr 5, 2021
Merged

test: CollectionControl #13656

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8c2910
Tests for CollectionControl
yardz Mar 16, 2021
a073bd9
add role to icon
yardz Mar 22, 2021
134e298
applying factory to props
yardz Mar 25, 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
0 ...omponents/controls/CollectionControl.less → .../CollectionControl/CollectionControl.less
View file
Open in desktop
File renamed without changes.
171 changes: 171 additions & 0 deletions ...set-frontend/src/explore/components/controls/CollectionControl/CollectionControl.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import userEvent from '@testing-library/user-event';
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import CollectionControl from '.';

jest.mock('@superset-ui/chart-controls', () => ({
InfoTooltipWithTrigger: (props: any) => (
<button
onClick={props.onClick}
type="button"
data-icon={props.icon}
data-tooltip={props.tooltip}
>
{props.label}
</button>
),
}));

jest.mock('..', () => ({
__esModule: true,
default: {
TestControl: (props: any) => (
<button
type="button"
onClick={() => props.onChange(0, 'update')}
data-test="TestControl"
>
TestControl
</button>
),
},
}));

let defaultProps = {
actions: {
addDangerToast: jest.fn(),
addInfoToast: jest.fn(),
addSuccessToast: jest.fn(),
addWarningToast: jest.fn(),
createNewSlice: jest.fn(),
fetchDatasourcesStarted: jest.fn(),
fetchDatasourcesSucceeded: jest.fn(),
fetchFaveStar: jest.fn(),
saveFaveStar: jest.fn(),
setControlValue: jest.fn(),
setDatasource: jest.fn(),
setDatasourceType: jest.fn(),
setDatasources: jest.fn(),
setExploreControls: jest.fn(),
sliceUpdated: jest.fn(),
toggleFaveStar: jest.fn(),
updateChartTitle: jest.fn(),
},
addTooltip: 'Add an item',
controlName: 'TestControl',
description: null,
hovered: false,
itemGenerator: jest.fn(),
keyAccessor: jest.fn(),
label: 'Time series columns',
name: 'column_collection',
onChange: jest.fn(),
placeholder: 'Empty collection',
type: 'CollectionControl',
validationErrors: [],
validators: [jest.fn()],
value: [{ key: 'hrYAZ5iBH' }],
};

beforeEach(() => {
defaultProps = {
actions: {
addDangerToast: jest.fn(),
addInfoToast: jest.fn(),
addSuccessToast: jest.fn(),
addWarningToast: jest.fn(),
createNewSlice: jest.fn(),
fetchDatasourcesStarted: jest.fn(),
fetchDatasourcesSucceeded: jest.fn(),
fetchFaveStar: jest.fn(),
saveFaveStar: jest.fn(),
setControlValue: jest.fn(),
setDatasource: jest.fn(),
setDatasourceType: jest.fn(),
setDatasources: jest.fn(),
setExploreControls: jest.fn(),
sliceUpdated: jest.fn(),
toggleFaveStar: jest.fn(),
updateChartTitle: jest.fn(),
},
addTooltip: 'Add an item',
controlName: 'TestControl',
description: null,
hovered: false,
itemGenerator: jest.fn(),
keyAccessor: jest.fn(),
label: 'Time series columns',
name: 'column_collection',
onChange: jest.fn(),
placeholder: 'Empty collection',
type: 'CollectionControl',
validationErrors: [],
validators: [jest.fn()],
value: [{ key: 'hrYAZ5iBH' }],
};
});

test('Should render', () => {
render(<CollectionControl {...defaultProps} />);
expect(screen.getByTestId('CollectionControl')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Just a tip, if you can't assign any reasonable role to the container component, you can always access it using:

const { container } = render(<CollectionControl {...defaultProps} />);
expect(container).toBeInTheDocument();

😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but I avoid using const {container} = render() as much as possible. I find it more reliable to use screen methods. I've had some problems with containers, so I avoid them whenever is possible.

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 curious what sort of problems you were running into there that might be worth avoiding them for. I like @michael-s-molina 's implementation - it's clean, and it makes less noise in the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rusackas It was not in this specific case. But in the past I've had problems that container had a very different result than screen. As screen is theoretically the HTML that will be rendered in the browser, I prefer to use it. Any change in behavior or configuration will not change the test result. That way I'm sure the test is right.

I only use container when I'm doing unit tests, and even in these cases I avoid it as much as possible.

Copy link
Member

@michael-s-molina michael-s-molina Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@yardz I don't know if in the past the implementation was different. Nowadays it's very similar. It will be different when we specify a container for render that is not the default.

Screen Shot 2021-03-26 at 11 32 38 AM

Screen Shot 2021-03-26 at 11 33 05 AM

Kent C. Dodds in his excellent article Common mistakes with React Testing Library recommends using screen whenever possible to avoid destructuring.

Screen Shot 2021-03-26 at 11 34 01 AM

I also think we should always use screen. That being said, in the case where we CANNOT find a satisfactory role/text/placeholder for our top component and we just want to test that the div has rendered, both test-id or container are valid options. Since both options are not accessible, I prefer the one where we don't leave fingerprints in the component's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very nice post, several tips!

Well, I don't think that adding this test-id has any negative side effects at any level so the discussion comes down to "use or not the test-id." When it comes down to that it seems to me more like a discussion of "taste" than of "good practices".

If you think this is a block for the merge I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

@yardz For me, it's not a blocker as indicated by the ✔️.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not seen it haha

});

test('Should show the button with the label', () => {
render(<CollectionControl {...defaultProps} />);
expect(
screen.getByRole('button', { name: defaultProps.label }),
).toBeInTheDocument();
expect(
screen.getByRole('button', { name: defaultProps.label }),
).toHaveTextContent(defaultProps.label);
});

test('Should have add button', () => {
render(<CollectionControl {...defaultProps} />);

expect(defaultProps.onChange).toBeCalledTimes(0);
userEvent.click(screen.getByRole('button', { name: 'add-item' }));
expect(defaultProps.onChange).toBeCalledWith([
{ key: 'hrYAZ5iBH' },
undefined,
]);
});

test('Should have remove button', () => {
render(<CollectionControl {...defaultProps} />);

expect(defaultProps.onChange).toBeCalledTimes(0);
userEvent.click(screen.getByRole('button', { name: 'remove-item' }));
expect(defaultProps.onChange).toBeCalledWith([]);
});

test('Should have SortableDragger icon', () => {
render(<CollectionControl {...defaultProps} />);
expect(screen.getByTestId('SortableDragger')).toBeVisible();
});

test('Should call Control component', () => {
render(<CollectionControl {...defaultProps} />);

expect(defaultProps.onChange).toBeCalledTimes(0);
userEvent.click(screen.getByTestId('TestControl'));
expect(defaultProps.onChange).toBeCalledWith([{ key: 'hrYAZ5iBH' }]);
});
12 changes: 8 additions & 4 deletions ...components/controls/CollectionControl.jsx → ...ents/controls/CollectionControl/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import {
} from 'react-sortable-hoc';

import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
import ControlHeader from '../ControlHeader';
import controlMap from '.';
import ControlHeader from 'src/explore/components/ControlHeader';
import controlMap from '..';
import './CollectionControl.less';

const propTypes = {
Expand Down Expand Up @@ -60,7 +60,11 @@ const defaultProps = {
const SortableListGroupItem = SortableElement(ListGroupItem);
const SortableListGroup = SortableContainer(ListGroup);
const SortableDragger = SortableHandle(() => (
<i className="fa fa-bars text-primary" style={{ cursor: 'ns-resize' }} />
<i
data-test="SortableDragger"
className="fa fa-bars text-primary"
style={{ cursor: 'ns-resize' }}
/>
));

export default class CollectionControl extends React.Component {
Expand Down Expand Up @@ -134,7 +138,7 @@ export default class CollectionControl extends React.Component {

render() {
return (
<div className="CollectionControl">
<div data-test="CollectionControl" className="CollectionControl">
<ControlHeader {...this.props} />
{this.renderList()}
<InfoTooltipWithTrigger
Expand Down