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.

Sign up for GitHub

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

Jump to bottom

fix: Encapsulate Wcmd within a private wrapper #275

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: Encapsulate Wcmd within a private wrapper #275

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07a72b9
fix: Encapsulate `Wcmd` within a private wrapper
surajk-m Sep 21, 2024
6c19c80
cargo fmt
surajk-m Sep 21, 2024
f3d43da
Add `test_wrap_command` to wrap `WebDriverCommand` in tests
surajk-m Oct 19, 2024
e26d58e
Update `issue_cmd` for boxed and non-boxed command types
surajk-m Oct 28, 2024
0dd402d
Use `<C, CC>` generics in `issue_cmd` with `C: Into<Box<CC>>`
surajk-m Oct 29, 2024
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
24 changes: 14 additions & 10 deletions src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ type Ack = oneshot::Sender<Result<Json, error::CmdError>>;

type Wcmd = WebDriverCommand<webdriver::command::VoidWebDriverExtensionCommand>;

// Wrapper to hide Wcmd from the public API.
#[derive(Debug)]
struct WcmdWrapper(Wcmd);

#[allow(clippy::large_enum_variant)]
#[derive(Debug)]
pub(crate) enum Cmd {
Expand All @@ -43,7 +47,7 @@ pub(crate) enum Cmd {
WebDriver(Box<dyn WebDriverCompatibleCommand + Send>),
}

impl WebDriverCompatibleCommand for Wcmd {
impl WebDriverCompatibleCommand for WcmdWrapper {
/// Helper for determining what URL endpoint to use for various requests.
///
/// This mapping is essentially that of <https://www.w3.org/TR/webdriver/#list-of-endpoints>.
Expand All @@ -52,16 +56,16 @@ impl WebDriverCompatibleCommand for Wcmd {
base_url: &url::Url,
session_id: Option<&str>,
) -> Result<url::Url, url::ParseError> {
if let WebDriverCommand::NewSession(..) = self {
if let WebDriverCommand::NewSession(..) = self.0 {
return base_url.join("session");
}

if let WebDriverCommand::Status = self {
if let WebDriverCommand::Status = self.0 {
return base_url.join("status");
}

let base = { base_url.join(&format!("session/{}/", session_id.as_ref().unwrap()))? };
match self {
match self.0 {
WebDriverCommand::NewSession(..) => unreachable!(),
WebDriverCommand::DeleteSession => unreachable!(),
WebDriverCommand::Get(..) | WebDriverCommand::GetCurrentUrl => base.join("url"),
Expand Down Expand Up @@ -159,7 +163,7 @@ impl WebDriverCompatibleCommand for Wcmd {
let mut body = None;

// but some have a request body
match self {
match self.0 {
WebDriverCommand::NewSession(command::NewSessionParameters::Spec(ref conf)) => {
// TODO: awful hacks
let mut also = String::new();
Expand Down Expand Up @@ -280,20 +284,20 @@ impl WebDriverCompatibleCommand for Wcmd {
}

fn is_new_session(&self) -> bool {
matches!(self, WebDriverCommand::NewSession(..))
matches!(self.0, WebDriverCommand::NewSession(..))
}

fn is_legacy(&self) -> bool {
matches!(
self,
self.0,
WebDriverCommand::NewSession(webdriver::command::NewSessionParameters::Legacy(..)),
)
}
}

impl From<Wcmd> for Cmd {
fn from(o: Wcmd) -> Self {
Cmd::WebDriver(Box::new(o))
Cmd::WebDriver(Box::new(WcmdWrapper(o)))
}
}

Expand Down Expand Up @@ -334,9 +338,9 @@ impl Client {
/// Issue the specified [`WebDriverCompatibleCommand`] to the WebDriver instance.
pub async fn issue_cmd(
&self,
cmd: impl WebDriverCompatibleCommand + Send + 'static,
cmd: impl Into<Box<dyn WebDriverCompatibleCommand + Send + 'static>>,
) -> Result<Json, error::CmdError> {
self.issue(Cmd::WebDriver(Box::new(cmd))).await
self.issue(Cmd::WebDriver(cmd.into())).await
}

pub(crate) fn is_legacy(&self) -> bool {
Expand Down
41 changes: 37 additions & 4 deletions tests/local.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,49 @@
//! Tests that don't make use of external websites.
use crate::common::{other_page_url, sample_page_url};
use fantoccini::wd::TimeoutConfiguration;
use fantoccini::wd::{TimeoutConfiguration, WebDriverCompatibleCommand};
use fantoccini::{error, Client, Locator};
use http_body_util::BodyExt;
use hyper::Method;
use serial_test::serial;
use std::time::Duration;
use url::Url;
use webdriver::command::WebDriverCommand;

mod common;

#[derive(Debug)]
struct GetTitle;

// Implement `WebDriverCompatibleCommand` for `GetTitle`
impl WebDriverCompatibleCommand for GetTitle {
fn endpoint(
&self,
base_url: &url::Url,
session_id: Option<&str>,
) -> Result<url::Url, url::ParseError> {
let base = base_url.join(&format!("session/{}/", session_id.unwrap()))?;
base.join("title")
}

fn method_and_body(&self, _: &url::Url) -> (http::Method, Option<String>) {
(http::Method::GET, None)
}

fn is_new_session(&self) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

Should we add default implementations for the two is_ methods directly on the trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'll make the update in the next commit.

false
}

fn is_legacy(&self) -> bool {
false
}
}

// Implement `Into<Box<dyn WebDriverCompatibleCommand + Send>>` for `GetTitle`
impl Into<Box<dyn WebDriverCompatibleCommand + Send>> for GetTitle {
Copy link
Owner

Choose a reason for hiding this comment

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

I think if you use exactly the version of issur_cmd I proposed, this impl becomes unnecessary

Copy link
Contributor Author

@surajk-m surajk-m Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was trying to use cmd: impl Into<Box<impl WebDriverCompatibleCommand + Send + 'static>> as the parameter type, but I'm getting error[E0666]

error[E0666]: nested `impl Trait` is not allowed
   --> src/session.rs:341:28
    |
341 |         cmd: impl Into<Box<impl WebDriverCompatibleCommand + Send + 'static>>,
    |              --------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--
    |              |             |
    |              |             nested `impl Trait` here
    |              outer `impl Trait`

To avoid using nested impl Trait, we should flatten the types.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that just means you'll need to use "proper" generics instead:

<C, CC> where C: Into<Box<CC>>, CC: WebDriverCompatibleCommand + Send + 'static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, got it. Using <C, CC> with C: Into<Box<CC>> and CC: WebDriverCompatibleCommand + Send + 'static works better. The only overhead is the conversion via .into(), which is a bit more than a direct Box::new call if the type isn’t already boxed. If we only prefer non-boxed type as input, should we manually box cmd with Box::new(cmd) as Box<_> as you suggested?

fn into(self) -> Box<dyn WebDriverCompatibleCommand + Send> {
Box::new(self)
}
}

async fn goto(c: Client, port: u16) -> Result<(), error::CmdError> {
let url = sample_page_url(port);
c.goto(&url).await?;
Expand Down Expand Up @@ -423,9 +456,9 @@ async fn timeouts(c: Client, _: u16) -> Result<(), error::CmdError> {
async fn dynamic_commands(c: Client, port: u16) -> Result<(), error::CmdError> {
let sample_url = sample_page_url(port);
c.goto(&sample_url).await?;
let title = c.issue_cmd(WebDriverCommand::GetTitle).await?;
let title = c.issue_cmd(GetTitle).await?;
assert_eq!(title.as_str(), Some("Sample Page"));
let title = c.issue_cmd(Box::new(WebDriverCommand::GetTitle)).await?;
let title = c.issue_cmd(GetTitle).await?;
assert_eq!(title.as_str(), Some("Sample Page"));
Ok(())
}
Expand Down
Loading