Use Driver finder for sending Options to Selenium Manager#11411
Use Driver finder for sending Options to Selenium Manager#11411titusfortner wants to merge 3 commits intotrunkfrom
Conversation
Codecov ReportBase: 54.54% // Head: 54.54% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## trunk #11411 +/- ##
=======================================
Coverage 54.54% 54.54%
=======================================
Files 85 85
Lines 5627 5627
Branches 243 243
=======================================
Hits 3069 3069
Misses 2315 2315
Partials 243 243 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| return score; | ||
| } | ||
|
|
||
| public Builder withOptions(SafariOptions options) { |
There was a problem hiding this comment.
Just curious, I observed the options are passed but not used. Is that intentional?
There was a problem hiding this comment.
To make it consistent with the others. It's just to avoid telling everyone they need to use the withOptions method if they want to start a service and then say "except here". I'm fine to remove it if we want to go this route.
| } | ||
|
|
||
| public Builder withOptions(SafariOptions options) { | ||
| return this; |
|
LGTM |
|
SonarCloud Quality Gate failed. |
diemol
left a comment
diemol
left a comment
There was a problem hiding this comment.
I am not sure if we need the *DriverFinder.java classes. I think we can call DriverFinder.findExecutable from the driver class.
Nevertheless, I think we do not want to follow this approach which implies sending options to the driver service? So far #11491 looks better to me.
| * Configures and returns a new {@link ChromeDriverService} using the supplied configuration. In | ||
| * this configuration, the service will use the ChromeDriver executable identified by the | ||
| * {@link #CHROME_DRIVER_EXE_PROPERTY} system property. Each service created by this method will | ||
| * {@link ChromeDriverFinder} system property. Each service created by this method will |
There was a problem hiding this comment.
Not sure if that is correct? There is no ChromeDriverFinder system property.
| package org.openqa.selenium.chrome; | ||
|
|
||
| import org.openqa.selenium.remote.service.DriverFinder; | ||
|
|
||
| import java.io.File; | ||
|
|
||
| import static org.openqa.selenium.chrome.ChromeDriverService.CHROME_DRIVER_EXE_PROPERTY; | ||
| public class ChromeDriverFinder extends DriverFinder { | ||
| protected static File findExecutable() { | ||
| return findExecutable( | ||
| "chromedriver", | ||
| CHROME_DRIVER_EXE_PROPERTY, | ||
| "https://chromedriver.chromium.org/", | ||
| "https://chromedriver.chromium.org/downloads"); | ||
| } | ||
|
|
||
| protected static File findExecutable(ChromeOptions options) { | ||
| return findExecutable( | ||
| "chromedriver", | ||
| CHROME_DRIVER_EXE_PROPERTY, | ||
| "https://chromedriver.chromium.org/", | ||
| "https://chromedriver.chromium.org/downloads", | ||
| options); | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not following why we need this class. Why don't we call DriverFinder.findExecutable in ChromeDriver? We could put all hard coded values in variables, make the call from there and avoid this extra class.
This comment also applies for the rest of the drivers.
|
We decided to go with #11491 |
4 participants
This is an alternative implementation to #11376 for using Options class to work with SeleniumManager.
Instead of storing options instance in the
DriverServiceclass and using it in thefindExecutable()method, this PR moves the driver finding pieces out of service classes. There are currently 2 methods, one with options parameter that can call SeleniumManager and one without options. The goal is to get everything to go through that method, and the trick is going to be figuring out how to make that happen (I think it can be done with a combination of deprecations and warning messages).I think this one can be merged as-is, but at the least it needs more javadocs and someone(s) to make sure I've thought everything through.