Add Unit test for Breakdown Card Component #4852
Conversation
bipuladh
commented
Mar 30, 2020
bipuladh
commented
- Unit tests for BreakDown Card Body and BreakdownCard Chart
openshift-ci-robot
added
component/ceph
frontend/packages/ceph-storage-plugin/src/__tests__/breakdown-chart.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
React / Enzyme component unit testing tips:
- use shallow rendering (
shallow) whenever possible - use full DOM rendering (
mount) when the tested component depends on HOC or React context - consider snapshot-testing your component
For example, when testing Redux connected components:
let wrapper: ReactWrapper<YourComponentProps>;
beforeEach(() => {
wrapper = mount(
<YourComponent />,
{
wrappingComponent: ({ children }) => <Provider store={store}>{children}</Provider>,
},
);
});Another example, using official React test renderer:
import * as TestRenderer from 'react-test-renderer';
it('should render correctly', () => {
expect(TestRenderer.create(<YourComponent />).toJSON()).toMatchSnapshot();
});Besides React components, you can snapshot-test any serializable objects which are worth checking for potential regressions (unwanted changes).
| @@ -0,0 +1,48 @@ | |||
| import { humanizeBinaryBytes } from '@console/internal/components/utils'; | |||
|
|
|||
| export const mockData = { | |||
There was a problem hiding this comment.
I'd suggest naming it breakdownData so that when we import it in other module, we don't end up with a rather generic mockData variable or import { mockData as breakdownData } alias.
| const top5MetricsStats = getStackChartStats(mockData.top5, mockData.humanize); | ||
|
|
||
| describe('<BreakdownCardBody>', () => { | ||
| const wrapper: ShallowWrapper<BreakdownBodyProps> = shallow( |
There was a problem hiding this comment.
I'd suggest re-creating Enzyme wrapper for each test case, rather than sharing the same instance.
let wrapper: ShallowWrapper<BreakdownBodyProps>;
beforeEach(() => {
wrapper = shallow( /* your component */ );
});| const breakdownChart = wrapper.find(BreakdownChart); | ||
| expect(breakdownChart.exists()).toBe(true); | ||
| expect(breakdownChart.props().data.length).toBe(7); | ||
| // Last is poopped if capacityTotal is available(7 - 1) |
There was a problem hiding this comment.
| // Last is poopped if capacityTotal is available(7 - 1) | |
| // Last is popped if capacityTotal is available(7 - 1) |
| }); | ||
|
|
||
| it('Hides available capacity text, legend, stack', () => { | ||
| wrapper.setProps({ capacityTotal: null }); |
There was a problem hiding this comment.
This mutates the wrapper.
If all test cases share the same instance, this makes them dependent in terms of order of execution, which is generally a bad thing.
Test cases should not depend on each other. That's why it's best to re-create wrapper instance via beforeEach.
| const legends = getLegends(chartData); | ||
|
|
||
| describe('<BreakdownChart>', () => { | ||
| const wrapper: ShallowWrapper<BreakdownChartProps> = shallow( |
There was a problem hiding this comment.
I'd suggest re-creating wrapper instance via beforeEach as discussed above.
| }); | ||
|
|
||
| describe('<LinkableLegend>', () => { | ||
| const wrapper: ShallowWrapper<LinkableLegendProps> = shallow( |
There was a problem hiding this comment.
I'd suggest re-creating wrapper instance via beforeEach as discussed above.
| /> | ||
| </Tooltip> | ||
| ); | ||
| if (datum.name === OTHER || datum.name === CLUSTERWIDE) { |
There was a problem hiding this comment.
This differs from original, is that intentional?
if (datum.name[0] === OTHER || datum.name[0] === CLUSTERWIDE)There was a problem hiding this comment.
Yes. The original is erroneous.
|
|
||
| export const BreakdownChart: React.FC<BreakdownChartProps> = ({ | ||
| data, | ||
| legends, | ||
| metricModel, | ||
| ocsVersion, | ||
| }) => { | ||
| const chartData = data.map((d: StackDataPoint, index) => ( |
There was a problem hiding this comment.
The only change here is inlining chartData variable into JSX markup, right?
bipuladh
force-pushed
the
unit_test_brkdown
branch
from
4199b1a
to
b41e547
Compare
bipuladh
force-pushed
the
unit_test_brkdown
branch
from
b41e547
to
b9431bd
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
