Merge pull request #246 from launchdarkly/eb/ch77594/coverage-4-misc

(5.0 - #10) test coverage improvements + minor fixes

Author: eli-darkly

CONTRIBUTING.md

@@ -48,10 +48,6 @@ The project in the `benchmarks` subdirectory uses [JMH](https://openjdk.java.net
 
 ## Code coverage
 
-It is important to keep unit test coverage as close to 100% as possible in this project.
+It is important to keep unit test coverage as close to 100% as possible in this project. You can view the latest code coverage report in CircleCI, as `coverage/html/index.html` in the artifacts for the "Java 11 - Linux - OpenJDK" job. You can also run the report locally with `./gradlew jacocoTestCoverage` and view `./build/reports/jacoco/test`.
 
-Sometimes a gap in coverage is unavoidable, usually because the compiler requires us to provide a code path for some condition that in practice can't happen and can't be tested, or because of a known issue with the code coverage tool. Please handle all such cases as follows:
-
-* Mark the code with an explanatory comment beginning with "COVERAGE:".
-
-The current coverage report can be observed by running `./gradlew jacocoTestReport` and viewing `build/reports/jacoco/test/html/index.html`. This report is also produced as an artifact of the CircleCI build for the most recent Java version.
+Sometimes a gap in coverage is unavoidable, usually because the compiler requires us to provide a code path for some condition that in practice can't happen and can't be tested, or because of a known issue with the code coverage tool. In all such cases, please mark the code with an explanatory comment beginning with "COVERAGE:".

src/main/java/com/launchdarkly/sdk/server/Components.java

@@ -50,6 +50,8 @@
  * @since 4.0.0
  */
 public abstract class Components {
+  private Components() {}
+  
   /**
    * Returns a configuration object for using the default in-memory implementation of a data store.
    * <p>

src/main/java/com/launchdarkly/sdk/server/EventOutputFormatter.java

@@ -16,6 +16,8 @@
  * Transforms analytics events and summary data into the JSON format that we send to LaunchDarkly.
  * Rather than creating intermediate objects to represent this schema, we use the Gson streaming
  * output API to construct JSON directly.
+ * 
+ * Test coverage for this logic is in EventOutputTest and DefaultEventProcessorOutputTest.
  */
 final class EventOutputFormatter {
   private final EventsConfiguration config;
@@ -26,14 +28,12 @@ final class EventOutputFormatter {
     this.gson = JsonHelpers.gsonInstanceForEventsSerialization(config);
   }
 
-  int writeOutputEvents(Event[] events, EventSummarizer.EventSummary summary, Writer writer) throws IOException {
-    int count = 0;    
+  final int writeOutputEvents(Event[] events, EventSummarizer.EventSummary summary, Writer writer) throws IOException {
+    int count = events.length;    
     try (JsonWriter jsonWriter = new JsonWriter(writer)) {
       jsonWriter.beginArray();
       for (Event event: events) {
-        if (writeOutputEvent(event, jsonWriter)) {
-          count++;
-        }
+        writeOutputEvent(event, jsonWriter);
       }
       if (!summary.isEmpty()) {
         writeSummaryEvent(summary, jsonWriter);
@@ -44,7 +44,7 @@ int writeOutputEvents(Event[] events, EventSummarizer.EventSummary summary, Writ
     return count;
   }
 
-  private boolean writeOutputEvent(Event event, JsonWriter jw) throws IOException {
+  private final void writeOutputEvent(Event event, JsonWriter jw) throws IOException {
     if (event instanceof Event.FeatureRequest) {
       Event.FeatureRequest fe = (Event.FeatureRequest)event;
       startEvent(fe, fe.isDebug() ? "debug" : "feature", fe.getKey(), jw);
@@ -83,13 +83,10 @@ private boolean writeOutputEvent(Event event, JsonWriter jw) throws IOException
       startEvent(event, "index", null, jw);
       writeUser(event.getUser(), jw);
       jw.endObject();
-    } else {
-      return false;
     }
-    return true;
   }
 
-  private void writeSummaryEvent(EventSummarizer.EventSummary summary, JsonWriter jw) throws IOException {
+  private final void writeSummaryEvent(EventSummarizer.EventSummary summary, JsonWriter jw) throws IOException {
     jw.beginObject();
 
     jw.name("kind");
@@ -158,7 +155,7 @@ private void writeSummaryEvent(EventSummarizer.EventSummary summary, JsonWriter
     jw.endObject();
   }
 
-  private void startEvent(Event event, String kind, String key, JsonWriter jw) throws IOException {
+  private final void startEvent(Event event, String kind, String key, JsonWriter jw) throws IOException {
     jw.beginObject();
     jw.name("kind");
     jw.value(kind);
@@ -170,7 +167,7 @@ private void startEvent(Event event, String kind, String key, JsonWriter jw) thr
     }
   }
 
-  private void writeUserOrKey(Event event, boolean forceInline, JsonWriter jw) throws IOException {
+  private final void writeUserOrKey(Event event, boolean forceInline, JsonWriter jw) throws IOException {
     LDUser user = event.getUser();
     if (user != null) {
       if (config.inlineUsersInEvents || forceInline) {
@@ -182,14 +179,14 @@ private void writeUserOrKey(Event event, boolean forceInline, JsonWriter jw) thr
     }
   }
 
-  private void writeUser(LDUser user, JsonWriter jw) throws IOException {
+  private final void writeUser(LDUser user, JsonWriter jw) throws IOException {
     jw.name("user");
     // config.gson is already set up to use our custom serializer, which knows about private attributes
     // and already uses the streaming approach
     gson.toJson(user, LDUser.class, jw);
   }
 
-  private void writeLDValue(String key, LDValue value, JsonWriter jw) throws IOException {
+  private final void writeLDValue(String key, LDValue value, JsonWriter jw) throws IOException {
     if (value == null || value.isNull()) {
       return;
     }
@@ -198,38 +195,11 @@ private void writeLDValue(String key, LDValue value, JsonWriter jw) throws IOExc
   }
 
   // This logic is so that we don't have to define multiple custom serializers for the various reason subclasses.
-  private void writeEvaluationReason(String key, EvaluationReason er, JsonWriter jw) throws IOException {
+  private final void writeEvaluationReason(String key, EvaluationReason er, JsonWriter jw) throws IOException {
     if (er == null) {
       return;
     }
     jw.name(key);
-    
-    jw.beginObject();
-    
-    jw.name("kind");
-    jw.value(er.getKind().name());
-    
-    switch (er.getKind()) {
-    case ERROR:
-      jw.name("errorKind");
-      jw.value(er.getErrorKind().name());
-      break;
-    case PREREQUISITE_FAILED:
-      jw.name("prerequisiteKey");
-      jw.value(er.getPrerequisiteKey());
-      break;
-    case RULE_MATCH:
-      jw.name("ruleIndex");
-      jw.value(er.getRuleIndex());
-      if (er.getRuleId() != null) {
-        jw.name("ruleId");
-        jw.value(er.getRuleId());
-      }
-      break;
-    default:
-      break;
-    }   
-    
-    jw.endObject();
+    gson.toJson(er, EvaluationReason.class, jw); // EvaluationReason defines its own custom serializer
   }
 }

src/main/java/com/launchdarkly/sdk/server/EventSummarizer.java

@@ -91,12 +91,15 @@ public boolean equals(Object other) {
         EventSummary o = (EventSummary)other;
         return o.counters.equals(counters) && startDate == o.startDate && endDate == o.endDate;
       }
-      return true;
+      return false;
     }
 
     @Override
     public int hashCode() {
-      return counters.hashCode() + 31 * ((int)startDate + 31 * (int)endDate);
+      // We can't make meaningful hash codes for EventSummary, because the same counters could be
+      // represented differently in our Map. It doesn't matter because there's no reason to use an
+      // EventSummary instance as a hash key.
+      return 0;
     }
   }
 
@@ -157,6 +160,7 @@ public boolean equals(Object other)
       }
       return false;
     }
+    
     @Override
     public String toString() {
       return "(" + count + "," + flagValue + "," + defaultVal + ")";

src/main/java/com/launchdarkly/sdk/server/EventUserSerialization.java

@@ -11,7 +11,8 @@
 import java.util.Set;
 import java.util.TreeSet;
 
-class EventUserSerialization {
+abstract class EventUserSerialization {
+  private EventUserSerialization() {}
 
   // Used internally when including users in analytics events, to ensure that private attributes are stripped out.
   static class UserAdapterWithPrivateAttributeBehavior extends TypeAdapter<LDUser> {

src/main/java/com/launchdarkly/sdk/server/InMemoryDataStore.java

@@ -2,14 +2,12 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.launchdarkly.sdk.LDValue;
 import com.launchdarkly.sdk.server.interfaces.DataStore;
 import com.launchdarkly.sdk.server.interfaces.DataStoreStatusProvider.CacheStats;
 import com.launchdarkly.sdk.server.interfaces.DataStoreTypes.DataKind;
 import com.launchdarkly.sdk.server.interfaces.DataStoreTypes.FullDataSet;
 import com.launchdarkly.sdk.server.interfaces.DataStoreTypes.ItemDescriptor;
 import com.launchdarkly.sdk.server.interfaces.DataStoreTypes.KeyedItems;
-import com.launchdarkly.sdk.server.interfaces.DiagnosticDescription;
 
 import java.io.IOException;
 import java.util.HashMap;
@@ -22,7 +20,7 @@
  * As of version 5.0.0, this is package-private; applications must use the factory method
  * {@link Components#inMemoryDataStore()}.
  */
-class InMemoryDataStore implements DataStore, DiagnosticDescription {
+class InMemoryDataStore implements DataStore {
   private volatile ImmutableMap<DataKind, Map<String, ItemDescriptor>> allData = ImmutableMap.of();
   private volatile boolean initialized = false;
   private Object writeLock = new Object();
@@ -120,9 +118,4 @@ public CacheStats getCacheStats() {
   public void close() throws IOException {
     return;
   }
-
-  @Override
-  public LDValue describeConfiguration(LDConfig config) {
-    return LDValue.of("memory");
-  }
 }

src/main/java/com/launchdarkly/sdk/server/JsonHelpers.java

@@ -22,6 +22,8 @@
 import static com.launchdarkly.sdk.server.DataModel.SEGMENTS;
 
 abstract class JsonHelpers {
+  private JsonHelpers() {}
+  
   private static final Gson gson = new Gson();
 
   /**

src/main/java/com/launchdarkly/sdk/server/LDConfig.java

@@ -20,7 +20,7 @@ public final class LDConfig {
   static final URI DEFAULT_EVENTS_URI = URI.create("https://events.launchdarkly.com");
   static final URI DEFAULT_STREAM_URI = URI.create("https://stream.launchdarkly.com");
 
-  private static final Duration DEFAULT_START_WAIT = Duration.ofSeconds(5);
+  static final Duration DEFAULT_START_WAIT = Duration.ofSeconds(5);
 
   protected static final LDConfig DEFAULT = new Builder().build();
 
@@ -49,18 +49,6 @@ protected LDConfig(Builder builder) {
     this.threadPriority = builder.threadPriority;
   }
 
-  LDConfig(LDConfig config) {
-    this.dataSourceFactory = config.dataSourceFactory;
-    this.dataStoreFactory = config.dataStoreFactory;
-    this.diagnosticOptOut = config.diagnosticOptOut;
-    this.eventProcessorFactory = config.eventProcessorFactory;
-    this.httpConfig = config.httpConfig;
-    this.loggingConfig = config.loggingConfig;
-    this.offline = config.offline;
-    this.startWait = config.startWait;
-    this.threadPriority = config.threadPriority;
-  }
-
   /**
    * A <a href="http://en.wikipedia.org/wiki/Builder_pattern">builder</a> that helps construct
    * {@link com.launchdarkly.sdk.server.LDConfig} objects. Builder calls can be chained, enabling the

src/main/java/com/launchdarkly/sdk/server/SemanticVersion.java

@@ -24,13 +24,15 @@ public InvalidVersionException(String message) {
   private final int minor;
   private final int patch;
   private final String prerelease;
+  private final String[] prereleaseComponents;
   private final String build;
 
   public SemanticVersion(int major, int minor, int patch, String prerelease, String build) {
     this.major = major;
     this.minor = minor;
     this.patch = patch;
     this.prerelease = prerelease;
+    this.prereleaseComponents = prerelease == null ? null : prerelease.split("\\.");
     this.build = build;
   }
 
@@ -89,6 +91,7 @@ public static SemanticVersion parse(String input, boolean allowMissingMinorAndPa
       minor = matcher.group("minor") == null ? 0 : Integer.parseInt(matcher.group("minor"));
       patch = matcher.group("patch") == null ? 0 : Integer.parseInt(matcher.group("patch"));
     } catch (NumberFormatException e) {
+      // COVERAGE: This should be impossible, because our regex should only match if these strings are numeric.
       throw new InvalidVersionException("Invalid semantic version");
     }
     String prerelease = matcher.group("prerel");
@@ -129,7 +132,7 @@ public int comparePrecedence(SemanticVersion other) {
     if (other.prerelease == null) {
       return -1;
     }
-    return compareIdentifiers(prerelease.split("\\."), other.prerelease.split("\\."));
+    return compareIdentifiers(prereleaseComponents, other.prereleaseComponents);
   }
 
   private int compareIdentifiers(String[] ids1, String[] ids2) {

src/main/java/com/launchdarkly/sdk/server/Util.java

@@ -19,7 +19,9 @@
 import okhttp3.Response;
 import okhttp3.Route;
 
-class Util {
+abstract class Util {
+  private Util() {}
+  
   static Headers.Builder getHeadersBuilderFor(String sdkKey, HttpConfiguration config) {
     Headers.Builder builder = new Headers.Builder()
         .add("Authorization", sdkKey)