feat: add suggested selector for UiAutomator strategy #1394
Conversation
…nto add-suggested-uiautomator-selector
|
We may also show the full selector properly wrapped in a tooltip upon mouseover cell event |
| * @returns {Object.<string, string>} mapping of strategies to selectors | ||
| */ | ||
| export function getComplexSuggestedLocators(path, sourceDoc) { | ||
| export function getComplexSuggestedLocators(path, sourceDoc, isNative, automationName) { |
There was a problem hiding this comment.
mathods having more than 3 arguments are hard to read. Maybe we could combine some args into an object
There was a problem hiding this comment.
perhaps it would even make sense to have a common class that takes all common method params as constructor args, so we don't need to repeat them for each separate function
There was a problem hiding this comment.
Among all 3 related functions (getSuggestedLocators, getSimpleSuggestedLocators, getComplexSuggestedLocators), there is only one common parameter (isNative). 4 parameters isn't that bad in my opinion.
There was a problem hiding this comment.
I would say there is still a lot space for improvements. Ofc, not necessarily as part of this PR.
There was a problem hiding this comment.
Oh certainly - a lot of the Inspector code is very much in need of better code quality.
Unfortunately this does not appear to work in combination with the existing onClick tooltip (plus the text wrapping was quite ugly, although perhaps that can be customized). Another idea I had is to wrap the selector only after a certain width, but this would still make it quite difficult to read it for small window sizes. |
|
Also, while I've been using UiAutomator selectors for some time now, I'm still not sure whether the |
|
|
||
| // BASE CASE #4: Check all attributes and try to find unique ones | ||
| let uiSelector, othersWithAttr, mostUniqueSelector; | ||
| let othersWithAttrMinCount = 99; |
There was a problem hiding this comment.
what is this magic number?
There was a problem hiding this comment.
I guess Number.MAX_SAFE_INTEGER would make the intent more obvious. Another approach would be to skip the initial value here, and assign it in the following comparison if it is undefined.
|
I looked through these and it still isn't fully clear to me. It looks like |
* feat: add base for suggested UiAutomator selector * feat: finish UiAutomator selector implementation * fix: only use UiAutomator strategy in native context * fix: add horizontal scrolling to locators/attributes table * test: add tests for optimal UiAutomator selector * chore: format * chore: use common method for logging errors * chore: address most review comments * chore: make assembly of mostUniqueSelector clearer
This PR adds generation of suggested selectors for the UiAutomator locator strategy, when using the UiAutomator2 driver.
While it does not cover the full breadth of capabilities for this strategy (e.g. no UiScrollable or child selectors), it can still assemble selectors out of 4 attributes (
resource-id,text,content-desc,class), and add indices, which should cover most use cases.It is also oftentimes faster than the
idstrategy... 👀There is another related change in this PR: when reading the suggested locator table, the entry
-android uiautomator (docs)was quite lengthy and difficult to read, therefore the presentation of entries in the selected element tables was adjusted. Until now, some columns had a fixed width, and others took up the remaining width, potentially resulting in linebreaks - this could look very unreadable at small window sizes:Now the rows support horizontal scrolling, while still keeping the previously-fixed-width columns in place:
This does mean that very long selectors (e.g. complex xpaths/class chains) will require scrolling even for large screen sizes. I think this is still acceptable.