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

[java] remove excessive warnings about invalid capabilities #10739

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 18 additions & 18 deletions java/src/org/openqa/selenium/ImmutableCapabilities.java
Expand Up @@ -23,7 +23,7 @@
import java.util.Map;
import java.util.TreeMap;

import static org.openqa.selenium.SharedCapabilitiesMethods.setCapability;
import static org.openqa.selenium.SharedCapabilitiesMethods.setCapabilityNoValidation;

public class ImmutableCapabilities implements Capabilities {

Expand All @@ -40,7 +40,7 @@ public ImmutableCapabilities(String k, Object v) {
Require.nonNull("Value", v);

Map<String, Object> delegate = new TreeMap<>();
setCapability(delegate, k, v);
setCapabilityNoValidation(delegate, k, v);

this.delegate = Collections.unmodifiableMap(delegate);
this.hashCode = SharedCapabilitiesMethods.hashCode(this);
Expand All @@ -54,8 +54,8 @@ public ImmutableCapabilities(String k1, Object v1, String k2, Object v2) {

Map<String, Object> delegate = new TreeMap<>();

setCapability(delegate, k1, v1);
setCapability(delegate, k2, v2);
setCapabilityNoValidation(delegate, k1, v1);
setCapabilityNoValidation(delegate, k2, v2);

this.delegate = Collections.unmodifiableMap(delegate);
this.hashCode = SharedCapabilitiesMethods.hashCode(this);
Expand All @@ -71,9 +71,9 @@ public ImmutableCapabilities(String k1, Object v1, String k2, Object v2, String

Map<String, Object> delegate = new TreeMap<>();

setCapability(delegate, k1, v1);
setCapability(delegate, k2, v2);
setCapability(delegate, k3, v3);
setCapabilityNoValidation(delegate, k1, v1);
setCapabilityNoValidation(delegate, k2, v2);
setCapabilityNoValidation(delegate, k3, v3);

this.delegate = Collections.unmodifiableMap(delegate);
this.hashCode = SharedCapabilitiesMethods.hashCode(this);
Expand All @@ -95,10 +95,10 @@ public ImmutableCapabilities(

Map<String, Object> delegate = new TreeMap<>();

setCapability(delegate, k1, v1);
setCapability(delegate, k2, v2);
setCapability(delegate, k3, v3);
setCapability(delegate, k4, v4);
setCapabilityNoValidation(delegate, k1, v1);
setCapabilityNoValidation(delegate, k2, v2);
setCapabilityNoValidation(delegate, k3, v3);
setCapabilityNoValidation(delegate, k4, v4);

this.delegate = Collections.unmodifiableMap(delegate);
this.hashCode = SharedCapabilitiesMethods.hashCode(this);
Expand All @@ -123,11 +123,11 @@ public ImmutableCapabilities(

Map<String, Object> delegate = new TreeMap<>();

setCapability(delegate, k1, v1);
setCapability(delegate, k2, v2);
setCapability(delegate, k3, v3);
setCapability(delegate, k4, v4);
setCapability(delegate, k5, v5);
setCapabilityNoValidation(delegate, k1, v1);
setCapabilityNoValidation(delegate, k2, v2);
setCapabilityNoValidation(delegate, k3, v3);
setCapabilityNoValidation(delegate, k4, v4);
setCapabilityNoValidation(delegate, k5, v5);

this.delegate = Collections.unmodifiableMap(delegate);
this.hashCode = SharedCapabilitiesMethods.hashCode(this);
Expand All @@ -142,7 +142,7 @@ public ImmutableCapabilities(Capabilities other) {
Object value = other.getCapability(name);
Require.nonNull("Capability value", value);

setCapability(delegate, name, value);
setCapabilityNoValidation(delegate, name, value);
});

this.delegate = Collections.unmodifiableMap(delegate);
Expand All @@ -158,7 +158,7 @@ public ImmutableCapabilities(Map<?, ?> capabilities) {
Object v = capabilities.get(key);
Require.nonNull("Capability value", value);

setCapability(delegate, (String) key, v);
setCapabilityNoValidation(delegate, (String) key, v);
});

this.delegate = Collections.unmodifiableMap(delegate);
Expand Down
16 changes: 16 additions & 0 deletions java/src/org/openqa/selenium/MutableCapabilities.java
Expand Up @@ -101,6 +101,22 @@ public void setCapability(String key, Object value) {
SharedCapabilitiesMethods.setCapability(caps, key, value);
}

protected void setCapabilityWithoutValidation(String key, Object value) {
Require.nonNull("Capability name", key);

if (OPTION_KEYS.contains(key) && value instanceof Capabilities) {
((Capabilities) value).asMap().forEach(this::setCapability);
return;
}

if (value == null) {
caps.remove(key);
return;
}

SharedCapabilitiesMethods.setCapabilityNoValidation(caps, key, value);
}

@Override
public Map<String, Object> asMap() {
return Collections.unmodifiableMap(caps);
Expand Down
7 changes: 7 additions & 0 deletions java/src/org/openqa/selenium/SharedCapabilitiesMethods.java
Expand Up @@ -47,7 +47,14 @@ static boolean equals(Capabilities left, Capabilities right) {

static void setCapability(Map<String, Object> caps, String key, Object value) {
W3CCapabilityKeysValidator.validateCapability(key);
setCapabilityNoValidation(caps, key, value);
}

/**
* This method is temporary and intended to be used privately by other Selenium classes that are known to be
* compliant and do not need to be validated.
*/
static void setCapabilityNoValidation(Map<String, Object> caps, String key, Object value) {
if ("loggingPrefs".equals(key) && value instanceof Map) {
LoggingPreferences prefs = new LoggingPreferences();
@SuppressWarnings("unchecked") Map<String, String> prefsMap = (Map<String, String>) value;
Expand Down
4 changes: 2 additions & 2 deletions java/src/org/openqa/selenium/firefox/FirefoxOptions.java
Expand Up @@ -371,8 +371,8 @@ protected Object getExtraCapability(String capabilityName) {
public FirefoxOptions merge(Capabilities capabilities) {
Require.nonNull("Capabilities to merge", capabilities);
FirefoxOptions newInstance = new FirefoxOptions();
getCapabilityNames().forEach(name -> newInstance.setCapability(name, getCapability(name)));
capabilities.getCapabilityNames().forEach(name -> newInstance.setCapability(name, capabilities.getCapability(name)));
getCapabilityNames().forEach(name -> newInstance.setCapabilityWithoutValidation(name, getCapability(name)));
capabilities.getCapabilityNames().forEach(name -> newInstance.setCapabilityWithoutValidation(name, capabilities.getCapability(name)));
newInstance.mirror(this);
if (capabilities instanceof FirefoxOptions) {
newInstance.mirror((FirefoxOptions) capabilities);
Expand Down
23 changes: 15 additions & 8 deletions java/src/org/openqa/selenium/ie/InternetExplorerOptions.java
Expand Up @@ -237,18 +237,25 @@ private InternetExplorerOptions amend(String optionName, Object value) {

@Override
public void setCapability(String key, Object value) {
super.setCapability(key, value);

if (IE_SWITCHES.equals(key)) {
if (value instanceof List) {
value = ((List<?>) value).stream().map(Object::toString).collect(Collectors.joining(" "));
}
}

// This puts IE specific values inside se:options map
if (CAPABILITY_NAMES.contains(key)) {
// TODO - stop putting IE specific values at the top level of capabilities
// These should only be added to ieOptions to be w3c compliant
// Regardless, these are known valid for inside se:w3c so shouldn't throw warnings
super.setCapabilityWithoutValidation(key, value);

// Switches are a String for w3c instead of a List for jwp
if (IE_SWITCHES.equals(key)) {
if (value instanceof List) {
value = ((List<?>) value).stream().map(Object::toString).collect(Collectors.joining(" "));
}
}
ieOptions.put(key, value);
} else {
super.setCapability(key, value);
}

// This replaces se:options map with what is provided
if (IE_OPTIONS.equals(key)) {
ieOptions.clear();
Map<String, Object> streamFrom;
Expand Down
Expand Up @@ -347,6 +347,7 @@ WebDriver getLocalDriver() {
WebDriver localDriver = first.get().get();

if (localDriver != null && this.useCustomConfig) {
localDriver.quit();
throw new IllegalArgumentException("ClientConfig instances do not work for Local Drivers");
}

Expand Down
Expand Up @@ -55,6 +55,8 @@ public Map<String, Object> apply(Map<String, Object> unmodifiedCaps) {
Map<String, Object> options = (Map<String, Object>) unmodifiedCaps.getOrDefault(
"moz:firefoxOptions",
new TreeMap<>());
options = new TreeMap<>(options);

if (unmodifiedCaps.containsKey("firefox_binary") && !options.containsKey("binary")) {
// Here's hoping that the binary is just a string. It should be as FirefoxBinary.toJson just
// encodes the path.
Expand Down
4 changes: 2 additions & 2 deletions java/src/org/openqa/selenium/safari/SafariOptions.java
Expand Up @@ -80,9 +80,9 @@ public SafariOptions merge(Capabilities extraCapabilities) {

SafariOptions newInstance = new SafariOptions();

getCapabilityNames().forEach(name -> newInstance.setCapability(name, getCapability(name)));
getCapabilityNames().forEach(name -> newInstance.setCapabilityWithoutValidation(name, getCapability(name)));
extraCapabilities.getCapabilityNames()
.forEach(name -> newInstance.setCapability(name, extraCapabilities.getCapability(name)));
.forEach(name -> newInstance.setCapabilityWithoutValidation(name, extraCapabilities.getCapability(name)));

return newInstance;
}
Expand Down
Expand Up @@ -17,7 +17,9 @@

package org.openqa.selenium.chrome;

import org.junit.After;
import org.junit.Test;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.JavascriptExecutor;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
Expand Down Expand Up @@ -45,21 +47,31 @@ public class ChromeDriverFunctionalTest extends JUnit4TestBase {

private final String CLIPBOARD_READ = "clipboard-read";
private final String CLIPBOARD_WRITE = "clipboard-write";
private WebDriver localDriver;

@After
public void quitDriver() {
if (localDriver != null) {
localDriver.quit();
}
}

@Test
public void builderGeneratesDefaultChromeOptions() {
WebDriver driver = ChromeDriver.builder().build();
driver.quit();
localDriver = ChromeDriver.builder().build();
ChromeDriver chromeDriver = (ChromeDriver) localDriver;
Capabilities capabilities = chromeDriver.getCapabilities();

assertThat(localDriver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ZERO);
assertThat(capabilities.getCapability("browserName")).isEqualTo("chrome");
}

@Test
public void builderOverridesDefaultChromeOptions() {
ChromeOptions options = new ChromeOptions();
options.setImplicitWaitTimeout(Duration.ofMillis(1));
WebDriver driver = ChromeDriver.builder().oneOf(options).build();
assertThat(driver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ofMillis(1));

driver.quit();
localDriver = ChromeDriver.builder().oneOf(options).build();
assertThat(localDriver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ofMillis(1));
}

@Test
Expand Down Expand Up @@ -93,22 +105,18 @@ public void canSetPermissionHeadless() {
options.setHeadless(true);

//TestChromeDriver is not honoring headless request; using ChromeDriver instead
WebDriver driver = new WebDriverBuilder().get(options);
try {
HasPermissions permissions = (HasPermissions) driver;
localDriver = new WebDriverBuilder().get(options);
HasPermissions permissions = (HasPermissions) localDriver;

driver.get(pages.clicksPage);
assertThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("prompt");
assertThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("prompt");
localDriver.get(pages.clicksPage);
assertThat(checkPermission(localDriver, CLIPBOARD_READ)).isEqualTo("prompt");
assertThat(checkPermission(localDriver, CLIPBOARD_WRITE)).isEqualTo("prompt");

permissions.setPermission(CLIPBOARD_READ, "granted");
permissions.setPermission(CLIPBOARD_WRITE, "granted");
permissions.setPermission(CLIPBOARD_READ, "granted");
permissions.setPermission(CLIPBOARD_WRITE, "granted");

assertThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("granted");
assertThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("granted");
} finally {
driver.quit();
}
assertThat(checkPermission(localDriver, CLIPBOARD_READ)).isEqualTo("granted");
assertThat(checkPermission(localDriver, CLIPBOARD_WRITE)).isEqualTo("granted");
}

public String checkPermission(WebDriver driver, String permission){
Expand Down
Expand Up @@ -46,13 +46,13 @@ public class EdgeDriverFunctionalTest extends JUnit4TestBase {
private final String CLIPBOARD_WRITE = "clipboard-write";

@Test
public void builderGeneratesDefaultChromeOptions() {
public void builderGeneratesDefaultEdgeOptions() {
WebDriver driver = EdgeDriver.builder().build();
driver.quit();
}

@Test
public void builderOverridesDefaultChromeOptions() {
public void builderOverridesDefaultEdgeOptions() {
EdgeOptions options = new EdgeOptions();
options.setImplicitWaitTimeout(Duration.ofMillis(1));
WebDriver driver = EdgeDriver.builder().oneOf(options).build();
Expand Down
Expand Up @@ -54,7 +54,7 @@ public void tearDown() {
}

@Test
public void canStartChromeWithCustomOptions() {
public void canStartEdgeWithCustomOptions() {
EdgeOptions options = new EdgeOptions();
options.addArguments("user-agent=foo;bar");
edgeDriver = new WebDriverBuilder().get(options);
Expand Down
34 changes: 17 additions & 17 deletions java/test/org/openqa/selenium/firefox/FirefoxDriverTest.java
Expand Up @@ -63,6 +63,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeNotNull;
import static org.mockito.Mockito.atLeastOnce;
Expand Down Expand Up @@ -102,19 +103,22 @@ public void quitDriver() {
}

@Test
public void builderGeneratesDefaultChromeOptions() {
WebDriver driver = FirefoxDriver.builder().build();
driver.quit();
public void builderGeneratesDefaultFirefoxOptions() {
localDriver = FirefoxDriver.builder().build();
FirefoxDriver firefoxDriver = (FirefoxDriver) localDriver;
Capabilities capabilities = firefoxDriver.getCapabilities();

assertThat(driver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ZERO);
assertTrue((Boolean) capabilities.getCapability("acceptInsecureCerts"));
assertThat(capabilities.getCapability("browserName")).isEqualTo("firefox");
}

@Test
public void builderOverridesDefaultChromeOptions() {
public void builderOverridesDefaultFirefoxOptions() {
FirefoxOptions options = new FirefoxOptions();
options.setImplicitWaitTimeout(Duration.ofMillis(1));
WebDriver driver = FirefoxDriver.builder().oneOf(options).build();
assertThat(driver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ofMillis(1));

driver.quit();
localDriver = FirefoxDriver.builder().oneOf(options).build();
assertThat(localDriver.manage().timeouts().getImplicitWaitTimeout()).isEqualTo(Duration.ofMillis(1));
}

@Test
Expand Down Expand Up @@ -258,17 +262,13 @@ public void shouldWaitUntilBrowserHasClosedProperly() {

@Test
public void shouldBeAbleToStartMoreThanOneInstanceOfTheFirefoxDriverSimultaneously() {
WebDriver secondDriver = new WebDriverBuilder().get();
localDriver = new WebDriverBuilder().get();

try {
driver.get(pages.xhtmlTestPage);
secondDriver.get(pages.formPage);
driver.get(pages.xhtmlTestPage);
localDriver.get(pages.formPage);

assertThat(driver.getTitle()).isEqualTo("XHTML Test Page");
assertThat(secondDriver.getTitle()).isEqualTo("We Leave From Here");
} finally {
secondDriver.quit();
}
assertThat(driver.getTitle()).isEqualTo("XHTML Test Page");
assertThat(localDriver.getTitle()).isEqualTo("We Leave From Here");
}

@Test
Expand Down