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

Resolve webdriver api paths relative to server_url #240

Merged
merged 6 commits into from
Nov 25, 2024
Merged

Conversation

grelner
Copy link
Contributor

@grelner grelner commented Sep 27, 2024

Sorry for the delay, but here is a fix for #229.
Thirtyfour uses Url::join to resolve url's with an argument that starts with '/'. Given webdriver url "http://localhost:4444/wd/hub" and "/session", Url::join will return http://localhost:4444/session, thus making is incompatible with systems such as selenoid, and selenium versions < 4 which use /wd/hub as a base path.
This PR simply removes the leading slash from the selenium api paths and this makes Url::join resolve the paths as expected.

Copy link
Owner

@Vrtgs Vrtgs left a comment

Choose a reason for hiding this comment

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

when you use impl AsRef<_> in a function it adds a silent generic, which makes it so the function will be monomorphized for every type of path, this is esspecially bad for async fn and has bigger impact on compile times, as every invocation now has its own complicated state machine

so I would suggest doing what the stdlib already does with things like std::fs::write:

fn foo(p: impl AsRef<Path>) {
    fn inner(path: &Path) {
        /*code*/
    }
    inner(p.as_ref())
}

@Vrtgs
Copy link
Owner

Vrtgs commented Oct 22, 2024

hey I hope you aren't busy but I'm just checking to make sure you see this, so could you implement the changes or should I pull them in then change them my self?

@grelner
Copy link
Contributor Author

grelner commented Nov 4, 2024

Hi sorry this fell between the cracks. I've reverted the AsRef commit, as it's not relevant to this PR

@grelner grelner requested a review from Vrtgs November 5, 2024 06:52
Vrtgs
Vrtgs previously approved these changes Nov 25, 2024
Copy link
Owner

@Vrtgs Vrtgs left a comment

Choose a reason for hiding this comment

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

lgtm

@grelner
Copy link
Contributor Author

grelner commented Nov 25, 2024

@Vrtgs not sure I understand that coverage test failure. I don't see any failures in the log

@Vrtgs Vrtgs merged commit cc7abd6 into Vrtgs:main Nov 25, 2024
11 of 12 checks passed
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