Introduce an abstract base class for both Capabilities types

There's inconsitency between how `ImmutableCapabilities` and
`MutableCapabilities` handle tasks such as turning themselves
into strings. Since they are actually types of the same thing
and they are meant to have shared behaviour, it's okay to
introduce a common base class.

Author: shs96c

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

@@ -0,0 +1,80 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium;
+
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+abstract class AbstractCapabilities implements Capabilities {
+
+  protected final Map<String, Object> caps = new TreeMap<>();
+
+  @Override
+  public String toString() {
+    Map<Object, String> seen = new IdentityHashMap<>();
+    return "Capabilities " + abbreviate(seen, caps);
+  }
+
+  private String abbreviate(Map<Object, String> seen, Object stringify) {
+    if (stringify == null) {
+      return "null";
+    }
+
+    StringBuilder value = new StringBuilder();
+
+    if (stringify.getClass().isArray()) {
+      value.append("[");
+      value.append(
+          Stream.of((Object[]) stringify)
+              .map(item -> abbreviate(seen, item))
+              .collect(Collectors.joining(", ")));
+      value.append("]");
+    } else if (stringify instanceof Collection) {
+      value.append("[");
+      value.append(
+          ((Collection<?>) stringify).stream()
+              .map(item -> abbreviate(seen, item))
+              .collect(Collectors.joining(", ")));
+      value.append("]");
+    } else if (stringify instanceof Map) {
+      value.append("{");
+      value.append(
+          ((Map<?, ?>) stringify).entrySet().stream()
+              .sorted(Comparator.comparing(entry -> String.valueOf(entry.getKey())))
+              .map(entry -> String.valueOf(entry.getKey()) + ": " + abbreviate(seen, entry.getValue()))
+              .collect(Collectors.joining(", ")));
+      value.append("}");
+    } else {
+      String s = String.valueOf(stringify);
+      if (s.length() > 30) {
+        value.append(s.substring(0, 27)).append("...");
+      } else {
+        value.append(s);
+      }
+    }
+
+    seen.put(stringify, value.toString());
+    return value.toString();
+  }
+
+}

java/client/src/org/openqa/selenium/BUCK

