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.

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

feat: expose the returned remote capabilities on the Client #223

Merged
merged 3 commits into from
Sep 16, 2023
Merged

feat: expose the returned remote capabilities on the Client #223

merged 3 commits into from
Sep 16, 2023

Conversation

RomFouq
Copy link
Contributor

@RomFouq RomFouq commented Jul 21, 2023

This would tackle #195 by storing the remote capabilities (obtained with the new session response) in the Client, and exposing them via a public getter for later use.
This internally leverages the already existing webdriver::response::NewSessionResponse to map the response.

Being able to access the remote capabilities is necessary to be able to obtain the webSocketUrl remote capability and establish a WebDriver BiDi connection.

Notes:

  • The remote capabilities are stored in and exposed on the Client because it was the easiest, not sure if it would make more sense to have them on the Session instead.
  • The public getter is named .remote_capabilities(), as it exposes the capabilities returned by the remote end, maybe you would prefer another name as I don't think fantoccini uses the local end/remote end terminology from the W3C specification.
  • I've replaced the .remove()/.insert()-back-in-case-of-error JSON value extraction with .get()/.to_owned() as we now also need to access capabilities as well; do you think it would be better to avoid this clone?

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

This is a great addition, thank you! Left a few questions, but none of them major.

src/client.rs Outdated Show resolved Hide resolved
src/session.rs Outdated Show resolved Hide resolved
@RomFouq
Copy link
Contributor Author

RomFouq commented Aug 13, 2023

I updated the PR and addressed your comments:

  • The whole NewSessionResponse is now stored in the Client: as
    NewSessionResponse does not implemented Clone and we want Client to be
    Clone and still Sync, I put it into an Arc (and an Option, since there
    is no obvious default NewSessionResponse).
    That response is exposed with the getter session_creation_response() (I
    didn't name it new_session_response() because this looks too much like a
    constructor function to my taste).
  • The remote capabilities getter method is now simply
    named capabilities().

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #223 (f1256f5) into main (9960984) will increase coverage by 0.27%.
The diff coverage is 87.50%.

Files Changed Coverage
src/wd.rs 76.92%
src/session.rs 92.30%
src/client.rs 100.00%

src/client.rs Show resolved Hide resolved
@RomFouq
Copy link
Contributor Author

RomFouq commented Aug 27, 2023

Oh right, fixed that by introducing our own NewSessionResponse, marking it non_exhaustive as you said, making in Clone, and deriving Serialize and Deserialize.

@RomFouq RomFouq requested a review from jonhoo August 29, 2023 16:13
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Nice! Very nearly there 🎉

src/wd.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Sep 9, 2023

Also, you may want to merge from main to get a bunch of test fixes.

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Thanks!

@RomFouq
Copy link
Contributor Author

RomFouq commented Sep 10, 2023

The test checking that fantoccini can compile with the minimum version of each dependency seems to be failing because we try to derive Eq on NewSessionResponse, which contains Capabilities, a serde_json::Map, while serde_json has only been implementing Eq on Map since version 1.0.50, and current minimum version in fantoccini is 1.0.25.
Can we bump serde_json in this PR, or should I just not derive Eq on NewSessionResponse for now?

The other failing test seems unrelated.

@jonhoo
Copy link
Owner

jonhoo commented Sep 10, 2023

Totally fine to bump the minimum version of serde_json!

@RomFouq
Copy link
Contributor Author

RomFouq commented Sep 10, 2023

Great! So I bumped serde_json to the newest version (released yesterday). It shouldn't increase the MSRV.

@jonhoo
Copy link
Owner

jonhoo commented Sep 10, 2023

I'd generally prefer only bumping it to what we actually need (what goes in Cargo.toml is a minimum version after all). But not the end of the world if it preserves MSRV

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Thank you!

@jonhoo jonhoo merged commit c97694e into jonhoo:main Sep 16, 2023
18 of 19 checks passed
@jonhoo
Copy link
Owner

jonhoo commented Sep 16, 2023

Released in v0.20.0-rc.7 🎉

@RomFouq
Copy link
Contributor Author

RomFouq commented Sep 17, 2023

Thanks a lot for your help on this PR and for the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants