-
Notifications
You must be signed in to change notification settings - Fork 77
fix: DatePicker update unit tests #332
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.
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
Conversation
src/DatePicker/DatePicker.test.js
Outdated
| // check to make sure calendar is shown | ||
| expect(wrapper.state('hidden')).toBeFalsy(); | ||
|
|
||
| // click to hide calendar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong comment. It's simulating a click in the input, which based on the next assertion, should not hide the calendar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greg-a-smith - thanks for noticing, i just updated the comment
| let event = new MouseEvent('mousedown', { | ||
| target: document.querySelector('body') | ||
| }); | ||
| document.dispatchEvent(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the added mousedown event, but then no additional assertions. I'm probably just missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed MouseEvent code since it was not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still seeing a couple of these in the files changed diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, missed that mouseDown event. Just added an assertion to check that calendar is hidden.
| let event = new MouseEvent('mousedown', { | ||
| target: document.querySelector('body') | ||
| }); | ||
| document.dispatchEvent(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the added mousedown event, but then no additional assertions. I'm probably just missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added assertion to check that calendar is hidden
src/DatePicker/DatePicker.test.js
Outdated
|
|
||
|
|
||
| // press Esc key | ||
| wrapper.find('input[type="text"]').simulate('keypress', { key: 'Esc' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't each test block end with an assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assertions at the end of test
greg-a-smith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 🚢
* fix: update unit tests * fix comments, remove unnecessary test code * added assertion check after MouseDown event
Description
Update unit tests for DatePicker component