-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add Inserter testID #18832
Add Inserter testID #18832
Conversation
Thanks for the PR @jkmassel ! I understand that adding a new prop makes searching for the specific element a bit easier, but it also feels like introducing a second way for accessing elements from tests (the first being via the accessibility labels). Not sure we'd want to have to maintain two ways like that 🤔, especially since the second one provides a shortcut to people to miss properly setting the a11y labels. Instead, can we handle this on the testsuite side, where we could set the test helpers up such that to use the localized strings? @Tug can probably help with finding the proper functions to use the localized strings on the fly. Granted, it might not be trivial to go that way, but I'm thinking it might be more maintainable? |
Thanks for the thoughts @hypest!
This is sort of correct. In the past, we've relied on accessibility labels to retrieve strings, but this isn't a best practice, it's sort of a hack. Accessibility labels are meant to be for users that need accessibility tools to navigate the UI, whereas testIDs are meant to be used by tests. There are a few reasons that accessibility labels make poor identifiers:
In terms of your concern about maintainability, I totally understand your position – we don’t want a crazy proliferation of these all over the place that adds a maintenance burden. Fortunately, that's not necessary – many UI elements can be contextually identified already – I don't see a need to add I've added a test case in order to try to lower the maintenance burden and avoid any accidental breakage of this component, and I think that should be required any time a testID is added. Additionally, as an integration test, I'd like to add UI Tests to Lastly, you mentioned this:
While that might happen, I think that the team should continue as they have to this point – testIDs are really only needed for specific cases where tests can't be built any other way, so developers won't need to add them unless they're running into difficulties with tests. WDYT? |
Is it the idea for this to be used in WPiOS UI tests? What I remember from talking with @JavonDavis , is that we are not using testID because it's not recognised (found?) by the Android UI tests, so we had to fall back forcefully to accessibility labels. I haven't tested this personally, it might be good to give it a chance once again to be sure (things might have changed since the last time this was tested). If this is still the case, we won't be able to add e2e tests in this project to ensure the existence of testIDs since they will fail in Android. |
I see, so we will use them on both WPiOS and WPAndroid native UI Tests. Since it's the best practice, I'd say it's good to add them, but trying to use the testID with our Appium test will probably fail (that was what wasn't working for us on Android). I think it would be good to add a small comment about this, saying that the ID is meant to be used on native tests only, to avoid confusion and not have someone trying it and failing in Appium tests. I'd also say that in "normal gutenberg-mobile" development, we shouldn't be expected to add testIDs, but they should be added on demand when needed by Native tests (like now, as you also mentioned 👍 ) Does it sound good? :) |
Yes, this seems to be a current limitation of Appium.
I'm definitely happy to do this, but where do you figure it would live? IMHO inline with the prop makes sense, but would we leave the message any time it's used?
💯 Thanks for looking at this @etoledom! |
🤔 - Inline makes sense to me too, thinking that it shouldn't be used too often. It will help since we (or I at least) usually look at these previous implementation to If we start using them all over the place, we can re-think the where to put the comment 👍 |
@etoledom – great, thanks! I think this is ready for another review! :) |
Thank you for the update @jkmassel ! Published a PR to test this change on |
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.
CI on wordpress-mobile/gutenberg-mobile#1681 is ✅ so merging now.
Thank you @jkmassel 🙏
Description
Adds a testID to the inserter button on mobile – this allows it to be targeted by automated test tools without having to first localize the string for different device locales.
How has this been tested?
yarn run test-unit:native
andyarn test
. Additionally, this PR's CI checks should be green.gutenberg-mobile
project, using this branch as the submodule reference, referenced in the screenshot below.Screenshots
Types of changes
Checklist: