Add jest unit testing to Chip component #3558
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>. |
3 similar comments
|
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>. |
|
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>. |
|
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>. |
|
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>. |
user-event library should move to devDependencies check if label text is test check if the icon value is correct
| fireEvent.click(chipRemoveIcon); | ||
|
|
||
| expect(container.getElementsByClassName('p-chip-remove-icon pi pi-times-circle').length).toBe(0); | ||
| expect(removeOn).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
We can decide as a team or @mertsincan can decide but I am big fan of "arrange act assert" so something like this.
// Arrange
const removeOn = jest.fn();
const { container } = render(<Chip removable onRemove={removeOn} />);
const chipRemoveIcon = container.getElementsByClassName('p-chip-remove-icon')[0];
// Act
fireEvent.click(chipRemoveIcon);
// Assert
expect(container.getElementsByClassName('p-chip-remove-icon pi pi-times-circle').length).toBe(0);
expect(removeOn).toHaveBeenCalledTimes(1);only because it forces developers to properly segment and think through the test
There was a problem hiding this comment.
Hi, Testing library is a legit library. Buğra makes this understandable by using the right spaces. Reviews provide that too. With all due respect, I don't believe this is necessary. I wonder why you think it's necessary.
There was a problem hiding this comment.
@bahadirsofuoglu it is a very common pattern in unit testing in C#/Java etc.
Here is a great explanation: https://docs.telerik.com/devtools/justmock/basic-usage/arrange-act-assert#:~:text=Benefits%20of%20Using%20Arrange%20Act%20Assert&text=Clarifies%20and%20focuses%20attention%20on,many%20different%20things%20at%20once.
| const chipImage = within(wrapper[0]).getByRole('img'); | ||
|
|
||
| expect(chipImage.getAttribute('alt')).toBe('chip'); | ||
| }); |
There was a problem hiding this comment.
We should add a test for the imageAlt property of the Chip as well.
melloware
added
the
Component: Test
|
I am going to merge this and then update some changes |
Feature Requests
Fix #3557 Add jest unit test to Chip component