Wait Until Page Contains add option Limit#1547
Wait Until Page Contains add option Limit#1547IlfirinPL wants to merge 12 commits intorobotframework:masterfrom
Conversation
wait_until_page_contains_element wait_until_page_does_not_contain_element
|
Please write tests for the change, I think this would be best to be tested by acceptance tests. Perhaps this html page could work for in this case. |
aaltat
left a comment
There was a problem hiding this comment.
There are few things to consider.
| from SeleniumLibrary.base import LibraryComponent, keyword | ||
| from SeleniumLibrary.errors import ElementNotFound | ||
| from SeleniumLibrary.utils import is_noney, secs_to_timestr | ||
| from selenium.common.exceptions import StaleElementReferenceException |
There was a problem hiding this comment.
Could you move this import back to original place, so that it follows pep.
There was a problem hiding this comment.
I move into previous place, but using PyCharm default configuration it want to move it into different place
|
I have added some acceptance test cases, that use additional argument |
aaltat
left a comment
There was a problem hiding this comment.
Sorry that it took long to do the review but was busy with 4.3.0 release. This is a good start, but this enhancement would need more tests and splitting the test as separate tests.
|
|
||
| Wait Until Page Contains Element Limit | ||
| [Documentation] Tests also that format characters (e.g. %c) are handled correctly in error messages | ||
| Wait Until Page Contains Element new div 2 seconds limit=1 |
There was a problem hiding this comment.
- Using locator that matches more than one element could be better test.
- Testing with zero and negative value could be interesting.
| ... Wait Until Page Contains Element %cnon-existent 0.1 seconds | ||
| Run Keyword And Expect Error | ||
| ... Element 'id:ääööåå' did not appear in 100 milliseconds. | ||
| ... Wait Until Page Contains Element id:ääööåå 0.1 seconds |
There was a problem hiding this comment.
- Split test
Wait Until Page Contains Elementkeyword is used to separate tests. - Limit argument is not used in on the error scenarios.
- The current error test are pretty similar and do not in this context provide extra value.
- Write test with custom error message.
- Test for
plural_or_notis missing
There was a problem hiding this comment.
added test cases also rise issue in RF related with plural_or_not
| ... Custom Error ää ÖÖ | ||
| ... Wait Until Page Does Not Contain Element content 0.1 seconds Custom Error ää ÖÖ | ||
|
|
||
| Wait Until Page Does Not Contain Element |
There was a problem hiding this comment.
- Using locator that matches more than one element could be better test.
- Testing with zero value would be interesting.
| Wait Until Page Does Not Contain Element | ||
| [Documentation] Tests also that format characters (e.g. %c) are handled correctly in error messages | ||
| Wait Until Page Does Not Contain Element not_present 2 seconds limit=1 | ||
| Run Keyword And Expect Error |
There was a problem hiding this comment.
- Split test Wait Until Page Contains Element keyword is used to separate tests.
- Limit argument is not used in on the error scenarios.
- The current error test are pretty similar and do not in this context provide extra
- Write test with custom error
- Test for plural_or_not is missing
|
@aaltat |
|
I am having flu and I will take a look when I feel better. |
|
No worries, code will wait :), and flu will pass :) |
|
@aaltat did you maybe had time to look into this pull request ? |
|
Yes, I will try to find time for it in the next weekend. |
|
Closing in favor of #1574 |
#1543
Wait Until Page Contains add option Limit
Wait Until Page Not Contains add option Limit