refactor(tabs): convert tests to use react-testing-library #6988
Conversation
robinzigmond
force-pushed
the
FE-6635-react-tabbing-library
branch
from
e8a49f6 to
491170f
Compare
| }); | ||
|
|
||
| // coverage | ||
| it("renders the correct styles when size is `large`, position is `left`, a warning is present and a custom layout is used", () => { |
There was a problem hiding this comment.
nitpick:
| it("renders the correct styles when size is `large`, position is `left`, a warning is present and a custom layout is used", () => { | |
| test("renders the correct styles when size is `large`, position is `left`, a warning is present and a custom layout is used", () => { |
| @@ -182,6 +182,8 @@ const Tabs = ({ | |||
|
|
|||
| /** Handles the changing of tabs with the mouse */ | |||
| const handleTabClick = (ev: React.MouseEvent<HTMLElement>) => { | |||
| // istanbul ignore if | |||
| // (code doesn't seem to be ever reached, the keydown event on TabTitle calls stopPropagation) | |||
There was a problem hiding this comment.
question: if this line of code is unreachable, should we open a ticket to investigate if it's worth removing it?
There was a problem hiding this comment.
now done, thanks (I won't link to Jira here but I've put the ticket number in the comment in my code changes). I also realised the explanation I'd given in this comment of why it's apparently unreachable wasn't correct, the ticket has a more detailed description of why (in my opinion) it can't be reached.
| <TabTitle title="example title" onClick={() => {}} onKeyDown={() => {}} /> | ||
| ); | ||
|
|
||
| const tabTitle = screen.getByRole("tab"); |
There was a problem hiding this comment.
suggestion (non-blocking): you could combine the query and assertion a bit like below
expect(screen.getByRole("tab", {name: "example title"})).toBeVisible();
There was a problem hiding this comment.
maybe, I don't hate this but I'm not convinced it's an improvement. An accessible name can come from various places (eg an aria-label attribute) but to check it renders the title prop as visibe text - which is what the test description says and imo I think it's worth testing - the toHaveTextContent assertion is needed.
There was a problem hiding this comment.
That's true but you aren't passing anything like aria-label here so it's an acceptable shortcut given how manny lines of code this file already has imo. I'm happy for it to be left as is as it's not critical
| ); | ||
|
|
||
| const tabTitle = screen.getByRole("tab"); | ||
| expect(tabTitle).toHaveClass("class1"); |
There was a problem hiding this comment.
suggestion (non-blocking):
| expect(tabTitle).toHaveClass("class1"); | |
| expect(tabTitle).toHaveClass("class1", "class2"); |
There was a problem hiding this comment.
cool, thanks for the suggestion - I hadn't realised toHaveClass could take multiple arguments
| expect(screen.getByRole("tab")).not.toHaveAttribute("aria-selected"); | ||
| }); | ||
|
|
||
| test("`siblings` are rendered after the title when `titlePosition` is `before`", () => { |
There was a problem hiding this comment.
suggestion (non-blocking): As we can't assert the order (fine as we use chromatic anyway) maybe it's better to remove that part from the description for this and the two below
| test("`siblings` are rendered after the title when `titlePosition` is `before`", () => { | |
| test("`siblings` are rendered when `titlePosition` is `before`", () => { |
There was a problem hiding this comment.
pretty sure I did assert the order - by the combined (parent element) text content being "foobar" as opposed to "barfoo" (compare with the next test). So I don't agree with this - this is the important part of the test for me and if I were to change the test description in the way you suggest (which I'd rather not) I'd remove those particular assertions
There was a problem hiding this comment.
Ah so you did I skimmed the last line of the test my bad
src/components/tabs/tabs.test.tsx
Outdated
| </Tabs> | ||
| ); | ||
|
|
||
| expect(screen.getByTestId("tabs")).toHaveClass("custom-class-1"); |
There was a problem hiding this comment.
| expect(screen.getByTestId("tabs")).toHaveClass("custom-class-1"); | |
| expect(screen.getByTestId("tabs")).toHaveClass("custom-class-1", "custom-class-2")); |
robinzigmond
force-pushed
the
FE-6635-react-tabbing-library
branch
from
93d18b2 to
645fc9b
Compare
robinzigmond
force-pushed
the
FE-6635-react-tabbing-library
branch
from
645fc9b to
15e9a2d
Compare
|
🎉 This PR is included in version 142.13.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Proposed behaviour
Tabs component, and all subcomponents, should have tests written using React Testing Library
Current behaviour
Tests use Enzyme
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
Requires 1 QA approval due to the addition of a Playwright test (which was previously a unit tests, with a "TODO" comment to make it a Playwright test instead - I have done this in the course of refactoring the tests)