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

React 18 Migration: Refactor the React test suite to use React Testing Library #10184

Closed
131 tasks done
Tracked by #8143 ...
tay1orjones opened this issue Nov 28, 2021 · 14 comments · Fixed by #11778 or #13316
Closed
131 tasks done
Tracked by #8143 ...

React 18 Migration: Refactor the React test suite to use React Testing Library #10184

tay1orjones opened this issue Nov 28, 2021 · 14 comments · Fixed by #11778 or #13316
Labels
planning: umbrella Umbrella issues, surfaced in Projects views
Milestone

Comments

@tay1orjones
Copy link
Member

tay1orjones commented Nov 28, 2021

This issue is to track the refactoring of our React test suite to use React Testing Library.

Links and resources
How to find enzyme usages

The list below was generated by searching for enzyme within files having names containing our test suffix:

finding.enzyme.usages.mov

Please use the script to scaffold out new test files

Tasks

  1. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    tay1orjones
  2. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  3. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    aledavila
  4. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    andreancardona
  5. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    tw15egan
  6. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    francinelucca
  7. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  8. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  9. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  10. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  11. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    tw15egan
  12. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
  13. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
    tay1orjones
  14. role: dev 🤖
    tay1orjones
  15. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
    cknabe
  16. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
    tw15egan
  17. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
  18. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
  19. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    matthewgallo
  20. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    matthewgallo
  21. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖
    aledavila
  22. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
  23. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    aledavila
  24. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    cknabe
  25. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    aledavila
  26. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    tw15egan
  27. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    tw15egan
  28. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  29. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
  30. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    TannerS
  31. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    alisonjoseph
  32. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    aledavila
  33. adopter: PAL package: @carbon/react role: dev 🤖 type: infrastructure 🤖 version: 11
    francinelucca

Initial wave of refactor (complete)

  1. adopter: PAL package: @carbon/react role: dev 🤖
    tay1orjones
  2. adopter: PAL package: @carbon/react package: react role: dev 🤖
    jnm2377
  3. abbeyhrt
  4. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  5. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  6. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  7. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  8. jnm2377
  9. jnm2377
  10. jnm2377
  11. adopter: PAL package: @carbon/react role: dev 🤖
    jnm2377
  12. abbeyhrt
  13. abbeyhrt
  14. tw15egan
  15. abbeyhrt
  16. abbeyhrt
  17. abbeyhrt
  18. abbeyhrt
  19. jnm2377
  20. jnm2377
  21. jnm2377
  22. adopter: PAL package: @carbon/react package: react role: dev 🤖
    tw15egan
  23. abbeyhrt
  24. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  25. jnm2377
  26. jnm2377
  27. adopter: PAL package: @carbon/react role: dev 🤖
    jnm2377
  28. abbeyhrt
  29. abbeyhrt
  30. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  31. adopter: PAL package: @carbon/react role: dev 🤖
    tay1orjones
  32. adopter: PAL package: @carbon/react package: react role: dev 🤖
    tw15egan
  33. joshblack
  34. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  35. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  36. abbeyhrt
  37. abbeyhrt
  38. abbeyhrt
  39. abbeyhrt
  40. abbeyhrt
  41. joshblack
  42. joshblack
  43. joshblack
  44. abbeyhrt
  45. role: dev 🤖
    tay1orjones
  46. jnm2377
  47. jnm2377
  48. abbeyhrt
  49. abbeyhrt
  50. adopter: PAL package: @carbon/react role: dev 🤖
    dakahn
  51. tw15egan
  52. adopter: PAL package: @carbon/react role: dev 🤖
    abbeyhrt
  53. adopter: PAL package: @carbon/react role: dev 🤖
    jnm2377
@tay1orjones tay1orjones changed the title Refactor our existing React test suite to use React Testing Library Refactor the React test suite to use React Testing Library Nov 28, 2021
@tay1orjones tay1orjones changed the title Refactor the React test suite to use React Testing Library [Umbrella]: Refactor the React test suite to use React Testing Library Dec 10, 2021
@tay1orjones tay1orjones moved this to ⏱ Backlog in Design System Dec 10, 2021
@tay1orjones tay1orjones added the planning: umbrella Umbrella issues, surfaced in Projects views label Dec 10, 2021
@tay1orjones tay1orjones changed the title [Umbrella]: Refactor the React test suite to use React Testing Library Refactor the React test suite to use React Testing Library Dec 10, 2021
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🏗 In Progress in Design System Dec 13, 2021
@tay1orjones
Copy link
Member Author

tay1orjones commented Dec 13, 2021

High level steps

  1. Get familiar with the component, gather list of functionality/api to be tested
  2. Comment out or rename existing tests
  3. Include RTL imports
  4. Create new structure based on details to be tested
  5. Write tests
    1. Render the component using render inside each it block
    2. If applicable, find/query parts of a component to interact with
    3. If applicable, interact with the component as a person would using userEvent
    4. expect pieces of the component to be valid/correct using jest-dom matchers
  6. Double check old tests have parity with new tests - did we leave anything out?
  7. Validate that the test coverage from new tests is the same or higher as the old tests
  8. PR and delete old tests

