fix: generate suggested locators upon selecting element instead of upon retrieving source #1389
Conversation
…nto optimize-locator-gen
|
I love the optimization idea itself I would say the whole current implementation of location heuristics might/should be refactored and nicely isolated to separate logical units (classes and methods). It is just a pain to read long methods containing hundreds of lines of code, especially without proper understanding of the background logic. Would it make sense for you @eglitise if the logic for each locator type would be isolated into classes with shorter methods and cleaner hierarchy? |
I'm not sure about separating each locator type into its own module/class - really it's only xpath that has multiple methods. But separating all locator generation-specific methods from e.g. the source translation methods sounds very reasonable. |
There was a problem hiding this comment.
lgtm, overall. Thank you for this change, I tried to move the timing into select element, but did not have much time to understand and track the implementation... I tested with large source like I attached in the original issue.
I hope this will improve inspector users' experience after the page source loading
| * @param {string} selectedElement element node in JSON format | ||
| * @param {string} sourceXML | ||
| * @param {boolean} isNative whether native context is active | ||
| * @returns {Object} mapping of strategies to selectors |
There was a problem hiding this comment.
I believe we could have a better typedef for the returned result
There was a problem hiding this comment.
I couldn't find a definitive answer for how to describe an array of tuples, but hopefully Array<[string, string]> is valid syntax.
| /** | ||
| * Get suggested selectors for simple locator strategies (which match a specific attribute) | ||
| * | ||
| * @param {Object} attributes element attributes |
There was a problem hiding this comment.
this typedef could be improved
| * @param {Object} attributes element attributes | ||
| * @param {Document} sourceDoc | ||
| * @param {boolean} isNative whether native context is active | ||
| * @returns {Object} mapping of strategies to selectors |
| * | ||
| * @param {string} path a dot-separated string of indices | ||
| * @param {Document} sourceDoc | ||
| * @returns {Object} mapping of strategies to selectors |
|
Didn't make it here in time to do a full review, but this is an extremely useful improvement! Thank you |
…on retrieving source (appium#1389) * add slight caching for retrieved locator map * remove unused third param for getOptimalXPath calls * add comment to point to next actions * WIP moving xpath calc to getLocators * chore: remove unnecessary filter call * chore: move findElementByPath to utils * chore: remove unused state vars * chore: revert to object for getLocators response * chore: fix getLocators parameter * chore: move getLocators to utils file * start work on the complex locator detection * chore: rename various source variables for clarity * chore: move DOM conversion from areAttrAndValueUnique * chore: add function to retrieve DOM node using path * chore: update docstrings * chore: save DOMParser in a const * fix: calculate optimal xpath only on demand * chore: do not pass local consts as parameters * fix: enable predicate string for XCUIElementTypeApplication * chore: adjust comments for getOptimalClassChain * fix: calculate optimal class chain/predicate only on demand * chore: define NATIVE_APP only once * chore: update more var names * chore: remove dedicated isIOS variable * fix: skip accessibility id strategy in webviews * chore: simplify childNodesOf * chore: lint & format * test: update unit tests * test: add tests for accessibility id and class name * test: add tests for path lookup methods * test: add tests for class chain/predicate + format * chore: split utils file * chore: address review comments * chore: update typedefs * chore: update typedef again
This is a sizeable refactor of the logic behind generating suggested locators.
Until now, locator generation was invoked right after retrieving the app source, and it was run for every single node in the XML source, which is entirely unnecessary. For smaller XMLs, this delay was barely noticeable, but bigger XMLs could show a delay of several seconds or even more. iOS was also affected more than Android, due to the generation of class chains and predicate strings.
With this change, locator generation is invoked only upon selecting an element in an already-retrieved source, and it is run only for that element. In my experience it is also fast enough to not even be noticeable.
As a result of this change, the delay after refreshing the app source is reduced. I didn't do extensive testing, but for one sample app, the delay on Android was reduced by around 100ms, and on iOS by an entire second.
Fixes #1359.
Notable changes:
selectedElementVariableNameandelectedElementVariableTypefrom Redux storageutil.js(which already contained methods for retrieving complex locators)findElementByPathtoutil.js(it did not belong in the reducers file)sourceto clarify whether it is XML, JSON or DocumentDOMParserinstance instead of creating new ones for every conversionfindDOMNodeByPathmethod for finding a Node by path, similarly tofindElementByPathfor a JSON elementfindDOMNodeByPath,findJSONElementByPath,getOptimalClassChain,getOptimalPredicateStringXCUIElementTypeApplication(which is a valid type). Not really sure why class chain did not work, so that was left disabled.