Allow locator strategy of xpath= for matching xpath keywords. #527
Conversation
|
Should the _element.py line 624 Regardless of the argument name the argument name should match in lines 602 and 624. Other than that, look's good to me. |
|
|
||
| def xpath_should_match_x_times(self, xpath, expected_xpath_count, message='', loglevel='INFO'): | ||
| """Verifies that the page contains the given number of elements located by the given `xpath`. | ||
| def xpath_should_match_x_times(self, locator, expected_xpath_count, message='', loglevel='INFO'): |
There was a problem hiding this comment.
I left the argument in Get Matching Xpath Count as xpath but changed the argument in Xpath Should Match X Times to locator. Here I changed it so one wouldn't get an Robot Framework positional argument error. If, as in one of the acceptance tests, you wrote
Xpath Should Match X Times xpath=//input[@type="text"] ${1}and the argument was xpath, Robot Framework would think you were placing a named/optional argument, xpath, before a positional argument of expected_xpath_count.
They don't have to be the same. For Get Matching Xpath Count since there is only one argument, which I thought xpath was more descriptive, I left it. To avoid the unintentional error I described above I changed the argument to locator under Xpath Should Match X Times.
There was a problem hiding this comment.
If you want to avoid an issue with the named argument processing of RF, I would suggest calling the argument xpath_, rather than locator.
Adding a trailing underscore is the PEP 8 suggestion for when there is a conflict with Python Keywords.
BuiltIn's Sleep and Get Time keywords use a trailing underscore on the time argument.
|
@aaltat Not sure about
Line 624 doesn't match up to the code changes and doesn't contain xpath nor locator? What do you mean here? |
| if xpath.startswith('//'): | ||
| xpath = "xpath=" + xpath | ||
|
|
||
| if xpath.startswith('xpath='): |
There was a problem hiding this comment.
The detection of the xpath locator prefix is both case- and space-sensitive. This is not consistent with S2L.
| | Xpath Should Match X Times | xpath=//div[@id='sales-pop'] | 1 | ||
|
|
||
| See `Page Should Contain Element` for explanation about `message` and | ||
| `loglevel` arguments. | ||
| """ | ||
| actual_xpath_count = len(self._element_find("xpath=" + xpath, False, False)) | ||
| if not xpath_.lower().lstrip().startswith('xpath='): |
There was a problem hiding this comment.
I think this is good enough. But wanted to point out that the prefixes as seen in UT test_find_with_sloppy_prefix wouldn't all work here. lstrip would not remove spaces in the prefix or before the equals sign. ElementFinder._parse_locator partitions on = and strips both parts. The prefix is not space-sensitive because it does a lookup in a dict that ignores space.
I think this is good enough but wanted to point out that its still not the same.
There was a problem hiding this comment.
Just to think we are that close to support locators with the strategy in, I'm getting sick of removing it from my locator vars to verify the correct number of elements are present 😄
All joking aside, what is needed to get the PR merged? This will be a really helpful improvement; thanks a lot in advance 👍
|
We are planning some architecture changes. This architecture change will most likely change the current code base in such way that the PR will cause conflicts and therefore cannot be merged directly to the master. We don't have a excat date for the architecture change, but most likely it will happen in early next year. If you want this PR to be merged in the master before the architecture change, now is the time to improve the PR and get it merged to the master. |
Fixes #526.