Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use hyper::error as herror;
use std::error::Error;
use std::fmt;
use std::io::Error as IOError;
use std::time::Duration;
use url::ParseError;
use webdriver::error as wderror;

Expand Down Expand Up @@ -120,6 +121,10 @@ pub enum CmdError {

/// Could not decode a base64 image
ImageDecodeError(::base64::DecodeError),

/// The user specified timeout is reached
/// Contains expired duration
Timeout(Duration),
}

impl CmdError {
Expand Down Expand Up @@ -158,6 +163,7 @@ impl Error for CmdError {
CmdError::NotW3C(..) => "webdriver returned non-conforming response",
CmdError::InvalidArgument(..) => "invalid argument provided",
CmdError::ImageDecodeError(..) => "error decoding image",
CmdError::Timeout(..) => "timeout occured",
}
}

Expand All @@ -171,7 +177,10 @@ impl Error for CmdError {
CmdError::Lost(ref e) => Some(e),
CmdError::Json(ref e) => Some(e),
CmdError::ImageDecodeError(ref e) => Some(e),
CmdError::NotJson(_) | CmdError::NotW3C(_) | CmdError::InvalidArgument(..) => None,
CmdError::NotJson(_)
| CmdError::NotW3C(_)
| CmdError::InvalidArgument(..)
| CmdError::Timeout(..) => None,
}
}
}
Expand All @@ -191,6 +200,7 @@ impl fmt::Display for CmdError {
CmdError::Json(ref e) => write!(f, "{}", e),
CmdError::NotW3C(ref e) => write!(f, "{:?}", e),
CmdError::ImageDecodeError(ref e) => write!(f, "{:?}", e),
CmdError::Timeout(timeout) => write!(f, "timeout was exceeded on {} ms", timeout.as_millis()),
CmdError::InvalidArgument(ref arg, ref msg) => {
write!(f, "Invalid argument `{}`: {}", arg, msg)
}
Expand Down
40 changes: 27 additions & 13 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ pub use hyper::Method;
/// Error types.
pub mod error;

/// Wait logic
pub mod wait;

/// The long-running session future we spawn for multiplexing onto a running WebDriver instance.
mod session;
use crate::session::{Cmd, Session};
Expand Down Expand Up @@ -711,7 +714,7 @@ impl Client {
/// While this currently just spins and yields, it may be more efficient than this in the
/// future. In particular, in time, it may only run `is_ready` again when an event occurs on
/// the page.
pub async fn wait_for<F, FF>(&mut self, mut is_ready: F) -> Result<(), error::CmdError>
pub async fn wait_for<'a, F, FF>(&mut self, mut is_ready: F) -> Result<(), error::CmdError>
where
F: FnMut(&mut Client) -> FF,
FF: Future<Output = Result<bool, error::CmdError>>,
Expand All @@ -727,20 +730,31 @@ impl Client {
/// future. In particular, in time, it may only run `is_ready` again when an event occurs on
/// the page.
pub async fn wait_for_find(&mut self, search: Locator<'_>) -> Result<Element, error::CmdError> {
let s: webdriver::command::LocatorParameters = search.into();
loop {
match self
.by(webdriver::command::LocatorParameters {
using: s.using,
value: s.value.clone(),
})
.await
{
Ok(v) => break Ok(v),
Err(error::CmdError::NoSuchElement(_)) => {}
Err(e) => break Err(e),
fn closure(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your problem likely stems from trying to define a fn closure here. If you just put this code directly into the move |client| {} below, I think you should be fine, and not need the clone?

s: Locator<'_>,
mut client: Client, // TODO: handle lifetime issues with &mut Client type
) -> impl Future<Output = Option<Result<Element, error::CmdError>>> {
let s: webdriver::command::LocatorParameters = s.into();
async move {
match client
.by(webdriver::command::LocatorParameters {
using: s.using,
value: s.value.clone(),
})
.await
{
Ok(v) => Some(Ok(v)),
Err(error::CmdError::NoSuchElement(_)) => None,
Err(e) => Some(Err(e)),
}
}
}

const DEFAULT_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(500);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, by default, this method should never time out. The user asked us to wait indefinitely. If the user wants a timeout, they can do so by wrapping the returned future in a tokio::time::timeout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that you're speaking about changes in wait_for here's you are right.
It's just a method which I chose to demonstrate the Wait 😄 and didn't noticed that I break it's logic 😄

Probably there could be introduced a new type WaitForever which would do just loop. And after we could amalgamate these by a trait Waiter which has a method until.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My argument was more that all you need is the ability to wait forever. If the user wants to stop waiting before the action succeeds, they can just wrap the future in tokio::time::timeout :)

let mut wait = wait::Wait::new(self, DEFAULT_TIMEOUT);
wait.until(move |client| closure(search, client.clone()))
.await
.map_err(|timeout| error::CmdError::Timeout(timeout))?
}

/// Wait for the page to navigate to a new URL before proceeding.
Expand Down
70 changes: 70 additions & 0 deletions src/wait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/// The module contains a wait primitive
use crate::Client;
use std::future::Future;
use std::time::Duration;

const DEFAULT_POOLING_INTERVAL: Duration = Duration::from_millis(500);

///
#[derive(Debug)]
pub struct Wait<'a> {
waiter: DefaultWait<&'a mut Client>,
}

impl<'a> Wait<'a> {
///
pub fn new(client: &'a mut Client, timeout: Duration) -> Self {
Self {
waiter: DefaultWait::new(client, timeout, DEFAULT_POOLING_INTERVAL),
}
}

pub(crate) async fn until<F, FF, R>(&mut self, condition: F) -> Result<R, Duration>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole Wait type only really makes sense if users can define their own waiters, which would mean making this method pub, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was meant to be a base implementation on which could be piled different peaces. But I didn't decide at the time it created for users or for lib itself so it just get a smallest scope I was considering.

It may worth it.

where
F: FnMut(&mut &mut Client) -> FF,
FF: Future<Output = Option<R>>,
{
self.waiter.until(condition).await
}
}

///
#[derive(Debug)]
pub(crate) struct DefaultWait<T> {
input: T,
timeout: Duration,
pooling_interval: Duration,
}

impl<T> DefaultWait<T> {
///
pub(crate) fn new(input: T, timeout: Duration, pooling_interval: Duration) -> Self {
Self {
input,
timeout,
pooling_interval,
}
}

///
pub(crate) async fn until<F, FF, R>(&mut self, mut condition: F) -> Result<R, Duration>
where
F: FnMut(&mut T) -> FF,
FF: Future<Output = Option<R>>,
{
let now = std::time::Instant::now();

loop {
match condition(&mut self.input).await {
Some(result) => return Ok(result),
None => (),
}

if now.elapsed() > self.timeout {
return Err(now.elapsed() - self.timeout)?;
}

tokio::time::delay_for(self.pooling_interval).await;
}
}
}