Reworking MutableCapabilities.merge(..) to return a new instance instead of modifying the current object in place to conform the behavior specified in Capabilities interface.

DesiredCapabilities continues to work as in version 3,  it modifies itself.  This class should be deprecated as soon as RemoteWebDriver builder is ready.

Author: barancev

java/client/src/org/openqa/selenium/Capabilities.java

@@ -107,11 +107,7 @@ default boolean is(String capabilityName) {
    * {@code this}.
    */
   default Capabilities merge(Capabilities other) {
-    HashMap<String, Object> map = new HashMap<>(asMap());
-    if (other != null) {
-      map.putAll(other.asMap());
-    }
-    return new ImmutableCapabilities(map);
+    return new ImmutableCapabilities(new MutableCapabilities(this).merge(other));
   }
 
   default Set<String> getCapabilityNames() {

java/client/src/org/openqa/selenium/MutableCapabilities.java

@@ -67,22 +67,17 @@ public MutableCapabilities(Map<String, ?> capabilities) {
   }
 
   /**
-   * Merge the extra capabilities provided into this DesiredCapabilities instance. If capabilities
-   * with the same name exist in this instance, they will be overridden by the values from the
-   * extraCapabilities object.
-   *
-   * @param extraCapabilities Additional capabilities to be added.
-   * @return The DesiredCapabilities instance after the merge.
+   * Merge two {@link Capabilities} together and return the union of the two as a new
+   * {@link Capabilities} instance. Capabilities from {@code other} will override those in
+   * {@code this}.
    */
   @Override
-  public MutableCapabilities merge(Capabilities extraCapabilities) {
-    if (extraCapabilities == null) {
-      return this;
+  public MutableCapabilities merge(Capabilities other) {
+    MutableCapabilities newInstance = new MutableCapabilities(this);
+    if (other != null) {
+      other.asMap().forEach(newInstance::setCapability);
     }
-
-    extraCapabilities.asMap().forEach(this::setCapability);
-
-    return this;
+    return newInstance;
   }
 
   public void setCapability(String capabilityName, boolean value) {

java/client/src/org/openqa/selenium/remote/DesiredCapabilities.java

@@ -104,7 +104,9 @@ public void setAcceptInsecureCerts(boolean acceptInsecureCerts) {
    */
   @Override
   public DesiredCapabilities merge(Capabilities extraCapabilities) {
-    super.merge(extraCapabilities);
+    if (extraCapabilities != null) {
+      extraCapabilities.asMap().forEach(this::setCapability);
+    }
     return this;
   }
 

java/client/test/org/openqa/selenium/BUILD.bazel

@@ -7,7 +7,7 @@ SMALL_TESTS = [
     "ByTest.java",
     "CookieTest.java",
     "DimensionTest.java",
-    "ImmutableCapabilitiesTest.java",
+    "CapabilitiesTest.java",
     "KeysTest.java",
     "OutputTypeTest.java",
     "PersistentCapabilitiesTest.java",

java/client/test/org/openqa/selenium/ImmutableCapabilitiesTest.java renamed to java/client/test/org/openqa/selenium/CapabilitiesTest.java

@@ -30,7 +30,7 @@
 import java.util.Map;
 
 @Category(UnitTests.class)
-public class ImmutableCapabilitiesTest {
+public class CapabilitiesTest {
 
   @Test
   public void canCreateEmptyCapabilities() {
@@ -60,14 +60,14 @@ public void canCreateThreePairCapabilities() {
   public void canCreateFourPairCapabilities() {
     Capabilities caps = new ImmutableCapabilities("c1", "v1", "c2", 2, "c3", true, "c4", "v4");
     assertThat(caps.asMap())
-        .isEqualTo(ImmutableMap.of("c1", "v1", "c2", 2, "c3", true, "c4", "v4"));
+      .isEqualTo(ImmutableMap.of("c1", "v1", "c2", 2, "c3", true, "c4", "v4"));
   }
 
   @Test
   public void canCreateFivePairCapabilities() {
     Capabilities caps = new ImmutableCapabilities("c1", "v1", "c2", 2, "c3", true, "c4", "v4", "c5", "v5");
     assertThat(caps.asMap())
-        .isEqualTo(ImmutableMap.of("c1", "v1", "c2", 2, "c3", true, "c4", "v4", "c5", "v5"));
+      .isEqualTo(ImmutableMap.of("c1", "v1", "c2", 2, "c3", true, "c4", "v4", "c5", "v5"));
   }
 
   @Test
@@ -88,7 +88,36 @@ public void shouldCheckKeyType() {
     Map<Object, Object> map = new HashMap<>();
     map.put(new Object(), new Object());
     assertThatExceptionOfType(IllegalArgumentException.class)
-        .isThrownBy(() -> new ImmutableCapabilities(map));
+      .isThrownBy(() -> new ImmutableCapabilities(map));
   }
 
+  @Test
+  public void canMergeImmutableCapabilities() {
+    Map<String, Object> map1 = ImmutableMap.of("c1", "v1", "c2", "v2");
+    Map<String, Object> map2 = ImmutableMap.of("c1", "new value", "c3", "v3");
+    Capabilities caps1 = new ImmutableCapabilities(map1);
+    Capabilities caps2 = new ImmutableCapabilities(map2);
+    Capabilities merged = caps1.merge(caps2);
+    assertThat(merged).isNotSameAs(caps1).isNotSameAs(caps2);
+    assertThat(merged.asMap()).containsExactlyEntriesOf(
+      ImmutableMap.of(
+        "c1", "new value", "c2", "v2", "c3", "v3"));
+    assertThat(caps1.asMap()).containsExactlyEntriesOf(map1);
+    assertThat(caps2.asMap()).containsExactlyEntriesOf(map2);
+  }
+
+  @Test
+  public void canMergeMutableCapabilities() {
+    Map<String, Object> map1 = ImmutableMap.of("c1", "v1", "c2", "v2");
+    Map<String, Object> map2 = ImmutableMap.of("c1", "new value", "c3", "v3");
+    Capabilities caps1 = new MutableCapabilities(map1);
+    Capabilities caps2 = new MutableCapabilities(map2);
+    Capabilities merged = caps1.merge(caps2);
+    assertThat(merged).isNotSameAs(caps1).isNotSameAs(caps2);
+    assertThat(merged.asMap()).containsExactlyEntriesOf(
+      ImmutableMap.of(
+        "c1", "new value", "c2", "v2", "c3", "v3"));
+    assertThat(caps1.asMap()).containsExactlyEntriesOf(map1);
+    assertThat(caps2.asMap()).containsExactlyEntriesOf(map2);
+  }
 }