-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
test: improve tabs tests #6898
test: improve tabs tests #6898
Conversation
Generate changelog in
|
const TAB = "[role='tab']"; | ||
const TAB_LIST = "[role='tablist']"; | ||
const TAB_PANEL = "[role='tabpanel']"; | ||
const TAB_SELECTOR = `.${Classes.TAB}`; |
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.
No other test grabs by role-- use Classes
like the rest.
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've always heard it's best to select elements in tests as close as possible as to how your users would. Ideally by actual text content, followed by accessibility roles/element types (ex user searches for and clicks a <button>
), and as a last resort something like targeting by an id
or special test-id attribute.
So I don't think we should change these tests just because other tests are selecting by class name.
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.
OK! Changed.
const NUM_TABS = 3; | ||
assert.lengthOf(wrapper.find(TAB_SELECTOR), NUM_TABS); | ||
assert.lengthOf(wrapper.find(TAB_PANEL_SELECTOR), NUM_TABS); | ||
assert.lengthOf(wrapper.find(`.${tabClassName}`).hostNodes(), NUM_TABS * 2); |
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.
Should not have length 9, should have length 6. Need to use hostNodes
to remove the React nodes.
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.
lemme know if you think the hostNodes
change is important, but I'd prefer to not churn how we're targeting elements here
const TAB = "[role='tab']"; | ||
const TAB_LIST = "[role='tablist']"; | ||
const TAB_PANEL = "[role='tabpanel']"; | ||
const TAB_SELECTOR = `.${Classes.TAB}`; |
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've always heard it's best to select elements in tests as close as possible as to how your users would. Ideally by actual text content, followed by accessibility roles/element types (ex user searches for and clicks a <button>
), and as a last resort something like targeting by an id
or special test-id attribute.
So I don't think we should change these tests just because other tests are selecting by class name.
@@ -0,0 +1,5 @@ | |||
type: improvement | |||
fix: | |||
description: '[core] a11y(Tooltip): wrap contents in "tooltip" aria role' |
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.
✂️
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.
lemme know if you think the
hostNodes
change is important###
This PR is a precursor to #6896 , where this change is more beneficial
…vt/blueprint into bvandercar/test/tabs
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.
Thank you for the PR and splitting this out!
I think this is bordering on diff churn without real benefit, but provides a marginally better example for future tests so I'm okay merging. Though I wouldn't want to continue seeing more PRs like this.
Fixes #0000
Checklist