Skip to content
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

Improvement: the retry argument of find_element #252

Open
pmontrasio opened this issue Dec 1, 2020 · 0 comments
Open

Improvement: the retry argument of find_element #252

pmontrasio opened this issue Dec 1, 2020 · 0 comments

Comments

@pmontrasio
Copy link

pmontrasio commented Dec 1, 2020

Context: find_element calls search_element, which callsmake_req, which calls send_req which either immediately succeeds or calls make_retry every 250 ms (hardcoded value) for at most retry retries, where retry is the optional argument of find_element. It defaults to 5. It works but it could be improved.

Problem: the 7 in find_element(:css, "#foo", 7) is not immediately related with time and yet (sadly) it's common to see some find_element preceded by something like :time.sleep(500) because the developer finds out that it gives time to the front end to display #foo.

There are several reasons why that retry argument is a problem or at least leads to problems.

  1. Those 500 are ms, that 7 is either a dimensionless number or expressed in units of 250 ms, which is an uncommon unit. Common units are tenths, hundredths and thousandths of second (ms.)
  2. One doesn't know that 7 is equivalent to adding a 500 ms sleep unless one reads the code. I didn't find it at https://hexdocs.pm/hound/Hound.Helpers.Page.html#find_element/3 and that could be a separate issue. And even if a developer knows it the next one to read the code could be puzzled. A sleep(10000) doesn't puzzle anybody in any programming language. (Of course asking for 7 retries is a better solution because there isn't a fixed wait before searching for the element)
  3. If one day Hound changes the internal retry interval to a different value, all the retry arguments in a code base might need to be changed.

Solution: add another version of find_element such as def find_element(strategy, selector, %{timeout: timeout}) or similar.

That would divide timeout (ms) by the hardcoded value (250 now) and call the current version with retry set to the result of the division.
In this way we would use an argument expressed in units of time and explicitly control the amount of time find_element waits before returning.
Maybe that would help removing all those :time.sleep from tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant