[java] Improving merge of ChromiumOptions to take into account arguments, extentions and experimental options

Author: barancev

java/client/src/org/openqa/selenium/chrome/ChromeOptions.java

@@ -65,8 +65,8 @@ public ChromeOptions setLogLevel(ChromeDriverLogLevel logLevel){
   @Override
   public ChromeOptions merge(Capabilities extraCapabilities) {
     ChromeOptions newInstance = new ChromeOptions();
-    this.asMap().forEach(newInstance::setCapability);
-    extraCapabilities.asMap().forEach(newInstance::setCapability);
+    newInstance.mergeInPlace(this);
+    newInstance.mergeInPlace(extraCapabilities);
     return newInstance;
   }
 

java/client/src/org/openqa/selenium/chromium/ChromiumOptions.java

@@ -21,7 +21,9 @@
 import static java.util.Collections.unmodifiableMap;
 import static java.util.stream.Collectors.toList;
 
+import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.SessionNotCreatedException;
+import org.openqa.selenium.chrome.ChromeOptions;
 import org.openqa.selenium.internal.Require;
 import org.openqa.selenium.remote.AbstractDriverOptions;
 
@@ -31,6 +33,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Base64;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -61,10 +64,10 @@
 public class ChromiumOptions<T extends ChromiumOptions> extends AbstractDriverOptions<ChromiumOptions> {
 
   private String binary;
-  private List<String> args = new ArrayList<>();
-  private List<File> extensionFiles = new ArrayList<>();
-  private List<String> extensions = new ArrayList<>();
-  private Map<String, Object> experimentalOptions = new HashMap<>();
+  private final List<String> args = new ArrayList<>();
+  private final List<File> extensionFiles = new ArrayList<>();
+  private final List<String> extensions = new ArrayList<>();
+  private final Map<String, Object> experimentalOptions = new HashMap<>();
 
   private final String CAPABILITY;
 
@@ -232,4 +235,15 @@ public Map<String, Object> asMap() {
 
     return unmodifiableMap(toReturn);
   }
+
+  protected void mergeInPlace(Capabilities capabilities) {
+    capabilities.asMap().forEach(this::setCapability);
+    if (capabilities instanceof ChromiumOptions) {
+      ChromiumOptions<?> options = (ChromiumOptions<?>) capabilities;
+      addArguments(options.args);
+      addExtensions(options.extensionFiles);
+      addEncodedExtensions(options.extensions);
+      options.experimentalOptions.forEach(this::setExperimentalOption);
+    }
+  }
 }

java/client/src/org/openqa/selenium/edge/EdgeOptions.java

@@ -60,8 +60,8 @@ public EdgeOptions() {
   @Override
   public EdgeOptions merge(Capabilities extraCapabilities) {
     EdgeOptions newInstance = new EdgeOptions();
-    this.asMap().forEach(newInstance::setCapability);
-    extraCapabilities.asMap().forEach(newInstance::setCapability);
+    newInstance.mergeInPlace(this);
+    newInstance.mergeInPlace(extraCapabilities);
     return newInstance;
   }
 }

java/client/test/org/openqa/selenium/chrome/ChromeOptionsTest.java

@@ -19,6 +19,8 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.assertj.core.api.InstanceOfAssertFactories.LIST;
+import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
 import static org.openqa.selenium.chrome.ChromeDriverLogLevel.OFF;
 import static org.openqa.selenium.chrome.ChromeDriverLogLevel.SEVERE;
 
@@ -56,5 +58,15 @@ public void canBuildLogLevelFromStringRepresentation() {
     assertThat(ChromeDriverLogLevel.fromString("SEVERE")).isEqualTo(SEVERE);
   }
 
+  @Test
+  public void mergingOptionsMergesArguments() {
+    ChromeOptions one = new ChromeOptions().addArguments("verbose");
+    ChromeOptions two = new ChromeOptions().addArguments("silent");
+    ChromeOptions merged = one.merge(two);
 
+    assertThat(merged.asMap()).asInstanceOf(MAP)
+      .extractingByKey(ChromeOptions.CAPABILITY).asInstanceOf(MAP)
+      .extractingByKey("args").asInstanceOf(LIST)
+      .containsExactly("verbose", "silent");
+  }
 }

java/client/test/org/openqa/selenium/edge/EdgeOptionsTest.java

@@ -18,6 +18,7 @@
 package org.openqa.selenium.edge;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.InstanceOfAssertFactories.LIST;
 import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
 
 import com.google.common.io.Files;
@@ -26,6 +27,7 @@
 import org.junit.experimental.categories.Category;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.PageLoadStrategy;
+import org.openqa.selenium.chrome.ChromeOptions;
 import org.openqa.selenium.remote.BrowserType;
 import org.openqa.selenium.remote.CapabilityType;
 import org.openqa.selenium.testing.UnitTests;
@@ -106,4 +108,15 @@ private File createTempFile(File tmpDir, String content) {
     }
   }
 
+  @Test
+  public void mergingOptionsMergesArguments() {
+    EdgeOptions one = new EdgeOptions().addArguments("verbose");
+    EdgeOptions two = new EdgeOptions().addArguments("silent");
+    EdgeOptions merged = one.merge(two);
+
+    assertThat(merged.asMap()).asInstanceOf(MAP)
+      .extractingByKey(EdgeOptions.CAPABILITY).asInstanceOf(MAP)
+      .extractingByKey("args").asInstanceOf(LIST)
+      .containsExactly("verbose", "silent");
+  }
 }