Refactor ValidatedForm tests #465
Conversation
|
|
||
| expect(validator).not.toBeCalled(); | ||
| describe('on validation', () => { | ||
| let wrapper, form; |
| @@ -9,11 +9,7 @@ jest.mock('meteor/check'); | |||
| describe('ValidatedForm', () => { | |||
| const onChange = jest.fn(); | |||
| const onSubmit = jest.fn(); | |||
| const onValidate = jest.fn((model, error, next) => { | |||
| if (error) return; | |||
There was a problem hiding this comment.
I think, that this might be the issue - this test was not failing, because it never returned.
EDIT: That's not the case, but it's a good catch.
| wrapper.setProps({model}); | ||
|
|
||
| expect(validator).not.toBeCalled(); | ||
| beforeEach(async () => { |
There was a problem hiding this comment.
This doesn't have to be async.
| }); | ||
| onValidate.mockImplementationOnce((m, e, next) => { |
There was a problem hiding this comment.
Let's keep using full names everywhere.
| next(null); | ||
| }); | ||
| form.validate(); | ||
| await new Promise(resolve => process.nextTick(resolve)); |
There was a problem hiding this comment.
I'm pretty sure that this one is no longer needed, as you don't use wrapper nor form below.
| <ValidatedForm model={model} schema={schema} onValidate={onValidate} validate="onChange" /> | ||
| ); | ||
| describe('on change', () => { | ||
| describe('in "onChange" mode', () => { |
There was a problem hiding this comment.
There's no convention, but I'd keep ` instead of ".
| wrapper.find('form').simulate('submit'); | ||
| expect(validator).toHaveBeenCalled(); | ||
| expect(onValidate).toHaveBeenCalled(); | ||
| expect(onSubmit).toHaveBeenCalled(); |
There was a problem hiding this comment.
It's not called, because:
.validate()is not submitting the form (it should be.submit())onSubmitis defined above, but not passed to the form (line 36)
There was a problem hiding this comment.
Thanks! That makes perfect sense. I have now refactored it now (a9d7566). I have also addressed the stylistic points.
The test 'lets `onValidate` suppress `validator` errors' is really about testing `onValidate`, not the submit flow, so now the test is 'leaves error state alone when `onValidate` suppress `validator` errors' (like 'updates error state with async errors from `onValidate`').
Codecov Report
@@ Coverage Diff @@
## master #465 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 157 157
Lines 1404 1404
=====================================
Hits 1404 1404Continue to review full report at Codecov.
|
|
Thank you again @BudgieInWA! |
While looking to test my upcoming changes to #441, I got a bit carried away trying to make sense of the
ValidatedFormtests. I have refactored them in a way that highlights the patterns and makes it clear to me what my new tests should look like. Hopefully that holds true in general.There was one of the tests that I couldn't get to the bottom of, as described initially in fa4dc6b. What do you think?
The real history is available here for the curious.