Retry api (#81)

[Vypo]

This is kind of what I was thinking for #81. It lets you write code pretty much exactly like the example in that issue. Wanted to get your thoughts on it before I went too much further.

[jonhoo]

I'm still not sure I understand the value of having this inside fantoccini itself, rather than having the caller do the tokio wrapping. Fantoccini is a pretty low-level library, and I'd prefer to keep it that way where possible. It's absolutely possible to write more ergonomic APIs on top of it that has more fancy logic, but I think that probably belongs in a separate crate.

[Vypo]

Perhaps I misunderstood the crate's description: a high-level API for programmatically interacting with web pages through WebDriver?

It seems like most browser automation use cases will require retries, and having an API for limiting retries would be super convenient. This design makes all the calls consistent, so clicks/submits/etc could all be retried the same way.

There is the futures-retry crate which I could use for everything that isn't find, I guess.

[jonhoo]

Haha, you're right, the description is basically exactly contrary to what I just wrote above. I think what happened is that I set the description early on in the process, and only later realized that it'd be better for fantoccini to just be pretty direct bindings to webdriver, and then to have other crates provide nicer wrapper APIs on top. I should probably update things to reflect that decision.

I think I still don't quite understand the desire to have this timeout logic in fantoccini itself? It seems more reasonable to me to have a method that retries "forever", and then use "standard" futures combinators, like Timeout, to stop them early?

[Vypo]

I think I still don't quite understand the desire to have this timeout logic in fantoccini itself? It seems more reasonable to me to have a method that retries "forever", and then use "standard" futures combinators, like Timeout, to stop them early?

While I'm personally most interested in getting the timeout stuff merged since it would make fantoccini the only dependency I need for writing browser automation tests, I suppose they aren't too difficult to write in my tests.

Having a unified way to retry any operation, and not just finds, would be pretty slick though. I have a handful of loops that try to click on an element that takes a moment to appear because of JavaScript.

[jonhoo]

since it would make fantoccini the only dependency I need for writing browser automation tests

I'm not sure I follow this? You still need tokio to drive fantoccini, so you must already have that as a dependency? And tokio has tokio::time::Timeout. So I'm not sure I quite follow how this reduces your dependencies?

Having a unified way to retry any operation, and not just finds, would be pretty slick though. I have a handful of loops that try to click on an element that takes a moment to appear because of JavaScript.

Hmm, but isn't that exactly waiting for a find and then doing click? Which I believe the current API already supports.

[Vypo]

I'm not sure I follow this? You still need tokio to drive fantoccini, so you must already have that as a dependency? And tokio has tokio::time::Timeout. So I'm not sure I quite follow how this reduces your dependencies?

Sorry, I don't mean reducing the number of crates I depend on. I guess I'm saying that I'd like to reduce the cognitive load for writing tests, and having tools that solve common testing use cases in one crate helps with that.

I have a handful of loops that try to click on an element that takes a moment to appear because of JavaScript.

Hmm, but isn't that exactly waiting for a find and then doing click? Which I believe the current API already supports.

If the element is in the DOM, but isn't visible, I get an error about not being able to scroll it into view.

[jonhoo]

I guess I'm saying that I'd like to reduce the cognitive load for writing tests, and having tools that solve common testing use cases in one crate helps with that.

Ah, I see what you mean. It's tricky, because while I agree with the general sentiment, in the specific context of futures it sort of means that every crate "should" re-include every combinator. For example, if we wanted to support clicking either of two elements, we'd have to support a select combinator, which already exists in futures. Pulling that in as well seems unfortunate. I think my preference here is to leave combinators to the crates that already define them, and not repeat them in fantoccini.

If the element is in the DOM, but isn't visible, I get an error about not being able to scroll it into view.

I think I might need some code to understand why the current API can't do this, but the proposed one can.

[Vypo]

For example, if we wanted to support clicking either of two elements, we'd have to support a select combinator, which already exists in futures. Pulling that in as well seems unfortunate.

I think you could implement select in an extension trait generically, but doing the same for retrying an operation would require the future be Clone, I think. Maybe that would be an alternative implementation? Either way, you'd need a custom future type.

I think I might need some code to understand why the current API can't do this, but the proposed one can.

I've opened #90 since I think it deserves its own conversation. It could be solved by either adding wait_for methods for more operations, or have all operations return a retryable future:

• click / wait_for_click, find / wait_for_find, etc. or
• click().await / click().retry_forever().await

[Vypo]

Thinking about this a bit more, I think my main complaint about the current fantoccini API is that I can't add convenience methods on an extension trait generically for all operations. If that were possible, I could add retry_until and retry_for easily in my own crate.

How would you feel about this PR if:

• It changed the return type of most operations to something like Retry<Foo> where Retry implements Future;
• Retry only adds the retry_forever function (so no more retry_until or retry_for); and
• It removed the wait_for functions.

[jonhoo]

I have a bit of a hard time imagining exactly what that would look like, but it sounds like a promising idea!