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 Checkbox component tests #655

Closed

Conversation

yurytut1993
Copy link
Contributor

What?

This PR is aimed to refactor Checkbox 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.

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.

@yurytut1993 yurytut1993 requested review from a team as code owners December 3, 2021 07:56
@yurytut1993 yurytut1993 force-pushed the bigdesign-635-checkbox branch 2 times, most recently from 18d08e2 to a2e99ac Compare December 3, 2021 08:33
Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Looks good, however one minor comment.

const labels = container.querySelectorAll('label');
render(<Checkbox label="Checked" checked={true} onChange={onChange} />);

const labels = screen.getByRole<HTMLInputElement>('checkbox').parentElement!.querySelectorAll('label');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be screen.getAllByRole('label')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also looking on similar approach but looks like such role does not exist

Copy link
Contributor

Choose a reason for hiding this comment

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

what about getByText(/checked/)? might still be better than all this... 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we will not catch label with svg inside

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally forgot there is the screen.getByLabelText('') query 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Query above may return an input associated with the label, but we need to collect all labels and then iterate through them. Can you provide some code according to our component? Maybe I have wrong understanding of your idea

@golcinho
Copy link
Contributor

Outdated 👍

@golcinho golcinho closed this Aug 12, 2024
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.

4 participants