chore(Truncate): component unit test revamp#7686
chore(Truncate): component unit test revamp#7686wise-king-sullyman merged 3 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-7686.surge.sh A11y report: https://patternfly-react-pr-7686-a11y.surge.sh |
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This is looking really good! I only have a few changes I'd like to see 🙂
| test('renders tooltip placement', () => { | ||
| const { asFragment } = render( | ||
| <Truncate | ||
| content={''} | ||
| tooltipPosition='auto' | ||
| /> | ||
| ); | ||
|
|
||
| expect(asFragment()).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
Rather than using a snapshot here I would prefer to see independent tests that explicitly assert the behavior we're expecting, i.e. that Tooltip receives the position we expect, that it receives the content we expect, and that it receives the children we expect.
| test('renders pf-c-truncate__start when trailingNumChars prop passed', () => { | ||
| render( | ||
| <Truncate | ||
| content={'Testing truncate content'} | ||
| trailingNumChars={3} | ||
| position='middle' | ||
| /> | ||
| ); | ||
|
|
||
| const start = screen.getByText('Testing truncate cont'); | ||
|
|
||
| expect(start).toHaveClass('pf-c-truncate__start'); | ||
| }); | ||
|
|
||
| test('renders pf-c-truncate__end when trailingNumChars prop passed', () => { | ||
| render( | ||
| <Truncate | ||
| content={'Testing truncate content'} | ||
| trailingNumChars={3} | ||
| position='middle' | ||
| /> | ||
| ); | ||
|
|
||
| const end = screen.getByText('ent'); | ||
|
|
||
| expect(end).toHaveClass('pf-c-truncate__end'); | ||
| }); |
There was a problem hiding this comment.
I think these could actually be condensed into one test with multiple assertions since they're only testing one behavior.
Also regarding the test names, is it the trailingNumChars prop behavior that is causing __start and __end to both render, or is it the value of the position prop?
There was a problem hiding this comment.
I believe it is the value of the position prop. If it's start then we get __end, if it's end then we get __start, and middle gives us both
| @@ -28,3 +130,26 @@ test('renders middle truncation', () => { | |||
| ); | |||
| expect(asFragment()).toMatchSnapshot(); | |||
There was a problem hiding this comment.
Can you refactor this test to not be a snapshot and explicitly assert against the default behavior when position=middle and traillingNumChars isn't passed?
| test('only renders pf-c-truncate__start if truncate position is the end', () => { | ||
| render( | ||
| <Truncate | ||
| content={'Testing truncate content'} | ||
| position='end' | ||
| /> | ||
| ); | ||
|
|
||
| const start = screen.getByText('Testing truncate content'); | ||
|
|
||
| expect(start).toHaveClass('pf-c-truncate__start'); | ||
| }); |
There was a problem hiding this comment.
Since position=end is the default value I think this test should actually omit the position prop so that we have a test about the default behavior here.
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This looks great! Thanks for taking the time to make all these changes and put this together!
* chore(Truncate): component unit test revamp * chore(Truncate): updated snapshot tests to be explicitly asserting * chore(Truncate): updating tooltip content test
4 participants
What: Closes #7643 . Unit tests revamp -- note that there was an issue with the
positionprop where ifposition=start, then onlypf-c-truncate__endwould render. However,getByTextcould not detect the inputted text as a result of‎so a snapshot test was performed instead.Additional issues: N/A.