refactor: fix linting errors in RTL-migrated test files #6709

Parsium · 2024-05-02T09:23:13Z

Proposed Behaviour

Change file extension of existing test files fully migrated to RTL to *.test.*.
Resolve all linting errors present in the current RTL tests.

Current Behaviour

Both Enzyme and RTL tests are currently stored in files with the *.spec.* extension.
Some of the existing RTL tests do not adhere to the best practices recommended by RTL's maintainers.

Additional Context

This PR is the second part of a two-phased process to introduce linting support for RTL tests, following the first PR, #6715. This division has been implemented to streamline the code review process.

Checklist

src/hooks/__internal__/useMenuKeyboardNavigation/useMenuKeyboardNavigation.test.tsx

Parsium · 2024-05-02T12:23:39Z

src/components/time/time.test.tsx

-      await user.tab();
+    const user = userEvent.setup({
+      advanceTimers: jest.advanceTimersByTime,
+      delay: null,


We should be cautious about using the delay option to userEvent.setup.

By default, user-event adds a small delay between subsequent inputs, like key presses, to simulate how a real user would provide input. Setting delay to null removes it entirely. While this may be useful in some cases, it could potentially mask issues related to timeouts or promises in our code.

nineteen88 · 2024-05-02T15:41:08Z

I see some of the previous spec files that were more or less already RTL have had their filename changed so that RTL linting will occur on them but we should also make a note of these files that have been converted so they don't get missed when we sweep them for refactor. There's still quite a lot of bad practices in those files that we want to get on top of.

Parsium · 2024-05-03T10:49:03Z

Adding DO NOT MERGE label - since this PR depends on the release of #6715 first

Parsium · 2024-05-03T14:06:20Z

I see some of the previous spec files that were more or less already RTL have had their filename changed so that RTL linting will occur on them but we should also make a note of these files that have been converted so they don't get missed when we sweep them for refactor. There's still quite a lot of bad practices in those files that we want to get on top of.

Raised FE-6549 for this 👍🏼

edleeks87

Great work @Parsium some comments from me but mainly non-blocking

src/__internal__/focus-trap/focus-trap.test.tsx

edleeks87 · 2024-05-08T14:59:15Z

src/__internal__/focus-trap/focus-trap.test.tsx

    });

    it("refocuses the container within the trap when flag is set, if the wrapper has no tabindex", () => {
      const { rerender } = render(
        mockComponentToRender({ autoFocus: false, triggerRefocusFlag: false })
      );
      // need to blur the wrapper to remove the tabindex
-      fireEvent.blur(screen.getByTestId(WRAPPER_ID));
+      fireEvent.blur(screen.getByRole("dialog"));


suggestion (non-blocking): we could use user.tab() here to mimic the user behaviour, happy for that to be done in a new ticket though as we're just addressing linting complaints here etc

edleeks87 · 2024-05-08T15:04:26Z

src/components/file-input/file-input.component.tsx

@@ -25,7 +25,7 @@ import useLocale from "../../hooks/__internal__/useLocale";
 export interface FileInputProps
  extends Pick<ValidationProps, "error">,
    Pick<InputProps, "id" | "name" | "required">,
-    TagProps,
+    Omit<TagProps, "data-component">,


Praise: good spot 👏

src/components/flat-table/flat-table-body-draggable/flat-table-body-draggable.test.tsx

edleeks87 · 2024-05-08T15:17:27Z

src/components/navigation-bar/fixed-navigation-bar-context.test.tsx

@@ -35,7 +35,7 @@ describe("FixedNavigationBarContextProvider", () => {
          <MockComponent position={position} orientation="top" offset="20px" />
        );
        const result = screen.getByTestId("output");
-        expect(result.textContent).toBe("");
+        expect(result).toHaveTextContent("");


Praise: nice 👏

src/components/step-flow/step-flow.test.tsx

edleeks87 · 2024-05-08T15:24:11Z

src/components/tooltip/tooltip.test.tsx

-  return renderRTL(
-    <Tooltip {...props} message={message || props.message}>
-      {props.children || children}
+function renderTooltip(props: Partial<TooltipProps> = {}) {


comment (non-blocking): same comment about abstracting the render, happy to sort all this later though 👍

Yeah, this file needs a good review due to the nested structure and styling rules, so I'll leave this for now.

edleeks87 · 2024-05-08T15:25:42Z

src/components/tooltip/tooltip.test.tsx

-            let middleware;
-            if (useFloatingSpy.mock.calls[0][0]?.middleware?.[0]) {
-              middleware = useFloatingSpy.mock.calls[0][0]?.middleware?.[0];
+      describe.each(["top", "bottom", "left", "right"] as const)(


comment (non-blocking): this is the prime example of a test that we need to refactor 😆. Again happy for it to be part of future work

src/hooks/__internal__/useModalAria/useModalAria.test.tsx

nineteen88

Good work James. Can see a lot of very good improvements overall. A few things still don't sit right with me as previously mentioned/discussed but we'll get there!

src/__internal__/focus-trap/focus-trap.test.tsx

nineteen88 · 2024-05-10T00:04:32Z

src/__internal__/focus-trap/focus-trap.test.tsx

-        await shiftTabPress();
-        expect(screen.getByText(BUTTON_TWO)).toHaveFocus();
-      });
+    it("loops focus to the last focusable element, when focus on first radio button shift-tab pressed", async () => {


praise: much improved language/description of the test

Applies to all of those changes

nineteen88 · 2024-05-10T00:12:05Z

src/components/file-input/file-input.test.tsx

    expect(hintText).toBeVisible();
  });

-  it("accepts data tag props", () => {
+  it("renders root container with correct data tag props", () => {


praise: much improved test description

Applies for the others you've made as well

src/components/file-input/file-input.test.tsx

src/components/flat-table/flat-table-body-draggable/flat-table-body-draggable.test.tsx

src/components/step-flow/step-flow.test.tsx

nineteen88 · 2024-05-15T16:00:11Z

src/components/step-flow/step-flow.test.tsx

+    expect(screen.getByText("bar")).toBeVisible();
+  });
+
+  it("when the 'title' prop is passed, the correct element and text renders", () => {


suggestion(non-blocking): language still wrong on this test

Might be worth adding to the re-visit ticket rather than dealing with it now unless there were other changes to be made

nineteen88 · 2024-05-15T16:02:09Z

src/components/step-flow/step-flow.test.tsx

+    }
+  );
+
+  it("when the 'showCloseIcon' prop is true, the correct element renders", () => {


suggestion(non-blocking): language still wrong on this test

Might be worth adding to the re-visit ticket rather than dealing with it now unless there were other changes to be made

carbonci · 2024-05-22T15:29:13Z

🎉 This PR is included in version 135.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Parsium added Pending Review No QA Required No UX QA Required Squad Front End labels May 2, 2024

Parsium self-assigned this May 2, 2024

Parsium force-pushed the linting-rtl-tests branch from 3d7dc89 to 4ffe9af Compare May 2, 2024 09:30

Parsium commented May 2, 2024

View reviewed changes

src/hooks/__internal__/useMenuKeyboardNavigation/useMenuKeyboardNavigation.test.tsx Outdated Show resolved Hide resolved

Parsium force-pushed the linting-rtl-tests branch from 4ffe9af to 085cbd2 Compare May 2, 2024 10:32

edleeks87 self-requested a review May 2, 2024 10:44

Parsium commented May 2, 2024

View reviewed changes

Parsium changed the title ~~chore: set-up linting plugins for jest-dom and React Testing Library~~ chore: address linting errors in existing RTL tests May 3, 2024

Parsium force-pushed the linting-rtl-tests branch from 35d5cc4 to ee1fc8a Compare May 3, 2024 10:23

Parsium changed the title ~~chore: address linting errors in existing RTL tests~~ refactor: address linting errors in existing RTL tests May 3, 2024

Parsium mentioned this pull request May 3, 2024

chore: install linting plugins for RTL and jest-dom #6715

Merged

10 tasks

Parsium force-pushed the linting-rtl-tests branch 2 times, most recently from 406ad3d to 27997f0 Compare May 3, 2024 10:43

Parsium added the DO NOT MERGE label May 3, 2024

edleeks87 removed the DO NOT MERGE label May 8, 2024

edleeks87 reviewed May 8, 2024

View reviewed changes

nineteen88 reviewed May 10, 2024

View reviewed changes

Parsium force-pushed the linting-rtl-tests branch 4 times, most recently from a4e424b to 3e8a87a Compare May 15, 2024 11:28

nineteen88 reviewed May 15, 2024

View reviewed changes

nineteen88 approved these changes May 15, 2024

View reviewed changes

edleeks87 approved these changes May 20, 2024

View reviewed changes

edleeks87 marked this pull request as ready for review May 20, 2024 12:35

edleeks87 requested review from a team as code owners May 20, 2024 12:35

edleeks87 added Passed Review and removed Pending Review labels May 20, 2024

refactor: fix linting errors in RTL-migrated test files

16cc1ac

Parsium force-pushed the linting-rtl-tests branch from 3e8a87a to 16cc1ac Compare May 20, 2024 12:41

Parsium changed the title ~~refactor: address linting errors in existing RTL tests~~ refactor: fix linting errors in RTL-migrated test files May 20, 2024

Parsium merged commit e1c45e2 into master May 20, 2024
21 checks passed

Parsium deleted the linting-rtl-tests branch May 20, 2024 12:53

carbonci added the released label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: fix linting errors in RTL-migrated test files #6709

refactor: fix linting errors in RTL-migrated test files #6709

Parsium commented May 2, 2024 •

edited

Parsium May 2, 2024 •

edited

nineteen88 commented May 2, 2024

Parsium commented May 3, 2024

Parsium commented May 3, 2024

edleeks87 left a comment

edleeks87 May 8, 2024

edleeks87 May 8, 2024

edleeks87 May 8, 2024

edleeks87 May 8, 2024

Parsium May 14, 2024

edleeks87 May 8, 2024

nineteen88 left a comment

nineteen88 May 10, 2024

nineteen88 May 10, 2024

nineteen88 May 15, 2024

nineteen88 May 15, 2024

carbonci commented May 22, 2024

refactor: fix linting errors in RTL-migrated test files #6709

refactor: fix linting errors in RTL-migrated test files #6709

Conversation

Parsium commented May 2, 2024 • edited

Proposed Behaviour

Current Behaviour

Additional Context

Checklist

Parsium May 2, 2024 • edited

Choose a reason for hiding this comment

nineteen88 commented May 2, 2024

Parsium commented May 3, 2024

Parsium commented May 3, 2024

edleeks87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nineteen88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonci commented May 22, 2024

Parsium commented May 2, 2024 •

edited

Parsium May 2, 2024 •

edited