test(component): refactor Alert component tests

[bc-alexsaiannyi]

What?

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

[chanceaclark]

Looking good so far, but some comments to address.

[chanceaclark]

Let's refactor these tests to not use container too. For example,

  render(<Alert messages={[{ text: 'Success', link: { text: 'Link', href: '#' } }]} />);
  
  const alert = await screen.findByRole('alert');
  
  expect(alert).toMatchSnapshot();
  // ...

[bc-alexsaiannyi]

@chanceaclark, thanks for pointing this out. Changed.

[chanceaclark]

Why are we expecting the text Success to not have a class here, instead of the alert role?

Even so, I believe this is changing the behavior of the test. The diff from before is testing to see if any elements in the alert role does not contain a class test. While this is now testing if just the text Success doesn't contain the class test. For this test, this expect is a valid use-case for using container, based on the intentions of the test. (The expect(alert).not.toHaveStyle... is still good to keep.)

I believe this is what we might want 🤔

Suggested change

	expect(elem).not.toHaveClass('test');
	expect(container.getElementsByclassName('test')).toHaveLength(0);

[bc-alexsaiannyi]

@chanceaclark, I wonder when it’s acceptable to use container at all. I’ve gone through Kent's article again and it looks for me as he doesn’t recommend to use container in general.. Maybe you can advise some article or examples? Thanks.

[chanceaclark]

@bc-alexsaiannyi One of the primary goals of tests is to gain confidence in our application/page/component. RTL targets that goal in a way to write tests that mimic how normal users would interact with it, which in turn, helps us gain confidence that our application/page/component works. With that in mind, Kent recommends avoiding using container because using it is how a developer would interact, not a normal user. However, for this test case we are trying to test how a developer interacts with it and gain confidence that our component doesn't pass in a className and style props. So I would say this is a perfectly acceptable usage of container. The bottom line is that we should ask ourselves, is using container helping us gain confidence in our tests? If yes, and we know what we are doing, go right ahead.

I would say the TL'DR is to avoid using container unless you are trying to test something around how a developer would interact. And it's really only in niche cases, like this className stuff since we don't want developers applying custom styles to some of our components.

[bc-alexsaiannyi]

Thanks for explaining.

[chanceaclark]

Let's assign these to a variable before we expect on them, along with using the toBeDefined method.

  const alert = await screen.findByRole('alert');
  const heading = screen.getByRole('heading', { name: /header/i });

  expect(alert).toMatchSnapshot();
  expect(heading).toBeDefined();

[chanceaclark]

Same here

[chanceaclark]

One last comment, otherwise looks good to me

[chanceaclark]

Let's remove this cast too.

Suggested change

	const link = screen.getByRole('link') as HTMLAnchorElement;
	const link = await screen.fihdByRole<HTMLAnchorElement>('link');

[bc-alexsaiannyi]

Totally agree with removing cast here. But It looks like we don’t have async code here so what’re your thoughts on using a getBy* instead?

const link = screen.getByRole<HTMLAnchorElement>('link')

[chanceaclark]

I guess that's alright 👍