-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Datepicker: Allow null value for currentDate on mounting #12963
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { shallow } from 'enzyme'; | ||
import moment from 'moment'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import DatePicker from '../date'; | ||
|
||
const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss'; | ||
|
||
describe( 'DatePicker', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm glad to see tests, but for what it's worth these tests as written would not have caught the error described in my previous review, since there is zero coverage for |
||
it( 'should pass down a moment object for currentDate', () => { | ||
const currentDate = '1986-10-18T23:00:00'; | ||
const wrapper = shallow( <DatePicker currentDate={ currentDate } /> ); | ||
const date = wrapper.children().props().date; | ||
expect( moment.isMoment( date ) ).toBe( true ); | ||
expect( date.isSame( moment( currentDate ) ) ).toBe( true ); | ||
} ); | ||
|
||
it( 'should pass down a null date when currentDate is set to null', () => { | ||
const wrapper = shallow( <DatePicker currentDate={ null } /> ); | ||
expect( wrapper.children().props().date ).toBeNull(); | ||
} ); | ||
|
||
it( 'should pass down a moment object for now when currentDate is undefined', () => { | ||
const wrapper = shallow( <DatePicker /> ); | ||
const date = wrapper.children().props().date; | ||
expect( moment.isMoment( date ) ).toBe( true ); | ||
expect( date.isSame( moment(), 'second' ) ).toBe( true ); | ||
} ); | ||
|
||
describe( 'getMomentDate', () => { | ||
it( 'should return a Moment object representing a given date string', () => { | ||
const currentDate = '1986-10-18T23:00:00'; | ||
const wrapper = shallow( <DatePicker /> ); | ||
const momentDate = wrapper.instance().getMomentDate( currentDate ); | ||
|
||
expect( moment.isMoment( momentDate ) ).toBe( true ); | ||
expect( momentDate.isSame( moment( currentDate ) ) ).toBe( true ); | ||
} ); | ||
|
||
it( 'should return null when given a null agrument', () => { | ||
const currentDate = null; | ||
const wrapper = shallow( <DatePicker /> ); | ||
const momentDate = wrapper.instance().getMomentDate( currentDate ); | ||
|
||
expect( momentDate ).toBeNull(); | ||
} ); | ||
|
||
it( 'should return a Moment object representing now when given an undefined argument', () => { | ||
const wrapper = shallow( <DatePicker /> ); | ||
const momentDate = wrapper.instance().getMomentDate(); | ||
|
||
expect( moment.isMoment( momentDate ) ).toBe( true ); | ||
expect( momentDate.isSame( moment(), 'second' ) ).toBe( true ); | ||
} ); | ||
} ); | ||
|
||
describe( 'onChangeMoment', () => { | ||
it( 'should call onChange with a formated date of the input', () => { | ||
const onChangeSpy = jest.fn(); | ||
const currentDate = '1986-10-18T11:00:00'; | ||
const wrapper = shallow( <DatePicker currentDate={ currentDate } onChange={ onChangeSpy } /> ); | ||
const newDate = moment(); | ||
|
||
wrapper.instance().onChangeMoment( newDate ); | ||
|
||
expect( onChangeSpy ).toHaveBeenCalledWith( newDate.format( TIMEZONELESS_FORMAT ) ); | ||
} ); | ||
|
||
it( 'should call onChange with hours, minutes, seconds of the current time when currentDate is undefined', () => { | ||
let onChangeSpyArgument; | ||
const onChangeSpy = ( arg ) => onChangeSpyArgument = arg; | ||
const wrapper = shallow( <DatePicker onChange={ onChangeSpy } /> ); | ||
const newDate = moment( '1986-10-18T11:00:00' ); | ||
const current = moment(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This worries me a bit; I feel like it will be prone to test fragility, where the time required to execute the test is such that I think our options would be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets go with I figured comparison on basis of seconds was enough to overcome differences in execution time, but in an effort to prevent future headaches, a change is a good idea. I changed the tests to save the argument passed to onChange for later comparison using moment's |
||
const newDateWithCurrentTime = newDate | ||
.clone() | ||
.set( { | ||
hours: current.hours(), | ||
minutes: current.minutes(), | ||
seconds: current.seconds(), | ||
} ); | ||
wrapper.instance().onChangeMoment( newDate ); | ||
|
||
expect( moment( onChangeSpyArgument ).isSame( newDateWithCurrentTime, 'minute' ) ).toBe( true ); | ||
} ); | ||
|
||
it( 'should call onChange with hours, minutes, seconds of the current time when currentDate is null', () => { | ||
let onChangeSpyArgument; | ||
const onChangeSpy = ( arg ) => onChangeSpyArgument = arg; | ||
const wrapper = shallow( <DatePicker currentDate={ null } onChange={ onChangeSpy } /> ); | ||
const newDate = moment( '1986-10-18T11:00:00' ); | ||
const current = moment(); | ||
const newDateWithCurrentTime = newDate | ||
.clone() | ||
.set( { | ||
hours: current.hours(), | ||
minutes: current.minutes(), | ||
seconds: current.seconds(), | ||
} ); | ||
wrapper.instance().onChangeMoment( newDate ); | ||
|
||
expect( moment( onChangeSpyArgument ).isSame( newDateWithCurrentTime, 'minute' ) ).toBe( true ); | ||
} ); | ||
} ); | ||
} ); |
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.
getMomentDate
can returnnull
, at which point this would throw an error.It would be good to document any function we introduce, as it may make this sort of thing easier to catch, when recognizing that the return value of the function is nullable.
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.
Unit tests, as well, would be good to capture this sort of issue and the expected behavior.
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.
Eesh, good call. I've gone back to the original implementation here and included a comment about to explain why
moment()
is used ifcurrentDate
is undefined or null.Unit tests added. Thanks