Add cases to GoogleUtilitiesSpec. Does this improve coverage?

build.sbt

@@ -20,7 +20,7 @@ lazy val workbenchGoogle = project.in(file("google"))
   .settings(googleSettings:_*)
   .dependsOn(workbenchUtil)
   .dependsOn(workbenchModel)
-  .dependsOn(workbenchMetrics)
+  .dependsOn(workbenchMetrics % testAndCompile)
   .withTestSettings
 
 lazy val workbenchLibs = project.in(file("."))

google/src/test/scala/org/broadinstitute/dsde/workbench/google/GoogleUtilitiesSpec.scala

@@ -7,18 +7,19 @@ import akka.testkit.TestKit
 import com.google.api.client.googleapis.json.GoogleJsonError.ErrorInfo
 import com.google.api.client.googleapis.json.{GoogleJsonError, GoogleJsonResponseException}
 import com.google.api.client.http._
+import org.broadinstitute.dsde.workbench.metrics.{StatsDTestUtils, WorkbenchInstrumented}
+import org.broadinstitute.dsde.workbench.util.MockitoTestUtils
 import org.scalatest.{BeforeAndAfterAll, FlatSpecLike, Matchers}
-import org.scalatest.concurrent.ScalaFutures
+import org.scalatest.concurrent.{Eventually, ScalaFutures}
 import spray.json._
 
 import scala.collection.JavaConverters._
 import scala.concurrent.ExecutionContext
 import scala.concurrent.duration._
 
