Added tests for utils functions isSelectable and mergeModifiers#9
Added tests for utils functions isSelectable and mergeModifiers#9CatsOnFilm wants to merge 1 commit intohernansartorio:masterfrom CatsOnFilm:add_tests
utils functions isSelectable and mergeModifiers#9Conversation
* ... This change addresses the need by: - add unit tests for `isSelectable` predicate utility - dates within range (inclusive of range dates) -> true - dates outside range dates -> false - add unit tests for `mergeModifiers` utility - documenting behaviour of merging distinct lists - documenting behaviour of lists containing same keys
hernansartorio
left a comment
hernansartorio
left a comment
There was a problem hiding this comment.
Thank you for this! I had been meaning to add these but had to leave them for later.
I left some minor comments, but this is an awesome starting point :)
| @@ -0,0 +1,77 @@ | |||
| // import React from 'react' | |||
| // import { format } from 'date-fns' | |||
There was a problem hiding this comment.
You can remove these comments.
| expect(isSelectable(minimumDate, { minimumDate, maximumDate })).toBe(true) | ||
| expect(isSelectable(maximumDate, { minimumDate, maximumDate })).toBe(true) | ||
| }) | ||
| it('should return `false` for a date that is before `minimumDate` or after `maximumDate`', () => { |
There was a problem hiding this comment.
Purely stylistic (I should add an eslint rule for this) but could you leave a blank line between the end of a block and the start of a new one? In the same way that you do within the mergeModifiers tests.
| expect(mergeModifiers(mockBaseModifiers)).toEqual(mockBaseModifiers) | ||
| }) | ||
|
|
||
| it('should (in effect) merge a provided modifier into the `modifiers` object if the key is not already present', () => { |
There was a problem hiding this comment.
What does "in effect" mean?
| }) | ||
| }) | ||
|
|
||
| it('should check both `baseModifier` and `newModifier` of the same name', () => { |
There was a problem hiding this comment.
I think a better way to test this one, without worrying about what values are passed, might be using toHaveBeenCalled, something like:
const modifierA = jest.fn()
const modifierB = jest.fn()
const baseModifiers = { test: modifierA }
const newModifiers = { test: modifierB }
const mergedModfiers = mergeModifiers(baseModifiers, newModifiers)
mergedModfiers.test()
expect(modifierA).toHaveBeenCalled()
expect(modifierB).toHaveBeenCalled()
| expect(mergedModfiers.test(NOT_A_NUMBER_OVER_1_OR_A_STRING)).toBe(false) | ||
| }) | ||
|
|
||
| it('should prefer a `baseModifier` with the same name as a `newModifier`', () => { |
There was a problem hiding this comment.
Similarly, this one could be like this:
const modifierA = jest.fn(() => true)
const modifierB = jest.fn()
const baseModifiers = { test: modifierA }
const newModifiers = { test: modifierB }
const mergedModfiers = mergeModifiers(baseModifiers, newModifiers)
mergedModfiers.test()
expect(modifierA).toHaveBeenCalled()
expect(modifierB).not.toHaveBeenCalled()
| expect(mergedModfiers.today('which one?')).toBe('baseModifier') | ||
| }) | ||
|
|
||
| it('should assign the `newModifier` if no `baseModifier` of the same name is provided', () => { |
There was a problem hiding this comment.
Isn't what's tested here already covered by the should (in effect) merge a provided modifier into the 'modifiers' object if the key is not already present test?
fixes time zone issues
2 participants
This change addresses the need by:
isSelectablepredicate utilitymergeModifiersutility