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 3 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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ license = "MIT OR Apache-2.0"
default = ["native-tls"]
native-tls = ["hyper-tls", "openssl"]
rustls-tls = ["hyper-rustls"]
test_helpers = []

[dependencies]
webdriver = { version = "0.50", default-features = false }
Expand All @@ -47,6 +48,7 @@ tokio = { version = "1", features = ["full"] }
hyper = { version = "1.1.0", features = ["server"] }
hyper-util = { version = "0.1.3", features = ["server", "http1"] }
serial_test = "3.0"
fantoccini = { path = "../fantoccini", features = ["test_helpers"] }
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look right — how can fantoccini depend on itself with a path of ..?

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 for the feedback. I’ve removed the path reference, it was a misconfiguration introduced during testing test_helpers feature.


# for minimal-versions
[target.'cfg(any())'.dependencies]
Expand Down
3 changes: 3 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,3 +266,6 @@ pub mod wait;
pub mod wd;
#[doc(inline)]
pub use wd::Locator;

#[cfg(any(test, feature = "test_helpers"))]
pub use crate::session::test_wrap_command;
36 changes: 26 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,22 @@ 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);

/// Wraps a WebDriverCommand inside a WcmdWrapper and returns it as a
/// Box<dyn WebDriverCompatibleCommand>.
///
/// This helper function is intended for use in tests where internal
/// WebDriver commands need to be wrapped in the private `WcmdWrapper`
#[cfg(any(test, feature = "test_helpers"))]
pub fn test_wrap_command(
cmd: WebDriverCommand<webdriver::command::VoidWebDriverExtensionCommand>,
) -> Box<dyn WebDriverCompatibleCommand + Send + 'static> {
Box::new(WcmdWrapper(cmd))
}

#[allow(clippy::large_enum_variant)]
#[derive(Debug)]
pub(crate) enum Cmd {
Expand All @@ -43,7 +59,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 +68,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 +175,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 +296,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 +350,9 @@ impl Client {
/// Issue the specified [`WebDriverCompatibleCommand`] to the WebDriver instance.
pub async fn issue_cmd(
&self,
cmd: impl WebDriverCompatibleCommand + Send + 'static,
cmd: Box<dyn WebDriverCompatibleCommand + Send + 'static>,
Copy link
Owner

Choose a reason for hiding this comment

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

How about taking

Suggested change
cmd: Box<dyn WebDriverCompatibleCommand + Send + 'static>,
cmd: impl Into<Box<impl WebDriverCompatibleCommand + Send + 'static>>,

and then below doing:

Cmd::WebDriver(Box::from(cmd) as Box<_>)

so that callers can also provide a non-boxed type for convenience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I’ve updated issue_cmd to use impl Into<Box<impl WebDriverCompatibleCommand + Send + 'static>>

) -> Result<Json, error::CmdError> {
self.issue(Cmd::WebDriver(Box::new(cmd))).await
self.issue(Cmd::WebDriver(cmd)).await
}

pub(crate) fn is_legacy(&self) -> bool {
Expand Down
10 changes: 7 additions & 3 deletions tests/local.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Tests that don't make use of external websites.
use crate::common::{other_page_url, sample_page_url};
use fantoccini::wd::TimeoutConfiguration;
use fantoccini::{error, Client, Locator};
use fantoccini::{error, test_wrap_command, Client, Locator};
use http_body_util::BodyExt;
use hyper::Method;
use serial_test::serial;
Expand Down Expand Up @@ -423,9 +423,13 @@ 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(test_wrap_command(WebDriverCommand::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(test_wrap_command(WebDriverCommand::GetTitle))
.await?;
assert_eq!(title.as_str(), Some("Sample Page"));
Ok(())
}
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 what we're really looking to demonstrate here is that users can provide their own custom commands via fantoccini. That command doesn't even have to come from the webdriver crate. So how about we do this:

struct GetTitle;
impl WebDriverCompatibleCommand for GetTitle {
  // ...
}

and then check that it's possible to pass GetTitle and Box<GetTitle> to issue_cmd? Then you can get rid of test_wrap_command entirely (and the test helper feature).

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, this change removes the need for test_wrap_command and demonstrates issue_cmd can accept both boxed and non-boxed types.

Expand Down
Loading