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

convert menu tests to RTL FE-6550 #6744

Merged
merged 9 commits into from
Jun 12, 2024
Merged

convert menu tests to RTL FE-6550 #6744

merged 9 commits into from
Jun 12, 2024

Conversation

edleeks87
Copy link
Contributor

Proposed behaviour

Converts all unit test files to RTL
flattens test structure
removes unnecessary describe blocks

Current behaviour

Most unit tests are enzyme

Checklist

  • Commits follow our style guide
  • Unit tests added or updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Copy link
Contributor

@Parsium Parsium left a comment

Choose a reason for hiding this comment

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

Nice work on this @edleeks87! 👏🏼 Managing the separation of tests for each subcomponent while rewriting them simultaneously must have been quite the task 😅 The test titles now carry a lot more context too 👍🏼

src/components/menu/menu-divider/menu-divider.test.tsx Outdated Show resolved Hide resolved
src/components/menu/menu-divider/menu-divider.test.tsx Outdated Show resolved Hide resolved

afterAll(() => loggerSpy.mockRestore());

test("should not match any item and return undefined when user enters no search string (empty string is passed in)", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: these titles provide much more context behind how this util is used 👍

nineteen88
nineteen88 previously approved these changes May 28, 2024
});
});

test("should not render when closed", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking): be more descriptive with test title

Suggested change
test("should not render when closed", () => {
test("menu should not render when submenu is closed", () => {

expect(screen.queryByRole("menu")).not.toBeInTheDocument();
});

test("should render when mouseover event detected on parent menu item", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking): could prepend all of these tests with submenu to be more descriptive

Suggested change
test("should render when mouseover event detected on parent menu item", async () => {
test("submenu should render when mouseover event detected on parent menu item", async () => {

Or this is when a describe block is quite handy too encapsulate likeminded tests. Very minor nitpick overall though, can just ignore if you want

Parsium
Parsium previously approved these changes Jun 12, 2024
@edleeks87 edleeks87 marked this pull request as ready for review June 12, 2024 10:18
@edleeks87 edleeks87 requested review from a team as code owners June 12, 2024 10:18
@edleeks87 edleeks87 merged commit ad115f4 into master Jun 12, 2024
22 checks passed
@edleeks87 edleeks87 deleted the FE-6550-menu-rtl branch June 12, 2024 10:23
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 138.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants