feat(core): add data-testid to components #536
Conversation
| @@ -43,6 +44,7 @@ export const Drawer = ({ | |||
| <AntdDrawer | |||
| className={styles.drawer} | |||
| closeIcon={closeIcon} | |||
| data-testid={dataTestId} | |||
There was a problem hiding this comment.
This prop does not exist in antd, so it does not work in theory.
I suggest not using props if they are documented
There was a problem hiding this comment.
There are some tests, so if test passes it should work (and we can be notified if antd break this behaviour). We could trust on it?
There was a problem hiding this comment.
yes is as you answer, but why risk to do a DS breaking change if Antd change the behaviour?
I suggest to use a custom node or not use for this component the "data-testid" and use the traditional getByX to getting the node inside your test. Traditionally we use "data-testid" in small cases
| @@ -149,4 +149,12 @@ describe('Modal Component', () => { | |||
|
|
|||
| expect(baseElement).toMatchSnapshot() | |||
| }) | |||
|
|
|||
| test('should get modal by test id', async() => { | |||
| render(<Modal {...props} dataTestId="modal-test" />) | |||
There was a problem hiding this comment.
We cannot trust it to go if it is not documented by antd, because it could quietly break compatibility even if it works
There was a problem hiding this comment.
Same for every component that use data-testid directly on antd
| @@ -82,6 +83,7 @@ export const Menu = ({ | |||
| > | |||
| <AntMenu | |||
| className={menuClassNames} | |||
| data-testid={dataTestId} | |||
| }) | ||
|
|
||
| test('should get segment control by test id', () => { | ||
| render(<SegmentedControl {...props} dataTestId="segment-control-test" />) |
There was a problem hiding this comment.
I don't think we need all of this data-testid.
Is there a specific component that has some problems with testing?
I imagine that most components permit getting them easily with "getByRole" {name}.
Description
This PR aims to add the
dataTestIdprop to almost every component.I didn't add the prop to the components that I thought it was not necessary but it can be discussed and I'm not quite sure about the Icon component.
The components with the new prop are:
The components without the prop are:
Addressed issue
Closes #372
Checklist