@tay1orjones
Copy link
Member Author

tay1orjones commented Dec 13, 2021

Patterns

What should we test?

  • How does a person use the component?
    • Click, hover
    • Keyboard
      • focus
      • special keys relating to functionality - "esc to close"
    • Opening/closing
    • Selecting/de-selecting
    • Toggling/disclosure
    • Choosing from a list
  • How does a developer configure the component?
    • Internal composition via children
    • Potential common external composition? Like via Stack, Grid, etc?
    • Callbacks/event handlers
    • Refs
    • i18n, translateById
    • Other props
  • What else is part of the public API, or is depended upon by consumers?
    • "Should place class name, data-testid on the outermost DOM node"
    • "Should place forwardRef ref on the {intended element}" (input, label, outermost DOM node, etc)
    • "Should place extra props on the {intended element}" (input, label, outermost DOM node, etc)
    • ... what else?

Including RTL imports

import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import '@testing-library/jest-dom';
  • cleanup from @testing-library/react should not be needed

Find parts of a component to interact with

screen.getByRole('button', {name: /submit/i});

Avoid nesting while you're testing

  • render() in each it() block
    • avoid hasty abstractions that use beforeEach, afterAll, etc. unless absolutely necessary (mocking or something similar)
  • The less describe() blocks the better

Avoid using .toMatchSnapshot()

  • Snapshots can provide a false sense of security since they're included in coverage reports
  • Higher possibility for false positive/negatives from future component changes
  • Remove them where possible and use a more thorough assertion instead
  • Percy VRT tests (not covered here) should cover the use case of ensuring a component's rendering/styling remains consistent

@jnm2377
Copy link
Contributor

jnm2377 commented Dec 14, 2021

These are some helpful things I used a lot to refactor the tabs tests to RTL

Examples of RTL tests in our components

Helpful docs

@joshblack
Copy link
Contributor

joshblack commented Dec 15, 2021

Reference for old test code and how it could be updated

Renders as expected

About

This is a common pattern in the codebase that uses describe and it blocks to assert that a component renders or has the appropriate classes.

What does it look like

describe('ProgressIndicatorSkeleton', () => {
  describe('Renders as expected', () => {
    it('should have the expected classes', () => {
      const wrapper = mount(<ProgressIndicatorSkeleton />);
      expect(wrapper.hasClass(`${prefix}--skeleton`)).toEqual(true);
      expect(wrapper.hasClass(`${prefix}--progress`)).toEqual(true);
    });
  });
});

What about this is problematic

Tests that assert if a component renders are not needed as this would be caught by other tests that are asserting what happens as a user interacts with a component. For tests that assert on class names, these are not being tested in Jest and instead would be tested by appearance in VRT.

What to do about it

In most cases, you can remove these tests. If the test is for class names, replace it with a Visual Regression Test.

Component is mounted in an outer scope

About

In parts of our tests, you will see a component mounted outside of an it or beforeEach block. Often times, it will be at the top of a describe block

What does it look like

describe('SomeComponent', () => {
  const wrapper = mount(/* ... */);

  it('should do ...', () => {
    // Interacts with wrapper
  });
});

What about this is problematic

This outermost wrapper is inherently stateful. This means that if your tests are interacting with it, reading data from it, etc that you could run into flaky tests that depend on a specific ordering in order to pass. Overall, this leads to tests that are harder to debug when things go wrong.

What to do about it

You can move the wrapper into the it block directly if there are only several tests. For many tests that use the same setup, move the wrapper into a beforeEach with this pattern:

describe('SomeComponent', () => {
  let wrapper;

  beforeEach(() => {
    wrapper = mount(/* ... */);
  });

  it('should do ...', () => {
    // Interacts with wrapper
  });
});

This will ensure that the component is reset across test runs

Wrapper#setState

About

In enzyme tests, you will come across moments where wrapper.setState is called to change the internal state of a component.

What does it look like

it('should avoid updating state unless actual change in currentIndex is detected', () => {
  wrapper.setProps({ currentIndex: 1 });

  // 👇
  wrapper.setState({ currentIndex: 2 });
  // 👆

  wrapper.setProps({ currentIndex: 1 });
  expect(list.state().currentIndex).toEqual(2);
});

What about this is problematic

wrapper.setState allows you to change the internal state of a component without interacting with it in a way that the component intends for you to interact with it. As a result, you could end up with states that are invalid or unable to derive from a set of user interactions.

What to do about it

Replace wrapper.setState calls with actions. Actions are steps or procedures that a user can perform to change the state of a component.

From our example above, we are changing the state of a progress indicator to point to a different step. Instead of changing the internal state of the component, we could instead use an action to select a progress step as the current step.

