fix: Encapsulate `Wcmd` within a private wrapper #275

surajk-m · 2024-09-21T22:13:03Z

Closes #225

The change encapsulates Wcmd within a private wrapper.

codecov · 2024-09-21T22:25:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.33%. Comparing base (29bbfa7) to head (0dd402d).
Report is 1 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
src/session.rs	`73.53% <100.00%> (-0.05%)`	⬇️

... and 3 files with indirect coverage changes

jonhoo

Thanks for taking this on!

jonhoo · 2024-10-19T09:13:36Z

src/session.rs

-        cmd: impl WebDriverCompatibleCommand + Send + 'static,
-    ) -> Result<Json, error::CmdError> {
-        self.issue(Cmd::WebDriver(Box::new(cmd))).await
+    pub async fn issue_cmd(&self, cmd: Wcmd) -> Result<Json, error::CmdError> {


I think this specifically needs to not change, otherwise we're putting Wcmd back into the public API here.

Apologies for the oversight. I'll revise it to use the WebDriverCompatibleCommand trait.

jonhoo · 2024-10-27T09:47:39Z

Cargo.toml

@@ -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"] }


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

Thanks for the feedback. I’ve removed the path reference, it was a misconfiguration introduced during testing test_helpers feature.

jonhoo · 2024-10-27T09:52:02Z

src/session.rs

@@ -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>,


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?

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

jonhoo · 2024-10-27T09:54:57Z

tests/local.rs

 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(())
 }


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).

Thanks, this change removes the need for test_wrap_command and demonstrates issue_cmd can accept both boxed and non-boxed types.

- Remove `test_wrap_command` and `test_helpers` feature - Refactor `issue_cmd` to accept impl `Into<Box<dyn WebDriverCompatibleCommand + Send + static>>` - Introduce a `GetTitle` struct that implements `WebDriverCompatibleCommand`

jonhoo · 2024-10-28T07:50:12Z

tests/local.rs

+}
+
+// Implement `Into<Box<dyn WebDriverCompatibleCommand + Send>>` for `GetTitle`
+impl Into<Box<dyn WebDriverCompatibleCommand + Send>> for GetTitle {


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

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.

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

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

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?

jonhoo · 2024-10-28T07:50:51Z

tests/local.rs

+        (http::Method::GET, None)
+    }
+
+    fn is_new_session(&self) -> bool {


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

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

jonhoo · 2024-10-28T20:41:27Z

Just a heads up that I'll be on holiday for the next three weeks, so won't have a chance to land this until I get back :)

surajk-m · 2024-10-29T07:57:50Z

No worries! Wishing you a great holiday. We’ll pick this up when you're back. 😊 Safe travels!

surajk-m added 2 commits September 22, 2024 03:32

fix: Encapsulate Wcmd within a private wrapper

07a72b9

cargo fmt

6c19c80

jonhoo requested changes Oct 19, 2024

View reviewed changes

Add test_wrap_command to wrap WebDriverCommand in tests

f3d43da

jonhoo reviewed Oct 27, 2024

View reviewed changes

Update issue_cmd for boxed and non-boxed command types

e26d58e

- Remove `test_wrap_command` and `test_helpers` feature - Refactor `issue_cmd` to accept impl `Into<Box<dyn WebDriverCompatibleCommand + Send + static>>` - Introduce a `GetTitle` struct that implements `WebDriverCompatibleCommand`

jonhoo reviewed Oct 28, 2024

View reviewed changes

Use <C, CC> generics in issue_cmd with C: Into<Box<CC>>

0dd402d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Encapsulate `Wcmd` within a private wrapper #275

fix: Encapsulate `Wcmd` within a private wrapper #275

surajk-m commented Sep 21, 2024

codecov bot commented Sep 21, 2024 •

edited

Loading

jonhoo left a comment

jonhoo Oct 19, 2024

surajk-m Oct 19, 2024

jonhoo Oct 27, 2024

surajk-m Oct 28, 2024

jonhoo Oct 27, 2024

surajk-m Oct 28, 2024

jonhoo Oct 27, 2024

surajk-m Oct 28, 2024

jonhoo Oct 28, 2024

surajk-m Oct 28, 2024 •

edited

Loading

jonhoo Oct 28, 2024

surajk-m Oct 29, 2024

jonhoo Oct 28, 2024

surajk-m Oct 28, 2024

jonhoo commented Oct 28, 2024

surajk-m commented Oct 29, 2024

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

fix: Encapsulate Wcmd within a private wrapper #275

Are you sure you want to change the base?

fix: Encapsulate Wcmd within a private wrapper #275

Conversation

surajk-m commented Sep 21, 2024

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

jonhoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajk-m Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhoo commented Oct 28, 2024

surajk-m commented Oct 29, 2024

fix: Encapsulate `Wcmd` within a private wrapper #275

fix: Encapsulate `Wcmd` within a private wrapper #275

codecov bot commented Sep 21, 2024 •

edited

Loading

surajk-m Oct 28, 2024 •

edited

Loading