Playwright stage 1 - add data-testid attribute #7894
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request adds data-testid attributes to various components throughout the codebase, enhancing testability for E2E testing with Playwright. Key changes include:
- Added data-testid attributes to buttons, toggles, inputs, and other interactive elements
- Implemented consistent naming conventions for test IDs across components
- Improved selectability of components for automated testing purposes
- Ensured additions don't affect existing component functionality
These changes align with the goals outlined in issue #6641, specifically addressing the need for better test selection and interaction in automated E2E tests.
30 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| <StyledRowContent | ||
| onClick={handleClick} | ||
| clickable={disabled !== true} | ||
| data-testid={testId} |
There was a problem hiding this comment.
style: Ensure consistent naming convention for test IDs across the application
| }: React.PropsWithChildren<{ | ||
| onClick?: (event: React.MouseEvent<HTMLDivElement>) => void; | ||
| disabled?: boolean; | ||
| testId?: string; |
There was a problem hiding this comment.
style: Add JSDoc comment to explain the purpose of the testId prop
| @@ -135,6 +135,7 @@ export const AttachmentRow = ({ attachment }: { attachment: Attachment }) => { | |||
| onBlur={handleOnBlur} | |||
| autoFocus | |||
| onKeyDown={handleOnKeyDown} | |||
| dataTestId="file-name-input" | |||
There was a problem hiding this comment.
style: Consider using consistent naming: 'testId' vs 'dataTestId'
| @@ -121,6 +122,7 @@ export const Attachments = ({ | |||
| variant="secondary" | |||
| title="Add file" | |||
| onClick={handleUploadFileClick} | |||
| dataTestId="add-file-button" | |||
| ></Button> | |||
There was a problem hiding this comment.
syntax: Remove empty closing tag
| @@ -30,6 +30,7 @@ export const AddTaskButton = ({ | |||
| targetableObjects: activityTargetableObjects, | |||
| }) | |||
| } | |||
| data-testid="add-task-button" | |||
| ></Button> | |||
There was a problem hiding this comment.
style: consider adding a child element or text to the Button component
| @@ -165,6 +165,10 @@ export const RecordDetailRelationSection = ({ | |||
| return ( | |||
| <RecordDetailSection> | |||
| <RecordDetailSectionHeader | |||
| testId={fieldDefinition.label | |||
| .toLowerCase() | |||
| .replace(' ', '-') | |||
There was a problem hiding this comment.
style: Replace ' ' with a regex to handle multiple spaces
| <Button | ||
| title="Add to blocklist" | ||
| type="submit" | ||
| dataTestId="add-blocklist-button" | ||
| /> |
There was a problem hiding this comment.
style: Consider using camelCase for dataTestId to maintain consistency with React naming conventions
| @@ -76,6 +76,7 @@ export const SettingsDataModelFieldIconLabelForm = ({ | |||
| disabled={disabled} | |||
| maxLength={maxLength} | |||
| fullWidth | |||
| dataTestId="field-name-input" | |||
There was a problem hiding this comment.
style: Consider using a consistent naming convention for data-testid attributes across the project
| @@ -84,6 +84,7 @@ export const SettingsDataModelFieldToggle = ({ | |||
| value={value} | |||
| onChange={onChange} | |||
| toggleSize="small" | |||
| testId="relative-datetime-toggle" | |||
There was a problem hiding this comment.
style: Consider using a more specific testId. 'relative-datetime-toggle' might not accurately describe this toggle's purpose in all contexts.
| @@ -146,19 +146,22 @@ export const SettingsDataModelFieldNumberDecimalsInput = ({ | |||
| onClick={handleDecrementCounter} | |||
| Icon={IconMinus} | |||
| disabled={disabled} | |||
| dataTestId="decrement-decimals-button" | |||
There was a problem hiding this comment.
style: Consider using a consistent naming convention for data-testid attributes across the codebase
|
@BOHEUS I'm a bit confused, I thought we decided not to add data-testid attribute (only when we had no other obvious way to interact with a DOM element)? |
|
@charlesBochet that's true and that's what I did here, I added data-testid attribute to all elements which are problematic to locate, I'm aware it can be quite a lot but it's necessary to progress with e2e testing, if that's a problem, I'll try to redo it in a less-invasive way |
| @@ -41,6 +41,10 @@ export const ActionMenuBarEntry = ({ entry }: ActionMenuBarEntryProps) => { | |||
| <StyledButton | |||
| accent={entry.accent ?? 'default'} | |||
| onClick={() => entry.onClick?.()} | |||
| data-testid={entry.label | |||
There was a problem hiding this comment.
for example why do we need this one? can't we select based on entry.label directly (this will select the label but I guess it's still fine to trigger DOM events on parents)
There was a problem hiding this comment.
This one isn't necessary and can be located using text, I wanted to make life a bit easier but I'll refactor it, re-check my changes and delete attributes from places which can be located using text
|
@charlesBochet done |
Related to #6641