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 Box component tests #652

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

yurytut1993
Copy link
Contributor

What?

This PR is aimed to refactor Box 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 November 22, 2021 16:27
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.

Just the data-testid comment, but otherwise looks good to me.

@@ -7,91 +8,89 @@ import { render } from '@test/utils';
import { Box } from './index';

test('has margin props', () => {
const { container, rerender } = render(<Box />);
const { rerender } = render(<Box data-testid="box" />);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... 🤔 we should probably avoid using data-testid unless it's absolutely needed.

Maybe we can add some text and query that?

  const { rerender } = render(<Box>test</Box>); // screen.getByText('test');

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 think its a good way to use the most appropriate queries. Changed all over the component and updated snapshots

@yurytut1993 yurytut1993 merged commit 0a71bfb into bigcommerce:master Jan 19, 2022
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.

2 participants