Skip to content
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

[EuiDataGrid][EuiSelectable] Add data-test attributes for more explicit E2E testing #5643

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Feb 16, 2022

Summary

While fixing and updating several Kibana CI tests in elastic/kibana#125023, it became clear that we should add a few more data-test attributes to a few of our components to make several E2E selectors more deliberate.

This will hopefully be either a very small backport or patch release that we can then add to our Kibana upgrade.

Checklist

- Now that it no longer has a unique className, it isn't easy for E2E tests to hook into it
- to make the transition/check for consuming E2E tests that check for selection state to use a data-attribute since we recently switched from aria-selected to aria-checked
Comment on lines +248 to 249
data-test-selected={isChecked} // Whether the item is checked/selected
aria-checked={role === 'option' ? isChecked : undefined} // Whether the item is "checked"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly tested this on local with a random option set to { role: 'test' }and this data attribute is actually needed for that use case, because it 100% always reflects the visual checked icon/state of the item whereas aria-checked will no longer be present if role is not option.

TBF I can't really imagine a scenario where a consumer sets a custom role and still wants it to behave like an option, but we allow it in code, so 🤷‍♀️

CHANGELOG.md Outdated
@@ -1,6 +1,6 @@
## [`main`](https://github.com/elastic/eui/tree/main)

No public interface changes since `48.1.0`.
- Added two new `data-test` attributes to `EuiDataGrid` and `EuiSelectable` for more explicit E2E testing ([#5643](https://github.com/elastic/eui/pull/5643))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this go under a **Bug fixes** header if we want this to be a patch release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to just make it a patch during release. They're test-only changes.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshot changes LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
@cee-chen cee-chen enabled auto-merge (squash) February 16, 2022 17:18
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5643/

@cee-chen cee-chen merged commit 2364046 into elastic:main Feb 16, 2022
@cee-chen cee-chen deleted the data-attr-backport branch February 16, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants