Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(component): refactor Button component tests #651

Merged

Conversation

bc-alexsaiannyi
Copy link
Contributor

@bc-alexsaiannyi bc-alexsaiannyi commented Nov 18, 2021

What?

This PR is aimed to refactor Button component's tests according to best practices. We take into account recommendations from Kent C. Dodds article. For instance, using container for query-ing elements has been removed. Instead, we replace it with screen.

NOTE: We've also switched to using userEvent instead of fireEvent whenever it's possible. For this purpose a package @testing-library/user-event has been added and yarn.lock is updated as well.

Why?

the changes are based on GitHub issue and PRs will be open for each component separately.

Testing/Proof

The tests have been successfully run locally.

@bc-alexsaiannyi bc-alexsaiannyi requested review from a team as code owners November 18, 2021 08:24
@chanceaclark
Copy link
Contributor

Hey @bc-alexsaiannyi if this is a draft PR, would you mind using the Draft Pull Request feature instead of appending [DRAFT]? It'll leave reviewers off until you are ready for review.

@bc-alexsaiannyi bc-alexsaiannyi changed the title [DRAFT] test(component): refactor Button component tests test(component): refactor Button component tests Nov 22, 2021
@bc-alexsaiannyi
Copy link
Contributor Author

@chanceaclark , sorry. I will do it next time. This time you can check my changes)


fireEvent.click(button);
fireEvent.click(screen.getByRole<HTMLButtonElement>('button'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also switch out usages of fireEvent with userEvent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a good point. I also see that we will need @testing-library/user-event package for this purpose. Is it ok to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure

<Button iconLeft={plusIcon} iconOnly={plusIcon} iconRight={plusIcon}>
Button
</Button>,
);

expect(getAllByTestId('icon-only').length).toBe(1);
expect(screen.getAllByTestId('icon-only').length).toBe(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(screen.getAllByTestId('icon-only').length).toBe(1);
expect(screen.getAllByTestId('icon-only')).toHaveLength(1);

@bc-alexsaiannyi bc-alexsaiannyi force-pushed the bigdesign-635-button branch 3 times, most recently from b0b246d to 1525ec8 Compare November 23, 2021 16:34
Copy link
Contributor

@golcinho golcinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙇

@bc-alexsaiannyi bc-alexsaiannyi merged commit e015ff6 into bigcommerce:master Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants