test for traffic splitting and refactor component #4700
Conversation
openshift-ci-robot
added
size/L
|
/retest |
|
/test frontend |
nemesis09
force-pushed
the
test-traffic-splitting
branch
from
a904bda
to
869a684
Compare
| mockServiceData, | ||
| } from '../../../utils/__mocks__/traffic-splitting-utils-mock'; | ||
|
|
||
| describe('TrafficSplitting', () => { |
There was a problem hiding this comment.
nit:
| describe('TrafficSplitting', () => { | |
| describe(TrafficSplitting.displayName, () => { |
There was a problem hiding this comment.
@sahil143 unless we explicitly assign a display name, this won't work.
There was a problem hiding this comment.
@christianvogt This works with the functional component as well without defining it explicitly.
There was a problem hiding this comment.
Never Mind, This doesn't work, just tested this.
| it('should match the previous traffic splitting snapshot', () => { | ||
| const tree = Renderer.create( | ||
| <TrafficSplitting service={mockServiceData} revisions={mockRevisions} />, | ||
| ).toJSON(); | ||
| expect(tree).toMatchSnapshot(); |
There was a problem hiding this comment.
Why are you using snapshot testing here? I don't think snapshot testing is the right way here because If there are some changes made in the TrafficSplittingModal component this test will fail even though there are no changes made to the TrafficSplitting component.
There was a problem hiding this comment.
Yeah, I agree with Sahil. Snapshot testing isn't very useful in this case. I usually don't prefer snapshot testing. You should be testing some rendering logic of the component or even the validation logic.
There was a problem hiding this comment.
Snapshot testing this is not useful at all.
We should be using shallow to mount and then assert specific changes in in the rendered output.
There was a problem hiding this comment.
removed snapshot testing and updated code with shallow testing
|
/kind cleanup |
openshift-ci-robot
added
the
kind/cleanup
| }; | ||
| }; | ||
|
|
||
| const Controller: React.FC<ControllerProps> = (props) => { |
There was a problem hiding this comment.
What was the need to move the Controller component into a separate file? You are not using the component anywhere else. And now you have two components Controller and TrafficSplitingController which seems ambiguous to me.
There was a problem hiding this comment.
Agreed. In this case the only reason we have this other component is because of the poor implementation of Firehose which injects props into the child component. We will have (already do?) hooks to perform firehose like behavior and this component will just go away then.
There was a problem hiding this comment.
The intention was to follow "one file on component", but I can see now that this component just exists for implementing Firehose and has no other use. Have reverted to the initial implementation.
| it('should match the previous traffic splitting snapshot', () => { | ||
| const tree = Renderer.create( | ||
| <TrafficSplitting service={mockServiceData} revisions={mockRevisions} />, | ||
| ).toJSON(); | ||
| expect(tree).toMatchSnapshot(); |
There was a problem hiding this comment.
Yeah, I agree with Sahil. Snapshot testing isn't very useful in this case. I usually don't prefer snapshot testing. You should be testing some rendering logic of the component or even the validation logic.
nemesis09
force-pushed
the
test-traffic-splitting
branch
4 times, most recently
from
0543025
to
a4bb1a5
Compare
| ); | ||
| }); | ||
|
|
||
| it('should disable add row for one value', () => { |
There was a problem hiding this comment.
Shouldn't this say disable delete row button for one value? The prop you're checking is disableDeleteRow and we do not disable add button ever.
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('should not disable add row for more than one values', () => { |
nemesis09
force-pushed
the
test-traffic-splitting
branch
from
a4bb1a5
to
4578714
Compare
| ).toBe(false); | ||
| }); | ||
|
|
||
| it('should should render modal footer with proper values', () => { |
There was a problem hiding this comment.
| it('should should render modal footer with proper values', () => { | |
| it('should render modal footer with proper values', () => { |
nemesis09
force-pushed
the
test-traffic-splitting
branch
from
4578714
to
4d2f00a
Compare
|
/lgtm |
| }; | ||
|
|
||
| describe('TrafficSplittingModal', () => { | ||
| let wrapper; |
There was a problem hiding this comment.
@nemesis09 can we have type for wrapper
| mockRevisionItems, | ||
| } from '../../../utils/__mocks__/traffic-splitting-utils-mock'; | ||
|
|
||
| const formProps = { |
There was a problem hiding this comment.
can we have type for formProps and move it to beforeEach as there is just one describe block
| wrapper.find('form').simulate('submit', { | ||
| preventDefault: () => {}, | ||
| }); |
There was a problem hiding this comment.
@nemesis09 can we check on simulate submit , handleSubmit has been called
There was a problem hiding this comment.
To check changes made by handleSubmit we can check the status. That needs an async testing. I tried to test it asynchronously but I couldn't work it out as the call to Formik is made in TrafficSplitting component and handleSubmit is passed from there. To test this we need to pass a mock handleSubmit and make some changes to the form in it.
There was a problem hiding this comment.
placed a test in place to check if handle submit has been called on submit.
| expect( | ||
| wrapper | ||
| .find(ModalSubmitFooter) | ||
| .first() | ||
| .props().errorMessage, | ||
| ).toEqual('error'); | ||
| }); |
There was a problem hiding this comment.
this is a synchronised testing, ideally in the async case,error is set in status inside handleSubmit from TrafficSplitting component. Here I'm just checking if the prop is passed. any async call to handleSubmit is consumed.
nemesis09
force-pushed
the
test-traffic-splitting
branch
from
4d2f00a
to
49280c7
Compare
| formProps = { | ||
| errors: {}, | ||
| touched: {}, | ||
| isSubmitting: true, | ||
| isValidating: true, | ||
| status: { error: 'error' }, | ||
| submitCount: 0, | ||
| dirty: false, | ||
| handleReset: jest.fn(), | ||
| handleSubmit: jest.fn(), | ||
| getFieldProps: jest.fn(), | ||
| handleBlur: jest.fn(), | ||
| handleChange: jest.fn(), | ||
| initialValues: [{ percent: 100, revisionName: 'overlayimage-fdqsf' }], | ||
| initialErrors: {}, | ||
| initialStatus: {}, | ||
| initialTouched: {}, | ||
| isValid: true, | ||
| resetForm: jest.fn(), | ||
| setErrors: jest.fn(), | ||
| setFieldError: jest.fn(), | ||
| setFieldTouched: jest.fn(), | ||
| setFieldValue: jest.fn(), | ||
| setFormikState: jest.fn(), | ||
| setStatus: jest.fn(), | ||
| setSubmitting: jest.fn(), | ||
| setTouched: jest.fn(), | ||
| setValues: jest.fn(), |
There was a problem hiding this comment.
can import props from here frontend/packages/console-shared/src/test-utils/formik-props-utils.ts
nemesis09
force-pushed
the
test-traffic-splitting
branch
from
49280c7
to
ced7845
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dboytherealest1000, debsmita1, invincibleJai, nemesis09 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 |
openshift-ci-robot
added
the
approved
Fixes
issue https://issues.redhat.com/browse/ODC-3281
Analysis / Root cause:
Increase test coverage for components in the knative package
Solution Description:
adds unit test to
TrafficSplittingModalcomponentScreens:
Browser Conformance: