-
Notifications
You must be signed in to change notification settings - Fork 201
fix: Ensure toggle button test utils only find toggle buttons #3868
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
Conversation
797b0f5
to
6d2013e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3868 +/- ##
==========================================
- Coverage 97.15% 97.15% -0.01%
==========================================
Files 854 854
Lines 24960 24959 -1
Branches 8792 8794 +2
==========================================
- Hits 24251 24250 -1
Misses 660 660
Partials 49 49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6d2013e
to
2c8eec2
Compare
b4c3b23
to
458622e
Compare
src/test-utils/dom/button/index.ts
Outdated
export default class ButtonWrapper extends ComponentWrapper<HTMLButtonElement> { | ||
static rootSelector: string = styles.button; | ||
static rootSelector: string = buttonTestUtilsStyles.root; | ||
static legacyRootSelector: string = 'awsui_button_vjswe'; |
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.
I would suggest writing a unit test for this scenario - just copy the HTML markup for the legacy selector and verify it still works correctly. Yes, this scenario is technically covered in this repository: https://github.com/cloudscape-design/test-utils/blob/c7200f200913812a551c8286bb4d1dccd5087565/src/core/test/selectors.test.ts#L116
The difference is that this selector skips the last hash part, which is why I still recommend doing it this way
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.
good call, turns out what I had didn't actually work! updated
export default class ToggleButtonWrapper extends ButtonWrapper { | ||
static rootSelector: string = buttonStyles.button; | ||
static rootSelector: string = testStyles.root; | ||
static legacyRootSelector: string = ''; |
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 empty selector needed?
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.
yes, because extends ButtonWrapper
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.
Understood. Great that you handle it, but also important learning to check all descendants when using this property on other wrappers
No more questions from me on this PR, removing myself
30ca4c9
to
8a3f88b
Compare
|
||
describe('legacy selectors', () => { | ||
test( | ||
'finds legacy and new buttons', |
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.
[non-blocking] I'd suggest to turn this test and its page to a unit test since it's faster and less resource-intensive than running it in the browser
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.
see below, unit and integ tests handle selectors a bit differently, so this is deliberate
export default class ButtonWrapper extends ComponentWrapper<HTMLButtonElement> { | ||
static rootSelector: string = styles.button; | ||
static rootSelector: string = buttonTestUtilsStyles.root; | ||
static legacyRootSelector: string = 'awsui_button_vjswe_t8nlg_157'; |
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.
[non-blocking just curious] doesn't the hash at the end get regenerated with every build? I checked both the cloudWatch and lambda consoles though, and it looks like they both use this selector for their buttons
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.
Yes the last hash segment changes regularly. That's why I added the test as an integ test: the selectors
utils trim these "full" class selectors and turn them into [class*=...]
instead, so that they are insensitive to the 2nd part of the hash. dom
utils use the class as-is, as typically the dom test utils aren't used in multi-version environments.
Description
Re-run of #3816 with adjusted implementation for better cross-version compatibility
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.