@@ -35,6 +35,7 @@ java_library(name = 'core',
   maven_coords = 'org.seleniumhq.selenium:selenium-api:' + SE_VERSION,
   maven_pom_template = ':template-pom',
   srcs = [
+    'AbstractCapabilities.java',
     'Alert.java',
     'By.java',
     'Capabilities.java',

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

@@ -19,20 +19,21 @@
 
 
 import java.io.Serializable;
-import java.lang.reflect.Array;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.IdentityHashMap;
 import java.util.Map;
+import java.util.TreeMap;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
-public class ImmutableCapabilities implements Capabilities, Serializable {
+public class ImmutableCapabilities extends AbstractCapabilities implements Serializable {
 
   private static final long serialVersionUID = 665766108972704060L;
 
-  private final Map<String, Object> caps = new HashMap<>();
-
-  public ImmutableCapabilities() { }
+  public ImmutableCapabilities() {
+  }
 
   public ImmutableCapabilities(String k, Object v) {
     caps.put(k, v);
@@ -49,16 +50,23 @@ public ImmutableCapabilities(String k1, Object v1, String k2, Object v2, String
     caps.put(k3, v3);
   }
 
-  public ImmutableCapabilities(String k1, Object v1, String k2, Object v2, String k3, Object v3,
-                               String k4, Object v4) {
+  public ImmutableCapabilities(
+      String k1, Object v1,
+      String k2, Object v2,
+      String k3, Object v3,
+      String k4, Object v4) {
     caps.put(k1, v1);
     caps.put(k2, v2);
     caps.put(k3, v3);
     caps.put(k4, v4);
   }
 
-  public ImmutableCapabilities(String k1, Object v1, String k2, Object v2, String k3, Object v3,
-                               String k4, Object v4, String k5, Object v5) {
+  public ImmutableCapabilities(
+      String k1, Object v1,
+      String k2, Object v2,
+      String k3, Object v3,
+      String k4, Object v4,
+      String k5, Object v5) {
     caps.put(k1, v1);
     caps.put(k2, v2);
     caps.put(k3, v3);
@@ -128,76 +136,4 @@ public int hashCode() {
     return caps.hashCode();
   }
 
-  @Override
-  public String toString() {
-    Map<Object, String> seen = new IdentityHashMap<>();
-    StringBuilder builder = new StringBuilder("Capabilities ");
-    abbreviate(seen, builder, caps);
-    return builder.toString();
-  }
-
-  private void abbreviate(
-      Map<Object, String> seen,
-      StringBuilder builder,
-      Object stringify) {
-
-    if (stringify == null) {
-      builder.append("null");
-      return;
-    }
-
-    StringBuilder value = new StringBuilder();
-
-    if (stringify.getClass().isArray()) {
-      Array ary = (Array) stringify;
-      value.append("[");
-      int length = Array.getLength(ary);
-      for (int i = 0; i < length; i++) {
-        abbreviate(seen, value, Array.get(ary, i));
-        if (i < length - 1) {
-          value.append(", ");
-        }
-      }
-      value.append("]");
-    } else if (stringify instanceof Collection) {
-      Collection<?> c = (Collection<?>) stringify;
-      value.append("[");
-      int length = c.size();
-      int i = 0;
-
-      for (Object o : c) {
-        abbreviate(seen, value, o);
-        if (i < length - 1) {
-          value.append(", ");
-        }
-        i++;
-      }
-      value.append("]");
-    } else if (stringify instanceof Map) {
-      value.append("{");
-
-      Map<?, ?> m = (Map<?, ?>) stringify;
-      int length = m.size();
-      int i = 0;
-      for (Map.Entry<?, ?> entry : m.entrySet()) {
-        abbreviate(seen, value, entry.getKey());
-        value.append("=");
-        abbreviate(seen, value, entry.getValue());
-        if (i < length - 1) {
-          value.append(", ");
-        }
-      }
-      value.append("}");
-    } else {
-      String s = String.valueOf(stringify);
-      if (s.length() > 30) {
-        value.append(s.substring(0, 27)).append("...");
-      } else {
-        value.append(s);
-      }
-    }
-
-    seen.put(stringify, value.toString());
-    builder.append(value.toString());
-  }
-}
+}

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

@@ -22,14 +22,19 @@
 import org.openqa.selenium.logging.LoggingPreferences;
 
 import java.io.Serializable;
+import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.TreeMap;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
-public class MutableCapabilities implements Capabilities, Serializable {
+public class MutableCapabilities extends AbstractCapabilities implements Serializable {
 
   private static final long serialVersionUID = -112816287184979465L;
 
@@ -46,9 +51,6 @@ public class MutableCapabilities implements Capabilities, Serializable {
     OPTION_KEYS = Collections.unmodifiableSet(keys);
   }
 
-  private final Map<String, Object> caps = new HashMap<>();
-
-
   public MutableCapabilities() {
     // no-arg constructor
   }
@@ -175,29 +177,4 @@ public void setCapability(String key, Object value) {
       caps.put(key, value);
     }
   }
-
-  @Override
-  public String toString() {
-    return String.format("Capabilities [%s]", shortenMapValues(asMap()));
-  }
-
-  private Map<String, ?> shortenMapValues(Map<String, ?> map) {
-    Map<String, Object> newMap = new HashMap<>();
-
-    for (Map.Entry<String, ?> entry : map.entrySet()) {
-      if (entry.getValue() instanceof Map) {
-        @SuppressWarnings("unchecked") Map<String, ?> value = (Map<String, ?>) entry.getValue();
-        newMap.put(entry.getKey(), shortenMapValues(value));
-
-      } else {
-        String value = String.valueOf(entry.getValue());
-        if (value.length() > 1024) {
-          value = value.substring(0, 29) + "...";
-        }
-        newMap.put(entry.getKey(), value);
-      }
-    }
-
-    return newMap;
-  }
 }

java/client/test/org/openqa/selenium/remote/DesiredCapabilitiesTest.java

@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -128,7 +129,8 @@ public void shouldShortenLongValues() {
     }};
 
     DesiredCapabilities caps = new DesiredCapabilities(capabilitiesMap);
-    assertEquals(caps.toString().length(), 53);
+    String expected = "key: " + createString(27) + "...";
+    assertTrue(caps.toString(), caps.toString().contains(expected));
   }
 
   @Test
@@ -140,7 +142,8 @@ public void shouldShortenLongEnclosedValues() {
     }};
 
     DesiredCapabilities caps = new DesiredCapabilities(capabilitiesMap);
-    assertEquals(caps.toString().length(), 62);
+    String expected = "{subkey: " + createString(27) + "..." + "}";
+    assertTrue(caps.toString(), caps.toString().contains(expected));
   }
 
   @Test

java/client/test/org/openqa/selenium/support/ui/WebDriverWaitTest.java

@@ -30,8 +30,6 @@
 
 import org.junit.Before;
 import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.openqa.selenium.Capabilities;
@@ -51,7 +49,6 @@
 
 import java.io.IOException;
 
-@RunWith(JUnit4.class)
 public class WebDriverWaitTest {
 
   @Mock private WebDriver mockDriver;
@@ -80,7 +77,7 @@ public void shouldIncludeRemoteInfoForWrappedDriverTimeout() throws IOException
     Throwable ex = catchThrowable(() -> wait.until((d) -> false));
     assertNotNull(ex);
     assertThat(ex, instanceOf(TimeoutException.class));
-    assertThat(ex.getMessage(), containsString("Capabilities [{javascriptEnabled=true, platformName=ANY, platform=ANY}]"));
+    assertThat(ex.getMessage(), containsString("Capabilities {javascriptEnabled: true, platform: ANY, platformName: ANY}"));
     assertThat(ex.getMessage(), containsString("Session ID: foo"));
   }