userEvent.click(screen.getByLabelText('step-label'));

@tay1orjones
Copy link
Member Author

We need to resurrect #4467 so that jest-dom matchers like toBeVisible will work.

@joshblack
Copy link
Contributor

@tay1orjones PR for it over in: #10302 👍

@tay1orjones

This comment was marked as outdated.

@tay1orjones

This comment was marked as outdated.

@tay1orjones
Copy link
Member Author

Where to place component composition (children) integration tests

  • For components that leverage composition of specific children, place composition integration tests in the parent component's test file.
  • As an example, Breadcrumb is intended to be augmented with BreadcrumbItem children. BreadcrumbItem also has specific conditions for when an OverflowMenu is passed as its children.
    • Tests covering the full composition of Breadcrumb, BreadcrumbItems and the OverflowMenu should be placed in the Breadcrumb-test.js file.
    • BreadcrumbItem tests testing individual props or functionality scoped to only the BreadcrumbItem should be placed in the `BreadcrumbItem-test.js file.

@tay1orjones
Copy link
Member Author

Testing pseudo elements

  • jsdom does not currently support pseudo elements, so any component utilizing pseudo elements for core functionality can't really be fully tested through RTL.
  • We'll need to address these in two ways:
    1. Test what we can in RTL. Typically this will result in asserting a particular element receives a specific class name.
    2. Test this functionality in a browser-based environment via Cypress in an e2e test file. This can be deferred for now in the context of this refactor workstream.

@tay1orjones tay1orjones moved this from 🏗 In Progress to ⏱ Backlog in Design System Jan 19, 2022
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🕵️‍♀️ Triage in Design System Feb 28, 2022
@tay1orjones tay1orjones added this to the 2022 Q2 milestone Mar 15, 2022
@tay1orjones tay1orjones moved this from 🕵️‍♀️ Triage to ⏱ Backlog in Design System May 9, 2022
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🏗 In Progress in Design System May 9, 2022
@joshblack
Copy link
Contributor

joshblack commented May 17, 2022

Test file archetypes

Snapshot tests

A snapshot test is a test file that only tests the structural output of a component. For example, expect(wrapper).toMatchSnapshot()

Example
import React from 'react';
import { BreadcrumbSkeleton } from '../';
import { mount } from 'enzyme';

describe('BreadcrumbSkeleton', () => {
  it('should render', () => {
    const wrapper = mount(<BreadcrumbSkeleton />);
    expect(wrapper).toMatchSnapshot();
  });
});

This test should be refactored to test the following applicable parts of a component:

  • Behavior (can the user interact with this component in some way?)
  • Configuration (can the developer change props on this component in some way?)
Refactor
import React from 'react';
import { BreadcrumbSkeleton } from '../';
import { render } from '@testing-library/react';

describe('BreadcrumbSkeleton', () => {
  it('should support a custom `className` prop on the outermost element', () => {
    const { container } = render(<BreadcrumbSkeleton className="test" />);
    expect(container.firstChild).toHaveClass('test');
  });

  it('should spread additional props on the outermost element', () => {
    const { container } = render(<BreadcrumbSkeleton data-testid="test" />);
    expect(container.firstChild).toHaveAttribute('test');
  });
});

TODO

  • Test files that have a mix of enzyme and testing-library
  • Shallow tests
  • Class assertions
    • Prefix in outer scope
  • Wrapper not in test scope
  • Types of tests
    • Structural (an attribute exists on a specific element, or an element is a specific role)
    • Configuration (what happens when a user specifies a prop)
      • Make sure to test the default value
    • Behavior
    • Variants
      • These are variations of the base component that show up when a specific prop is changed (for example, type)
  • Naming conventions
    • What should you name things in tests
    • What should you avoid as names
      • Individual names
      • Popular names
      • Gendered words

Insights

  • Timing for specific components
    • BreadscrumbSkeleton (sm)
    • Button (md, lg)
  • Classname checks are fine, they're helpful for catching bugs

Checklist

  • Does the component have state?
  • Can a user interact with this in some way?
  • Does the component have props?
    • What element do the props get applied to?
    • Does the prop change the behavior of the component in some way?
  • Structure
    • Imports are sorted
    • Only one describe block in the test suite

  • When visiting a component
    • If a component has a -test.e2e.js file, you can remove it
    • If a test file imports cleanup from @testing-library/react you can remove it
    • If a component has an accessibility describe block that is using aXe and AC, you can remove it

@tay1orjones tay1orjones modified the milestones: 2022 Q4, 2023 Q1 Jan 24, 2023
This was referenced Jan 25, 2023
@kodiakhq kodiakhq bot closed this as completed in #13316 Mar 9, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in Design System Mar 9, 2023
@sstrubberg sstrubberg changed the title Refactor the React test suite to use React Testing Library React 18 Migration: Refactor the React test suite to use React Testing Library Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planning: umbrella Umbrella issues, surfaced in Projects views
Projects
Archived in project
6 participants