[java] implement DriverFinder completely independent of Service classes

[titusfortner]

Description

This is an alternate implementation of #11411

Advantages:

• better separation of concerns; it keeps the finding logic completely separate from Service classes
• backwards compatible; user doesn't need to change anything in their code for this to work

Disadvantages:

• The implementation defeats the purpose of the builder pattern; the value can be null in the constructor, and requires a setter/getter
• Exceptions in finding the driver happen in Driver constructor, not the Service constructor

Motivation and Context

I got the idea from trying to see how I would implement this functionality in Ruby.

The underlying problem is that a user can create a service instance before the options instance. We can either:

• Force the user to pass in options to the Service class if they are creating their own (Use Driver finder for sending Options to Selenium Manager #11411), but we agreed last TLC meeting that we didn't like that idea in general, so we want to deprecate that rather than extend it.
• Somehow find and update the executable path on behalf of the user regardless of how they are constructing.

Is there a better Java pattern here? Like, I guess we can avoid the setter/getter by building a new Service instance from the Driver constructor with a builder method that takes an existing Service instance and the executable path from the DriverFinder, but that doesn't prevent a Service instance from having a null executable value when constructed, and errors still happen in driver constructor rather than the service construction.

Any feedback appreciated.

[codecov-commenter]

Codecov Report

Base: 54.65% // Head: 54.65% // No change to project coverage 👍

Coverage data is based on head (0632c76) compared to base (728db91).
Patch has no changes to coverable lines.

❗ Current head 0632c76 differs from pull request most recent head 673cd7e. Consider uploading reports for the commit 673cd7e to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #11491   +/-   ##
=======================================
  Coverage   54.65%   54.65%           
=======================================
  Files          85       85           
  Lines        5643     5643           
  Branches      244      244           
=======================================
  Hits         3084     3084           
  Misses       2315     2315           
  Partials      244      244           

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pujagani]

Can we not have a Require.nonNull check in the DriverService class constructors?

selenium/java/src/org/openqa/selenium/chrome/ChromeDriverService.java

Line 126 in 436db51

public ChromeDriverService(

this calls the DriverService class constructor.
If executable is passed explicitly, it should not be null.

Since

selenium/java/src/org/openqa/selenium/chrome/ChromeDriverService.java

Line 145 in 436db51

public static ChromeDriverService createDefaultService() {

will use the DriverFinder methods to identify executable because user chooses to use the default mechanism.

Just a suggestion, I might be missing something here.

[titusfortner]

can we not have a Require.nonNull check in the DriverService class constructors

Driver finder needs the options instance.
If service instance was always created in driver constructor, then we'd always have the options class for driver finder. But user can create it and pass it into the driver constructor, so we can't use finder. Service constructor needs to allow null executable because the builder is no longer locating the driver. It's getting set on an existing service instance in the driver constructor.
I don't see a good alternative that doesn't require the user to change code to initialize service class with options instance.

[diemol]

I think this PR looks better than #11411, thanks @titusfortner!

However, I agree with the comments, it'd be nice to have a single DriverFinder class.

[titusfortner]

So, this moves all the DriverFinder classes into one, except for SafariDriverFinder because the logic is different. We could probably get rid of that one too if we overloaded a couple methods in SafariDriver (2 valid service classes) and could push the logic for checking OS version etc to the Selenium Manager, but I'm not sure that's better.

Really, though, combining the builder pattern with the one-off setter/getter feels really weird to me. It's nice that it just works without the user needing to do anything, but I was hoping there might be a better way to do this... 😄

[titusfortner]

Oh, and I changed the default error message to link to our documentation instead of directly to the driver URLs, which I think is preferable.

[diemol]

A small change requested to help code readability. Everything else looks 👍

Thank you!

[pujagani]

Looks good to me overall.

[titusfortner]

I'm moving this to draft because there is a selenium manager issue that needs to get resolved first, and it won't get released until at least 4.9

[titusfortner]

I figured out a simple way to remove the need for the SafariDriverFinder class by overloading DriverFinder's getPath()

[titusfortner]

@diemol all the issues I ran into with the selenium manager binary have been sorted now.

[pujagani]

There seems to be a linting error in the GeckoDriverServiceTest. Can you please help fix it? Overall, the PR LGTM.

[titusfortner]

Diego wanted me to keep the constants in the Service class; instead of calling the service class values from the driver, I created an interface and methods to get the values in the drivers.

Except, I can't get that class to load with Bazel. (please help)

Also, I updated the DriverInfo classes because isAvailable() will no longer error when just building the service class. To make that easier, I created a basic version for getting path from DriverFinder via SeleniumManager that ignores Options class.

[diemol]

I did not notice this PR being ready and I caused the conflict. I will review, solve the conflict, and if everything OK, I will merge it.

[diemol]

Thank you, @titusfortner!

CI failures are not related to this change.