ui-components: remove enzyme and refactor tests to use RTL

[laurenbarker]

We've decided to move away from Enzyme and HTML snapshot tests in
favor of React Testing Library and testing what the user sees. Enzyme
has been removed from the managed-service repo, but we are still
working to switch over in the cockroach repo. Removing enzyme from
the ui repo gets us a step closer to a unified testing front.

Since these components are generic, reusable components, the tests will
be short if we aren't testing implementation details. Some of the
refactored tests still look for class names and attributes, but we could
choose to remove these tests entirely if we aren't getting value out of
them.

Any snapshot tests were removed and not replaced.

---

This change is 

[sjbarag]

/nick cockroach-teamcity

This change is 

/nick sjbarag

[sjbarag]

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @lassenordahl, @laurenbarker, and @Marjan154)

---

packages/ui-components/src/InlineAlert/InlineAlert.test.tsx, line 9 at r1 (raw file):

    const title = "Hello world!";
    render(<InlineAlert title={title} />);
    screen.getByText(title);

Should this be wrapped in an expect(…).not.toBeNull() or something? If I understand correctly, this would continue to pass if we stop rendering the title, but perhaps that's out of scope for this PR?

---

packages/ui-components/src/Spinner/Spinner.tsx, line 26 at r1 (raw file):

      fill="none"
      xmlns="http://www.w3.org/2000/svg"
      aria-label="Loading..."

It might be worth calling this out in the commit body as a very small behavior change if it was intentional!

[sjbarag]

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @lassenordahl, @laurenbarker, and @Marjan154)

a discussion (no related file):
Heck, I forgot to press some buttons and clicked "Publish" too soon.

This looks excellent! I'm really surprised how small this diff is — maybe we just didn't have that many components in this repo? 🤷

---

[laurenbarker]

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @lassenordahl, @Marjan154, and @sjbarag)

a discussion (no related file):

Previously, sjbarag (Sean Barag) wrote…

Heck, I forgot to press some buttons and clicked "Publish" too soon.

This looks excellent! I'm really surprised how small this diff is — maybe we just didn't have that many components in this repo? 🤷

TFTR! We don't have many components in this package and newer components were already using RTL for tests.

---

---

packages/ui-components/src/InlineAlert/InlineAlert.test.tsx, line 9 at r1 (raw file):

Previously, sjbarag (Sean Barag) wrote…

Should this be wrapped in an expect(…).not.toBeNull() or something? If I understand correctly, this would continue to pass if we stop rendering the title, but perhaps that's out of scope for this PR?

getByText() will error if the title isn't found, so the test will fail if we stop rendering the title. expect(screen.getByText(title)) would be equivalent.

Code quote:

getByText

---

packages/ui-components/src/Spinner/Spinner.tsx, line 26 at r1 (raw file):

Previously, sjbarag (Sean Barag) wrote…

It might be worth calling this out in the commit body as a very small behavior change if it was intentional!

It was intentional! I'll add it to the commit message on merge.

[sjbarag]

Reviewable status: 0 of 14 files reviewed, all discussions resolved (waiting on @lassenordahl and @Marjan154)

---

packages/ui-components/src/InlineAlert/InlineAlert.test.tsx, line 9 at r1 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

getByText() will error if the title isn't found, so the test will fail if we stop rendering the title. expect(screen.getByText(title)) would be equivalent.

WOAH I had no idea. I've gotta read the RTL docs more thoroughly then :)

[laurenbarker]

---

packages/ui-components/src/InlineAlert/InlineAlert.test.tsx, line 9 at r1 (raw file):

Previously, sjbarag (Sean Barag) wrote…

WOAH I had no idea. I've gotta read the RTL docs more thoroughly then :)

https://testing-library.com/docs/queries/about/#types-of-queries goes over the different types of queries available :)

[nathanstilwell]

I just spend about an hour trying to figure out a way to test intents or colors with React Testing Library and came up short (because Jest won't use babel to compile library code and I can't find a format for style-dictionary that will satisfy both Jest and TypeScript, I push up an example).

Do we currently have a strategy to test that styles are being applied correctly in RTL?

[nathanstilwell]

Here was my attempt. #430

[laurenbarker]

RTL testing philosophy focuses on behavior, so if a style doesn't change behavior, it's going to be difficult to test using RTL helpers.

It's still possible to check that classes are being applied using Jest helpers and Javascript, but testing that classes are being applied doesn't give us confidence that everything looks correct. For instance, a class could be correctly added to an element, but the class in the stylesheet has a typo so no style is applied. The test would pass, but the component would have a bug.

Visual snapshot testing is the best tool to ensure there are no style bugs.

[nathanstilwell]

Agreed. I can see the difference between having a component behaving in a certain way based on props and "looking correct" (which should involve taking a visual snapshot).