-class GoogleUtilitiesSpec extends TestKit(ActorSystem("MySpec")) with GoogleUtilities with FlatSpecLike with BeforeAndAfterAll with Matchers with ScalaFutures {
+class GoogleUtilitiesSpec extends TestKit(ActorSystem("MySpec")) with GoogleUtilities with FlatSpecLike with BeforeAndAfterAll with Matchers with ScalaFutures with Eventually with MockitoTestUtils with StatsDTestUtils {
   implicit val executionContext = ExecutionContext.global
-  override val workbenchMetricBaseName = "test"
-  implicit val histo = ExpandedMetricBuilder.empty.asHistogram("test")
+  implicit def histo = ExpandedMetricBuilder.empty.asHistogram("histo")
 
   override def afterAll {
     TestKit.shutdownActorSystem(system)
@@ -94,44 +95,64 @@ class GoogleUtilitiesSpec extends TestKit(ActorSystem("MySpec")) with GoogleUtil
   }
 
   "retryWhen500orGoogleError" should "retry once per backoff interval and then fail" in {
-    val counter = new Counter()
-    whenReady(retryWhen500orGoogleError(counter.alwaysBoom).failed) { f =>
-      f shouldBe a[IOException]
-      counter.counter shouldBe 4 //extra one for the first attempt
+    withStatsD {
+      val counter = new Counter()
+      whenReady(retryWhen500orGoogleError(counter.alwaysBoom).failed) { f =>
+        f shouldBe a[IOException]
+        counter.counter shouldBe 4 //extra one for the first attempt
+      }
+    } { capturedMetrics =>
+      capturedMetrics should contain ("test.histo.samples", "1")
+      capturedMetrics should contain ("test.histo.max", "4")  // 4 exceptions
     }
   }
 
   it should "not retry after a success" in {
-    val counter = new Counter()
-    whenReady(retryWhen500orGoogleError(counter.boomOnce)) { s =>
-      s shouldBe 42
-      counter.counter shouldBe 2
+    withStatsD {
+      val counter = new Counter()
+      whenReady(retryWhen500orGoogleError(counter.boomOnce)) { s =>
+        s shouldBe 42
+        counter.counter shouldBe 2
+      }
+    } { capturedMetrics =>
+      capturedMetrics should contain ("test.histo.samples", "1")
+      capturedMetrics should contain ("test.histo.max", "1")  // 1 exception
     }
   }
 
   "retryWithRecoverWhen500orGoogleError" should "stop retrying if it recovers" in {
-    val counter = new Counter()
+    withStatsD {
+      val counter = new Counter()
 
-    def recoverIO: PartialFunction[Throwable, Int] = {
-      case _: IOException => 42
-    }
+      def recoverIO: PartialFunction[Throwable, Int] = {
+        case _: IOException => 42
+      }
 
-    whenReady(retryWithRecoverWhen500orGoogleError(counter.alwaysBoom)(recoverIO)) { s =>
-      s shouldBe 42
-      counter.counter shouldBe 1
+      whenReady(retryWithRecoverWhen500orGoogleError(counter.alwaysBoom)(recoverIO)) { s =>
+        s shouldBe 42
+        counter.counter shouldBe 1
+      }
+    } { capturedMetrics =>
+      capturedMetrics should contain ("test.histo.samples", "1")
+      capturedMetrics should contain ("test.histo.max", "0")  // 0 exceptions
     }
   }
 
   it should "keep retrying and fail if it doesn't recover" in {
-    val counter = new Counter()
+    withStatsD {
+      val counter = new Counter()
 
-    def recoverHttp: PartialFunction[Throwable, Int] = {
-      case h: HttpResponseException if h.getStatusCode == 404 => 42
-    }
+      def recoverHttp: PartialFunction[Throwable, Int] = {
+        case h: HttpResponseException if h.getStatusCode == 404 => 42
+      }
 
-    whenReady(retryWithRecoverWhen500orGoogleError(counter.httpBoom)(recoverHttp).failed) { f =>
-      f shouldBe a[HttpResponseException]
-      counter.counter shouldBe 4 //extra one for the first attempt
+      whenReady(retryWithRecoverWhen500orGoogleError(counter.httpBoom)(recoverHttp).failed) { f =>
+        f shouldBe a[HttpResponseException]
+        counter.counter shouldBe 4 //extra one for the first attempt
+      }
+    } { capturedMetrics =>
+      capturedMetrics should contain ("test.histo.samples", "1")
+      capturedMetrics should contain ("test.histo.max", "4")  // 4 exceptions
     }
   }
 }

metrics/src/main/scala/org/broadinstitute/dsde/workbench/metrics/InstrumentedRetry.scala

@@ -18,12 +18,10 @@ trait InstrumentedRetry extends Retry {
     * Converts an RetryableFuture[A] to a Future[A].
     * Given an implicit Histogram, instruments the number of failures in the histogram.
     */
-  protected implicit def retryableFutureToFuture[A](af: RetryableFuture[A])(implicit histo: Histogram, executionContext: ExecutionContext): Future[A] = {
+  protected implicit def retryableFutureToFutureWithHisto[A](af: RetryableFuture[A])(implicit histo: Histogram, executionContext: ExecutionContext): Future[A] = {
     val instrumentedAf = af.andThen {
-      case Success(Left(errors)) =>
-        histo += errors.toList.size
-      case Success(Right((errors, _))) =>
-        histo += errors.size
+      case Success(Left(errors)) => histo += errors.toList.size
+      case Success(Right((errors, _))) => histo += errors.size
     }
     super.retryableFutureToFuture(instrumentedAf)
   }

metrics/src/main/scala/org/broadinstitute/dsde/workbench/metrics/WorkbenchInstrumented.scala

@@ -51,7 +51,8 @@ trait WorkbenchInstrumented extends DefaultInstrumented {
     def asHistogram(name: String): Histogram =
       metrics.histogram(makeName(name))
 
-    private def makeName(name: String): String = s"$m.$name"
+    private def makeName(name: String): String =
+      if (m.nonEmpty) s"$m.$name" else name
 
     override def toString: String = m
   }

metrics/src/test/scala/org/broadinstitute/dsde/workbench/metrics/StatsDTestUtils.scala

@@ -16,7 +16,7 @@ import scala.concurrent.duration._
   */
 trait StatsDTestUtils { this: Eventually with MockitoTestUtils =>
 
-  protected def workbenchMetricBaseName = "test"
+  protected val workbenchMetricBaseName = "test"
   def clearRegistries(): Unit = SharedMetricRegistries.getOrCreate("default").removeMatching(MetricFilter.ALL)
 
   protected def withStatsD[T](testCode: => T)(verify: Seq[(String, String)] => Unit = _ => ()): T = {

util/src/main/scala/org/broadinstitute/dsde/workbench/util/Retry.scala

@@ -34,11 +34,11 @@ trait Retry {
 
   val defaultErrorMessage = "retry-able operation failed"
 
-  def retry[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(op: () => Future[T])(implicit executionContext: ExecutionContext): Future[T] = {
+  def retry[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(op: () => Future[T])(implicit executionContext: ExecutionContext): RetryableFuture[T] = {
     retryInternal(allBackoffIntervals, pred, failureLogMessage)(op)
   }
 
-  def retryExponentially[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(op: () => Future[T])(implicit executionContext: ExecutionContext): Future[T] = {
+  def retryExponentially[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(op: () => Future[T])(implicit executionContext: ExecutionContext): RetryableFuture[T] = {
     retryInternal(exponentialBackOffIntervals, pred, failureLogMessage)(op)
   }
 
@@ -52,7 +52,7 @@ trait Retry {
    * @tparam T
    * @return
    */
-  def retryUntilSuccessOrTimeout[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(interval: FiniteDuration, timeout: FiniteDuration)(op: () => Future[T])(implicit executionContext: ExecutionContext): Future[T] = {
+  def retryUntilSuccessOrTimeout[T](pred: Predicate[Throwable] = always, failureLogMessage: String = defaultErrorMessage)(interval: FiniteDuration, timeout: FiniteDuration)(op: () => Future[T])(implicit executionContext: ExecutionContext): RetryableFuture[T] = {
     val trialCount = Math.ceil(timeout / interval).toInt
     retryInternal(Seq.fill(trialCount)(interval), pred, failureLogMessage)(op)
   }
@@ -95,7 +95,7 @@ trait Retry {
   /**
     * Converts an RetryableFuture[A] to a Future[A].
     */
-  protected implicit def retryableFutureToFuture[A](af: RetryableFuture[A])(implicit executionContext: ExecutionContext): Future[A] = {
+  protected[util] implicit def retryableFutureToFuture[A](af: RetryableFuture[A])(implicit executionContext: ExecutionContext): Future[A] = {
     af.flatMap {
       // take the head (most recent) error
       case Left(NonEmptyList(t, _)) => Future.failed(t)

util/src/test/scala/org/broadinstitute/dsde/workbench/util/RetrySpec.scala

@@ -34,8 +34,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   "Retry" should "retry 3 times by default" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retry()(() => testable.failure)
+    val result: Future[Int] = testable.retry()(() => testable.failure)
 
     // result should be a failure
     // invocationCount should be 4 (1 initial run plus 3 retries)
@@ -55,8 +56,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   it should "not retry upon success" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retry()(() => testable.success)
+    val result: Future[Int] = testable.retry()(() => testable.success)
 
     // result should a success
     // invocationCount should be 1
@@ -70,8 +72,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   it should "not retry if the predicate returns false" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retry(_ => false)(() => testable.failure)
+    val result: Future[Int] = testable.retry(_ => false)(() => testable.failure)
 
     // result should be a failure
     // invocationCount should be 1
@@ -86,9 +89,10 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   it should "log a custom error message" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
     val customLogMsg = "custom"
 
-    val result = testable.retry(failureLogMessage = customLogMsg)(() => testable.failure)
+    val result: Future[Int] = testable.retry(failureLogMessage = customLogMsg)(() => testable.failure)
 
     // result should be a failure
     // invocationCount should be 4 (1 initial run plus 3 retries)
@@ -110,8 +114,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
     implicit val patienceConfig = PatienceConfig(timeout = scaled(Span(2, Minutes)))
 
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retryExponentially()(() => testable.failure)
+    val result: Future[Int] = testable.retryExponentially()(() => testable.failure)
 
     // result should be a failure
     // invocationCount should be 7 (1 initial run plus 6 retries)
@@ -126,8 +131,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   it should "retry until a success" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retryUntilSuccessOrTimeout()(100 milliseconds, 1 minute)(() => testable.failureNTimes(10))
+    val result: Future[Int] = testable.retryUntilSuccessOrTimeout()(100 milliseconds, 1 minute)(() => testable.failureNTimes(10))
 
     // result should be a success
     // invocationCount should be 11 (10 failures and 1 success)
@@ -141,8 +147,9 @@ class RetrySpec extends TestKit(ActorSystem("MySpec")) with FlatSpecLike with Be
 
   it should "retry until a timeout" in {
     val testable = new TestRetry(system, setUpMockLogger)
+    import testable._
 
-    val result = testable.retryUntilSuccessOrTimeout()(100 milliseconds, 1 second)(() => testable.failure)
+    val result: Future[Int] = testable.retryUntilSuccessOrTimeout()(100 milliseconds, 1 second)(() => testable.failure)
 
     // result should be a failure
     // invocationCounts should be 11
@@ -180,7 +187,7 @@ class TestRetry(val system: ActorSystem, val slf4jLogger: SLF4JLogger) extends R
   }
   def failure = {
     increment
-    Future.failed(new Exception("test exception"))
+    Future.failed[Int](new Exception("test exception"))
   }
   def failureNTimes(n: Int) = {
     if (invocationCount < n) failure