test(Dropdown): add more unit tests#1201
Conversation
silviuaavram
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
silviuaavram
changed the title
Generated by 🚫 dangerJS |
Codecov Report
@@ Coverage Diff @@
## master #1201 +/- ##
==========================================
+ Coverage 82.53% 83.21% +0.67%
==========================================
Files 751 751
Lines 8881 8881
Branches 1260 1260
==========================================
+ Hits 7330 7390 +60
+ Misses 1536 1477 -59
+ Partials 15 14 -1
Continue to review full report at Codecov.
|
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
| expect(onSelectedChange).toHaveBeenLastCalledWith( | ||
| null, | ||
| expect.objectContaining({ | ||
| value: items[itemSelectedIndex], |
There was a problem hiding this comment.
| value: items[itemSelectedIndex], | |
| value: 'item2', |
Nit: It's more human readable
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
Outdated
Show resolved
Hide resolved
…t-ui/react into chore-add-dropdown-tests
| const triggerButton = wrapper.find(`button.${Dropdown.slotClassNames.triggerButton}`) | ||
|
|
||
| triggerButton | ||
| .simulate('click') |
There was a problem hiding this comment.
Shouldn't we first navigate to the list items by arrow down and then press escape? Otherwise we are laying about what the test is checking..
There was a problem hiding this comment.
we are already checking that on trigger click, the focus goes to the list, it's a test called 'opens the menu and moves focus to list in selection mode'
so assuming this works, the focus is already in the list, and we hit Escape to close it.
layershifter
changed the title
| onOpenChange.mockReset() | ||
| }) | ||
|
|
||
| it('is false when closed by trigger button click', () => { |
There was a problem hiding this comment.
| it('is false when closed by trigger button click', () => { | |
| it('is "false" when closed by trigger button click', () => { |
NIT
There was a problem hiding this comment.
will change here and for next tests as well thanks!
| open: false, | ||
| }), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
expect(onOpenChange).toHaveBeenLastCalledWithMatch(null, { open: false })
Not related to this PR, but I suggest to introduce a custom matcher for these cases (6 lines vs 1)...
| ) | ||
| }) | ||
|
|
||
| it('has multiple items at multiple selections if the multiple prop is supplied', () => { |
There was a problem hiding this comment.
I don't have a suggestion there, but there are definitely too many multiple 🐱
| expect(onSelectedChange).toHaveBeenLastCalledWith( | ||
| null, | ||
| expect.objectContaining({ | ||
| value: [items[indexesOfItemsSelected[0]], items[indexesOfItemsSelected[1] + 1]], |
There was a problem hiding this comment.
| value: [items[indexesOfItemsSelected[0]], items[indexesOfItemsSelected[1] + 1]], | |
| value: [items[1], items[4]], |
Can we use constant values there? It's very hard to read...
| searchInput | ||
| .simulate('click') | ||
| .simulate('change', { target: { value: 'test' } }) | ||
| .simulate('keydown', { keyCode: keyboardKey.Escape, key: 'Escape' }) |
There was a problem hiding this comment.
Not related to this PR:
It's strange to me that we should pass key always for Downshift: https://github.com/downshift-js/downshift/blob/8f987928e6a0ff4d0d9e6259748bcd243532f0ec/src/utils.js#L252
It will be great to reuse keyboard-key there an simplify it...
There was a problem hiding this comment.
I will try to take a look in a future PR.
layershifter
changed the title
layershifter
left a comment
layershifter
left a comment
There was a problem hiding this comment.
Please take a look on comments before merge 💃
6 participants
I did not follow any test plan, we don't have any. There are tests for basic things, like
highlightedIndex,open,valueand some props likehighlightFirstItemOnOpenormoveFocusOnTab.There are many more to be added, but this one is already a biggie. Will put it for review like this and will add others in a separate PR.
Did not manage to test:
moveFocusOnTab. Focus is not handled when using Tab. We can check if selection is performed but not where the focus is.