improve snapshot tests #3597
improve snapshot tests #3597
Conversation
|
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>. |
| snapshot({ severity: 'danger' }, 'severity danger'); | ||
| snapshot({ severity: 'invalid' }, 'severity invalid'); | ||
| }); | ||
| snapshot(<Badge />, 'default'); |
There was a problem hiding this comment.
To make the snapshot function easier to use across components, it expects the element, not the properties only.
| @@ -1,78 +1,78 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`Badge check snapshots: default 1`] = ` | |||
| exports[`Badge default 1`] = ` | |||
There was a problem hiding this comment.
Here the 'check snapshots: ' string was not really useful, as it is clear that snapshots are being checked.
| snapshot(<Button />, 'when label is true it renders Button with default aria label'); | ||
| snapshot(<Button label={'test'} />, 'when aria-label prop is not exist aria-label prop should be equal to label prop '); | ||
| snapshot(<Button aria-label={'test'} />, 'when label prop is not exist label prop should be equal to aria-label prop'); | ||
| snapshot(<Button label={'test'} badge={'lost'} />, 'when using badge and label the aria-label should contain both values'); |
There was a problem hiding this comment.
Here, the snapshot calls replaced the old code, that was repetitive. It is now more clearly visible, that some tests are doing the same thing and that test names might be a bit incorrect. Three of them are rendering just <Button />.
| test('when removable is true it returns with remove icon', () => { | ||
| // Arrange | ||
| const removeOn = jest.fn(); | ||
| const { container } = render(<Chip removable onRemove={removeOn} />); | ||
|
|
||
| expect(container).toMatchSnapshot('before remove'); |
There was a problem hiding this comment.
Here is example how to pass a name to the toMatchSnapshot call, so that two snapshots are made in the same test. Here is makes sense to snapshot before and after the action, as the component rendered differently.
| /> | ||
| </div> | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
You can observe the above how the before remove and after remove snapshots rendered different results (as expected)
| @@ -27,7 +27,6 @@ describe('InputText', () => { | |||
|
|
|||
| // Act | |||
| expect(input).toBeEnabled(); | |||
| expect(input).not.toHaveClass('p-filled'); | |||
There was a problem hiding this comment.
This is not needed, as the snapshot will cover this check.
There was a problem hiding this comment.
I left this here because it was explicitly what i was testing not meant to be implicitly figured out from the SNAPSHOT. I am proving that its not FILLED before checking the SNAPSHOT.
| @@ -40,7 +40,6 @@ describe('InputTextarea', () => { | |||
|
|
|||
| // Act | |||
| expect(input).toBeEnabled(); | |||
| expect(input).not.toHaveClass('p-filled'); | |||
There was a problem hiding this comment.
This is not needed, as the snapshot will cover this check.
There was a problem hiding this comment.
Explicit test since its in the description of the test its testing
| test(name, () => { | ||
| expect(render(element).container).toMatchSnapshot(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The reusable function, to avoid repetitive calls to test, expect, render and toMatchSnapshot
|
Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>. |
|
Just a couple of comments. |
I will put comments in the
Files changedtab about the ideas of the changes.