Explicit Waits? about selenium HOT 15 OPEN

I apologize for the delay in response!

I'm definitely open to making changes to help your use-case.

Can you further explain the scenarios in which explicit waits are needed? Also, does the Ruby API handle this better?

I know that client-side binding imposes some bad interactions with this API (see #54). I'd wish that there was better a WebDriver API available for this, but we can probably cover it up at this level.

from selenium.

Thanks for the reply!

So Selenium's concept of an implicit wait, which is the length of time that it'll wait if it can't perform an action immediately, is more of a default time and changing that time repeatedly isn't ideal. Instead, Selenium has explicit waits which can be thought of as one-off overrides.

An actual use case of mine is automating video playback verification: I need to know that playback has started and one way I can do that is waiting for a data field showing the current play-head to be greater than 0.

Ruby's implementation looks like this:

# Create a wait with the desired time.
wait = Selenium::WebDriver::Wait.new(timeout: 60)

# Use wait.until to repeatedly run the provided code block 
# until:
#   It comes back as true. 
# or
#   It fails
# If the block returns true at any time before the wait
# time, then it'll continue on with code execution
wait.until { @driver.find_element(id: 'currentime').text.to_f.positive? }

Here's Ruby's implementation &
here's Java's implementation of it

Ultimately it's a shortcut for manually specifying sleeps and whatnot; it's just really useful.

I'm happy to try taking another crack at it if you're open to it, too.

from selenium.

I'm open to an addition like that. On one hand, I'm hesitant to provide functionality in the main interface that can be easily accomplished by the caller in a simple loop. On the other, I think this is oft-repeated functionality that other language APIs already have.

If you're interested in adding this, can you first make a proposal of what the caller would look like? (The proposal can either be here or take the form of a PR with a test). Do any other Go APIs have a similar function that we can borrow from? Should this be a helper function in the package or an addition to the existing WebDriver type?

Thanks for volunteering!

from selenium.

@minusnine I have a proposal, something like how it's done in Node.js and Java.

1. Add a condition type that is alias to a function that returns func(arguments) bool
2. Add a webdriver.Wait(cond condition, timeout int, interval int) (and also aliases with 1 and 2 arguments that uses default timeout and interval) that runs a for loop, where it checks for the result of the condition and then sleeps for interval, or fails if there's a timeout.
3. Probably write some internal helpers, like .urlIs(s string), titleIs(s string) and so on (not sure which is the better way to implement it though).

That way we would have the ability to write a code this way:

webdriver.Wait(func() func() bool {
    return webdriver.Title() === "Google"
}(), 30 * time.Second)

or something like this:

// For example, implement "webdriver.Conditions()" method that returns object 
// that has some methods that return a condition

// The function inside it would be declared like this:
func TitleIs(title string) func() bool {
    return webdriver.Title() === title
}

// ... and called like this:
webdriver.Wait(webdriver.Conditions().TitleIs("Google"), 30 * time.Second)

I can make a PR on the first 2 points, if you are interested.

from selenium.

Thanks for the concrete suggestion.

Does it really have to be a func() func() bool? I think providing a closure that is a func() bool would be sufficient.

One other minor problem with the example is that errors are not handled. One of the most common wait conditions is for an element to exist, which can be tested using FindElement, and which returns an error if no element is found. (Also, a variety of other errors can happen at any time).

One way to handle this would be to require that the closure be a func() (bool, error). Either the caller or selenium.Wait would have to distinguish whether the error should allow waiting to continue or whether the error is terminal. The API would be that selenium.Wait waits until the bool return parameter is true or an error occurs.

So then selenium.Wait needs to return an error to indicate if the function returned because the condition became true or for another error.

(I admit that Go's error handling dogma in this case of small utility functions is slightly less elegant compared to other exception-based languages).

I'd be happy to continue this discussion over a PR.

As for the chaining suggestion, I'd rather leave that to a separate, higher-level package that provides syntactic sugar.

from selenium.

@minusnine I've thought about func() func() bool being too much for this, but the thing is, I want to have the ability to pass the function as the argument (like in my example):

webdriver.Wait(webdriver.Conditions().TitleIs("Google"), 30 * time.Second)

I want also to implement some basic conditions support, so the user would just pass a function, like in the example above. The problem is, the implementation need to call the function that is passed, and that means that this argument function should return another function that would be called from .Wait(). That's why I proposed to do it this way (btw, in Node.js it's done exactly this way). I don't see a solution how it could be solved in another way, feel free to suggest something if you know how to do it better ;)
(I don't have that much experience with Golang, in fact I started learning it 2 weeks ago, but I have a lot of experience with Node)

As for error handling, I think you're totally right, the selenium.Wait() should definitely return an error, either in the case of timeout or function execution error. I just didn't include it for the case of the simplicity of the example.

About the chaining, what do you mean?

from selenium.

By "chaining", I meant "method chaining". This isn't often used in idiomatic Go, often because this prevents error handling in the intermediate functions, since we lack exceptions and handling multiple return values must be explicit.

I still don't think a func() func() bool is appropriate, but a func() (bool, error) is sufficient, since the func() will close over the WebDriver instance. In your most recent example, the TitleIs function could return a func() (bool, error) if it were to obtain the selenium.WebDriver instance somehow. One way would be to have Conditions take it as an argument:

err := selenium.Wait(selenium.Conditions(wd).TitleIs("Google"), 30 * time.Second)
err := selenium.Wait(selenium.Condition{Driver: wd, Title: "Google"}, 30 * time.Second)

Or add it to the Wait API, where Wait would know how to handle a Condition:

err := selenium.Wait(wd, selenium.Condition{Title: "Google"}, 30 * time.Second)

or to make Wait part of the WebDriver interface:

err := wd.Wait(selenium.Condition{Title: "Google"}, 30 * time.Second)

I also highlight that I don't think there needs to be a Conditions function. I could see having a factory function like TitleIs that produces a type that can be consumed by Wait, but it could also just be a struct. Or, that type produced by the TitleIs function could be a closure of type func() (bool, error), a pure function of type func(d WebDriver) (bool, error), or something else.

I would prefer to not add to the WebDriver interface if possible, since I'd like to get rid of that interface (#72), but this isn't a hard requirement.

For the record, another consideration (which I'm not sold on either) could be to use a context.Context, which is used for deadline and cancellation propagation, instead of a time.Duration:

ctx, done := context.WithTimeout(ctx, 30*time.Second)
defer done()
err := selenium.Wait(ctx, selenium.Condition(wd).TitleIs("Google"))

This provides more flexibility on how waiting is terminated, at the cost of a lot more typing if you aren't already using contexts, which I suspect most WebDriver-oriented tests are not. Plus, there is no way currently to cancel outgoing requests at the moment, but this will change (#71).

A more complicated and more expressive API idea is fine, but I'd like to keep only the basics in this package. This package should handle the low-level boundary between the WebDriver protocol and client operations with it, plus supporting setting up the infrastructure needed to speak the protocol. This package should encourage other packages to build more expressiveness, if that is what is needed.

I think I'm still partial to the simple case, maybe with helper functions for the common scenarios:

package selenium
func Wait(d time.Duration, func() (bool, error)) error { ... }

package condition  // maybe?
func TitleIs(wd selenium.WebDriver, s string) func() (bool, error) { ... }
func Exists(wd selenium.WebDriver, by, value string) func() (bool, error) { ... }
func Visible(wd selenium.WebDriver, by, id string) func() (bool, error) { ... }

package some_test
if err := selenium.Wait(30*time.Second, condition.TitleIs(wd, "Google")); err != nil { ... }

or, shifting wd around:

package selenium
func Wait(wd WebDriver, d time.Duration, func(wd WebDriver) (bool, error)) error { ... }

package condition  // maybe?
func TitleIs(s string) func(wd WebDriver) (bool, error) { ... }
func Exists(by, value string) func(wd WebDriver) (bool, error) { ... }
func Visible(by, id string) func(wd WebDriver) (bool, error) { ... }

package some_test
if err := selenium.Wait(wd, 30*time.Second, condition.TitleIs("Google")); err != nil { ... }

and possibly adding, as you suggested:

type Condition func(wd WebDriver) (bool, error)

which reduces a bit of verbosity in either of the two options above.

from selenium.

@minusnine I like your approach actually.
I think I'll implement selenium.Wait() function that accepts func (wd Webdriver) (bool, error), and create the package that provides the syntax sugar for creating such conditions.

from selenium.

from selenium.

@minusnine yeah, sure, just don't forget about it.
Have a nice trip!

from selenium.

@serge1peshcoff just committed 988bde7, which adds an API for waiting for a condition.

I still think adding a few common (read: used often) condition functions as in the commentary above could be useful.

from selenium.

@minusnine I've actually started working on this already: https://github.com/serge1peshcoff/selenium-go-conditions
For now there are only a few wrappers, since I was waiting for this PR to be merged, but I'm going to implement at least basic conditions helpers/wrappers in the future, like in Node.js/Java/other bindings.

from selenium.

Ok. I'd be open to a PR that adds a new package to this repository for those helper functions, mostly because I'd want to use a subset of them within the existing tests, without a circular dependency. Either way works, though!

from selenium.

@minusnine I suggest that you implement some basic helpers and I would implement some advanced helpers in my library, will that do?

from selenium.

Sounds good.

from selenium.