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: convert even more component unit tests using enzyme render to RTL #6974

Merged

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Jul 19, 2023

Summary

This PR converts EuiPopover, EuiPanel, EuiNotification, EuiMarkdownEditor, EuiLoading*, EuiListGroup, EuiKeyPadMenu and EuiHeader unit tests using Enzyme's render() function to RTL render() counterpart to reduce our tech debt.

QA

  • Make sure all unit tests are passing
  • Verify the updated snapshots don't contain any unexpected changes

General checklist

@tkajtoch tkajtoch added testing Issues or PRs that only affect tests - will not need changelog entries skip-changelog tech debt labels Jul 19, 2023
@tkajtoch tkajtoch requested a review from a team July 19, 2023 23:09
@tkajtoch tkajtoch self-assigned this Jul 19, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6974/

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

Comment on lines 110 to 112
// TODO: This keeps re-rendering differently because of fake id creation
// describe('showToolTip', () => {
// test('is rendered', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional] We've since mocked our ID generator to generate a static string - I wonder if we could uncomment this test and remove the TODO? No worries if you'd rather skip it though!

Comment on lines -132 to +135
<h2>
Circumambulate the city
</h2>
>
<span>
Circumambulate the city
</span>
</h3>
Copy link
Contributor

@cee-chen cee-chen Jul 20, 2023

Choose a reason for hiding this comment

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

Ha, thanks for fixing this - I ran into it as well locally when starting the Emotion conversion for this component. The years before EUI had an accessibility dev on the team sure were wild 🤣

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Just a comment about future enhancements for winding down use of Enzyme mount().

@@ -7,9 +7,10 @@
*/

import React, { ReactNode } from 'react';
import { render, mount } from 'enzyme';
import { requiredProps } from '../../test/required_props';
import { mount } from 'enzyme';
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about a future enhancement to refactor tests using Enzyme mount() for keyboard / event behaviors to use RTL? I gave myself a 20 minute window to try refactoring a couple assertions, and this is what I came up with:

// The cue to use `getByText` came from this SO response: https://stackoverflow.com/a/59590889

// Also assumes a few more `import` calls
import { fireEvent, getByText } from '@testing-library/react';

/*
 * ...
 * More tests here
 */

describe('closePopover', () => {
  // Positive assertion
  it('is called when ESC key is hit and the popover is open', () => {
    const closePopoverHandler = jest.fn();

    const { container } = render(
      <EuiPopover
        ownFocus={false}
        id={getId()}
        button={<button />}
        closePopover={closePopoverHandler}
        isOpen
      >
        <p>Popover content</p>
      </EuiPopover>
    );

    fireEvent.keyDown(getByText(container, 'Popover content'), {
      key: 'Escape',
    });
    expect(closePopoverHandler).toBeCalledTimes(1);
  });

  // Negative assertion
  it('is not called when ESC key is hit and the popover is closed', () => {
    const closePopoverHandler = jest.fn();

    const { container } = render(
      <EuiPopover
        id={getId()}
        button={<button>Test</button>}
        closePopover={closePopoverHandler}
        isOpen={false}
      />
    );

    fireEvent.keyDown(getByText(container, 'Test'), {
      key: 'Escape',
    });
    expect(closePopoverHandler).not.toBeCalled();
  });
});

@tkajtoch tkajtoch force-pushed the test/convert-enzyme-render-to-rtl-chunk-2 branch from ac9db9a to 695dc42 Compare July 24, 2023 15:21
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6974/

@tkajtoch tkajtoch force-pushed the test/convert-enzyme-render-to-rtl-chunk-2 branch from 695dc42 to 7cbab7f Compare July 24, 2023 16:05
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Thanks for fixing EuiComboBox as part of this PR! 🚢

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6974/

@tkajtoch tkajtoch merged commit 69c0142 into elastic:main Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog tech debt testing Issues or PRs that only affect tests - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants