Skip to content

Commit

Permalink
[grid] improved logging when driver discovery failed
Browse files Browse the repository at this point in the history
  • Loading branch information
@joerg1985
joerg1985 committed Dec 21, 2023
1 parent f22e08f commit 48e8db2
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 2 deletions.
10 changes: 10 additions & 0 deletions java/src/org/openqa/selenium/chrome/ChromeDriverInfo.java
View file
Expand Up @@ -21,6 +21,8 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.SessionNotCreatedException;
Expand All @@ -29,10 +31,12 @@
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.chromium.ChromiumDriverInfo;
import org.openqa.selenium.remote.CapabilityType;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@AutoService(WebDriverInfo.class)
public class ChromeDriverInfo extends ChromiumDriverInfo {
private static final Logger LOG = Logger.getLogger(ChromeDriverInfo.class.getName());

@Override
public String getDisplayName() {
Expand Down Expand Up @@ -64,7 +68,10 @@ public boolean isAvailable() {
try {
DriverFinder.getPath(ChromeDriverService.createDefaultService(), getCanonicalCapabilities());
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -75,7 +82,10 @@ public boolean isPresent() {
DriverFinder.getPath(
ChromeDriverService.createDefaultService(), getCanonicalCapabilities(), true);
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down
10 changes: 10 additions & 0 deletions java/src/org/openqa/selenium/edge/EdgeDriverInfo.java
View file
Expand Up @@ -21,6 +21,8 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.SessionNotCreatedException;
Expand All @@ -29,10 +31,12 @@
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.chromium.ChromiumDriverInfo;
import org.openqa.selenium.remote.CapabilityType;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@AutoService(WebDriverInfo.class)
public class EdgeDriverInfo extends ChromiumDriverInfo {
private static final Logger LOG = Logger.getLogger(EdgeDriverInfo.class.getName());

@Override
public String getDisplayName() {
Expand Down Expand Up @@ -67,7 +71,10 @@ public boolean isAvailable() {
try {
DriverFinder.getPath(EdgeDriverService.createDefaultService(), getCanonicalCapabilities());
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -78,7 +85,10 @@ public boolean isPresent() {
DriverFinder.getPath(
EdgeDriverService.createDefaultService(), getCanonicalCapabilities(), true);
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down
10 changes: 10 additions & 0 deletions java/src/org/openqa/selenium/firefox/GeckoDriverInfo.java
View file
Expand Up @@ -22,16 +22,20 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@AutoService(WebDriverInfo.class)
public class GeckoDriverInfo implements WebDriverInfo {
private static final Logger LOG = Logger.getLogger(GeckoDriverInfo.class.getName());

@Override
public String getDisplayName() {
Expand Down Expand Up @@ -67,7 +71,10 @@ public boolean isAvailable() {
try {
DriverFinder.getPath(GeckoDriverService.createDefaultService(), getCanonicalCapabilities());
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -78,7 +85,10 @@ public boolean isPresent() {
DriverFinder.getPath(
GeckoDriverService.createDefaultService(), getCanonicalCapabilities(), true);
return true;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down
10 changes: 10 additions & 0 deletions java/src/org/openqa/selenium/ie/InternetExplorerDriverInfo.java
View file
Expand Up @@ -22,17 +22,21 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.Platform;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@AutoService(WebDriverInfo.class)
public class InternetExplorerDriverInfo implements WebDriverInfo {
private static final Logger LOG = Logger.getLogger(InternetExplorerDriverInfo.class.getName());

@Override
public String getDisplayName() {
Expand Down Expand Up @@ -68,7 +72,10 @@ public boolean isAvailable() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -82,7 +89,10 @@ public boolean isPresent() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down
5 changes: 3 additions & 2 deletions java/src/org/openqa/selenium/remote/service/DriverFinder.java
View file
Expand Up @@ -19,6 +19,7 @@

import java.io.File;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.internal.Require;
import org.openqa.selenium.manager.SeleniumManager;
import org.openqa.selenium.manager.SeleniumManagerOutput.Result;
Expand All @@ -37,8 +38,8 @@ public static Result getPath(DriverService service, Capabilities options, boolea
if (result.getDriverPath() == null) {
try {
result = SeleniumManager.getInstance().getDriverPath(options, offline);
} catch (Exception e) {
throw new NoSuchDriverException(
} catch (RuntimeException e) {
throw new WebDriverException(

This comment has been minimized.

Sign in to view

Copy link
@titusfortner

titusfortner Jan 8, 2024

Member

The NoSuchDriverException adds a link to the troubleshooting guide for driver issues, is there a reason you used the more generic exception? I'm refactoring other things in this class now and want to know if I'm breaking something else you did.

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Jan 8, 2024

Author Member

The NoSuchDriverException is handled inside the (Chrome|Safari|...|)DriverInfo.isAvailable method differently than the WebDriverException, the NoSuchDriverException will not be written to the logs of the grid, as it is possible to have not all drivers in place.

Calling the SeleniumManager should never fail, it might return a result without a driver (which will raise a NoSuchDriverException later in the code). When something goes wrong here e.g. the missing c++ redistirbutable, it should not be translated to a NoSuchDriverException.

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Jan 8, 2024

Author Member

@titusfortner one addition to the comment above, when selenium manager is not a beta any more and is available for all plattforms, this try/catch block could be removed in my mind.

This comment has been minimized.

Sign in to view

Copy link
@titusfortner

titusfortner Jan 8, 2024

Member

Hmmm, I think if anything goes wrong trying to get a driver via this class, it should have a NSDE thrown with a link on how to set the driver via System Property or via a method on the Service class. Why wouldn't you want this information?

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Jan 8, 2024

Author Member

@titusfortner Changing this to a NSDE might encurage users to set the property instead of raising an issue.
But i am okay with reverting this, the only thing to change too, is to log the NSDE inside the isAvailable method to have it in the logs for debugging.

This comment has been minimized.

Sign in to view

Copy link
@titusfortner

titusfortner Jan 8, 2024 •
edited

Member

ohhh, now I see what you mean. If it's a problem with user code, give NSDE, if it's a problem with Selenium code throw WDE. That makes sense. I'll rerethink. 😂
String.format("Unable to obtain: %s, error %s", options, e.getMessage()), e);
}
}
Expand Down
11 changes: 11 additions & 0 deletions java/src/org/openqa/selenium/safari/SafariDriverInfo.java
View file
Expand Up @@ -22,18 +22,23 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.Platform;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@AutoService(WebDriverInfo.class)
public class SafariDriverInfo implements WebDriverInfo {

private static final Logger LOG = Logger.getLogger(SafariDriverInfo.class.getName());

@Override
public String getDisplayName() {
return "Safari";
Expand Down Expand Up @@ -72,7 +77,10 @@ public boolean isAvailable() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -86,7 +94,10 @@ public boolean isPresent() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down
10 changes: 10 additions & 0 deletions java/src/org/openqa/selenium/safari/SafariTechPreviewDriverInfo.java
View file
Expand Up @@ -22,18 +22,22 @@

import com.google.auto.service.AutoService;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.Platform;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebDriverInfo;
import org.openqa.selenium.remote.NoSuchDriverException;
import org.openqa.selenium.remote.service.DriverFinder;

@SuppressWarnings("unused")
@AutoService(WebDriverInfo.class)
public class SafariTechPreviewDriverInfo implements WebDriverInfo {
private static final Logger LOG = Logger.getLogger(SafariTechPreviewDriverInfo.class.getName());

@Override
public String getDisplayName() {
Expand Down Expand Up @@ -73,7 +77,10 @@ public boolean isAvailable() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand All @@ -89,7 +96,10 @@ public boolean isPresent() {
return true;
}
return false;
} catch (NoSuchDriverException e) {
return false;
} catch (IllegalStateException | WebDriverException e) {
LOG.log(Level.WARNING, "failed to discover driver path", e);
return false;
}
}
Expand Down

0 comments on commit 48e8db2

Please sign in to comment.