-
Notifications
You must be signed in to change notification settings - Fork 22
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
add tests for the Tabs component #401
add tests for the Tabs component #401
Conversation
cfcae5e
to
65412df
Compare
11ef2bb
to
c83ee65
Compare
9e82f8d
to
540b989
Compare
e9b3da6
to
ebf4250
Compare
); | ||
|
||
expect(screen.getByTestId("tab1-content")).toBeVisible(); | ||
expect(screen.queryByTestId("tab2-content")).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test repeat renders the content of the active tab only
test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we test what happens when there is no defaultActiveKey specified whereas in the above mentioned renders the content of the active tab only
test we specify the defaultActiveKey. So we're testing the same behaviour but in different scenarious(when defaultActiveKey is given and when it's not given). This was my idea when writing the tests, but of course if it is redundant I can delete one of the tests. Let me know your opinion?
expect(screen.getByTestId("tab1-content")).toBeVisible(); | ||
expect(screen.queryByTestId("tab2-content")).not.toBeInTheDocument(); | ||
}); | ||
it("when the key specified is not present in any of the Tabs won't render any Tab's content", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the intended behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it definitely should render any tab's content but that is not the case. I'm not sure what can be done about it?
expect(tabs[1]).toHaveTextContent("Tab 3"); | ||
expect(tabs[2]).toHaveTextContent("Tab 1"); | ||
}); | ||
it("when defaultActiveKey given and tabsOrder differs from the order of children, renders tabs as specified in tabsOrder", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the tabs
rendering order depend on defaultActiveKey
?
Do you want to check that if defaultActiveKey
is present, regardless of the tabsOrder
, it determines which tab's content is rendered?
const tab1 = screen.getByText("Tab 1"); | ||
expect(tab1).toHaveClass("custom-tab-class"); | ||
}); | ||
it("renders the Tab component with the specified title", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test repeat the it("renders tab elements correctly"
above?
Do you want to set className
here, and if so - what for?
ebf4250
to
e2b049e
Compare
Signed-off-by: Haris Bikovic <[email protected]>
e2b049e
to
90d7857
Compare
ce97cef
into
edgehog-device-manager:release-0.7
Related: #390
This PR adds tests for the Tabs component.