-
Notifications
You must be signed in to change notification settings - Fork 86
test(action-popover): refine testing suite #7761
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
base: master
Are you sure you want to change the base?
Conversation
| const iconPosition = await icon.boundingBox(); | ||
| const textPosition = await text.boundingBox(); | ||
|
|
||
| if (!iconPosition) throw new Error("Icon isn't visible"); |
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.
question (non-blocking): boundingBox() method returns either an object or null. Could we use expect(iconPosition).not.toBeNull() instead of if (!iconPosition) throw new Error("Icon isn't visible"); to avoid any CI / Lint warnings?
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.
Thanks for pointing this one out Mihai, I've attempted to workaround this but it appears to cause issues with the expect(iconPosition.x).toBeGreaterThan(textPosition.x); and the linter has an issue when we try to ignore it potentially being null with if (iconPosition && textPosition) { expect(iconPosition.x).toBeLessThan(textPosition.x); }
Seems we're doing similar in other areas of the codebase like in the Dialog component so may leave this as is for now.
| ActionPopoverCustom, | ||
| ActionPopoverMenuWithProps, | ||
| ActionPopoverPropsComponent, | ||
| ActionPopoverWithIconsAndNoSubmenus, |
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.
suggetion: I know we've removed a fair bulk of test components here which is super positive, but I think a manual review of the remaining test components could be beneficial, especially:
ActionPopoverWithIconsAndNoSubmenusActionPopoverWithPropsActionPopoverWithSubmenusAndIconsActionPopoverWithDownloadButton
Might be worth seeing where these are used, if the tests are useful etc. It may also be worth seeing if these different variations are captured in a chromatic snapshot, if they aren't, this could be worth exploring too 👍
Proposed behaviour
Removes a number of Playwright tests which are duplicated elsewhere in the testing suite and adds more detail to one of the interaction tests in Chromatic for the
Action PopovercomponentCurrent behaviour
There are a number of tests which exist in more than one testing suite along with flaky tests which can cause the test to fail or take a significant amount of time.
Checklist
d.tsfile added or updated if requiredQA
Additional context
This should significantly reduce the amount of time taken for Playwright test to run in the Action Popover component without losing any coverage in Jest.
Testing instructions
Ensure the Playwright tests pass and Chromatic snapshots are correct.