inject-core: add 'runAfterAll' hook to IntegrationTestMixin

Problem

The `EmbeddedTwitterServer` has a `closeOnExit` method for
registering logical hooks for cleaning up test resources
when the server shuts down, but we do not have a general
utility for registering shutdown logic that is detached from
the `EmbeddedTwitterServer` lifecycle *in a simple, consistent
manner*. The current solution is to override ScalaTest
`afterAll` directly, which comes with its own pitfalls. This
can result in `super.afterAll` not being called by an inheritor
and not handling exceptions as part of clean-up can result in
unexpected TestSuite failures.

Solution

Introduce a `runAfterAll` method that can be called to register
shutdown logic that will not fail the `TestSuite` as a result
of an exception during clean-up. It should be highlighted that
we do not prevent anyone from explicitly overriding `afterAll`
for logic that should result in a test failure as a result of
an exception.

JIRA Issues: CSL-11136

Differential Revision: https://phabricator.twitter.biz/D707939

CHANGELOG.rst

@@ -7,6 +7,25 @@ Note that ``RB_ID=#`` and ``PHAB_ID=#`` correspond to associated message in comm
 Unreleased
 ----------
 
+Added
+~~~~~
+
+* inject-core: Introduce a `runAfterAll` hook in `c.t.inject.IntegrationTestMixin` to allow for
+  running logic to clean-up test resources in the `org.scalatest.BeforeAndAfterAll#afterAll` without
+  needing to 1) override `org.scalatest.BeforeAndAfterAll#afterAll`, 2) ensure `super` is called for
+  other resources clean-up, and 3) ensure all resources get cleaned up, regardless of non-fatal
+  exceptions thrown as part of the clean-up logic and otherwise fail the TestSuite run.
+  ``PHAB_ID=D707939``
+
+Changed
+~~~~~~~
+
+* finatra: Bump version of Jackson to 2.13.0 ``PHAB_ID=D744627``
+
+* http-server (BREAKING API CHANGE): Will now serialize many self-referential Jackson types as "{}"
+  instead of returning a serialization error.  See https://github.com/FasterXML/jackson-databind/commit/765e2fe1b7f6cdbc6855b32b39ba876fdff9fbcc
+  for more details. ``PHAB_ID=D744627``
+
 21.10.0
 -------
 
@@ -49,7 +68,7 @@ Fixed
 Breaking API Change
 ~~~~~~~~~~~~~~~~~~~
 
-* inject-utils: Removed deprecated `c.t.inject.conversions.string`, use 
+* inject-utils: Removed deprecated `c.t.inject.conversions.string`, use
   `c.t.conversions.StringOps` in the util/util-core project instead.
   ``PHAB_ID=D692729``
 

inject/inject-core/src/test/scala/com/twitter/inject/IntegrationTestMixin.scala

@@ -1,6 +1,9 @@
 package com.twitter.inject
 
-import org.scalatest.{Suite, SuiteMixin}
+import java.util.concurrent.ConcurrentLinkedQueue
+import org.scalatest.Suite
+import org.scalatest.SuiteMixin
+import scala.util.control.NonFatal
 
 /**
  * Testing trait which extends the [[com.twitter.inject.TestMixin]] to provide
@@ -16,7 +19,49 @@ import org.scalatest.{Suite, SuiteMixin}
  */
 trait IntegrationTestMixin extends SuiteMixin with TestMixin { this: Suite =>
 
+  // while the test execution (beforeAll, afterAll, beforeEach, afterEach, test, etc)
+  // is serial, we have no guarantees of where they will register afterAll functions, so
+  // we attempt to provide thread safety.
+  @volatile private[this] var afterAllStarted: Boolean = false
+  private[this] val afterAllFns: ConcurrentLinkedQueue[() => Unit] =
+    new ConcurrentLinkedQueue[() => Unit]()
+
   /* Protected */
 
   protected def injector: Injector
+
+  /**
+   * Logic that needs to be executed during the [[afterAll()]] block of tests. This method
+   * allows for resources, such as clients or servers, to be cleaned up after test execution
+   * without the need for multiple overrides to `afterAll` and calling `super`.
+   *
+   * @param f The logic to execute in the [[afterAll()]]
+   */
+  protected final def runAfterAll(f: => Unit): Unit =
+    if (afterAllStarted) executeFn(() => f) // execute inline if we've started `afterAll`
+    else afterAllFns.add(() => f) // otherwise we add it to the queue
+
+  /**
+   * When overriding, ensure that `super.afterAll()` is called.
+   */
+  override protected def afterAll(): Unit =
+    try {
+      super.afterAll()
+    } finally {
+      afterAllStarted = true
+      // we drain the queue and call the exits, regardless of exceptions
+      while (!afterAllFns.isEmpty) {
+        executeFn(afterAllFns.poll())
+      }
+    }
+
+  // we always try/catch because we don't want non-fatal errors to fail tests in our `afterAll`
+  private[this] def executeFn(f: () => Unit): Unit =
+    try {
+      f()
+    } catch {
+      case NonFatal(e) =>
+        e.printStackTrace(System.err)
+    }
+
 }

inject/inject-server/src/test/scala/com/twitter/inject/server/FeatureTestMixin.scala

@@ -1,8 +1,9 @@
 package com.twitter.inject.server
 
-import com.twitter.inject.{Injector, IntegrationTestMixin}
-import org.scalatest.{Suite, SuiteMixin}
-import scala.util.control.NonFatal
+import com.twitter.inject.Injector
+import com.twitter.inject.IntegrationTestMixin
+import org.scalatest.Suite
+import org.scalatest.SuiteMixin
 
 /**
  * Testing trait which extends the [[com.twitter.inject.IntegrationTestMixin]] to provide
@@ -23,6 +24,14 @@ trait FeatureTestMixin extends SuiteMixin with IntegrationTestMixin { this: Suit
 
   override protected def injector: Injector = server.injector
 
+  runAfterAll {
+    try {
+      server.close()
+    } finally {
+      server.assertCleanShutdown()
+    }
+  }
+
   def printStats: Boolean = false
 
   override protected def afterEach(): Unit = {
@@ -40,17 +49,4 @@ trait FeatureTestMixin extends SuiteMixin with IntegrationTestMixin { this: Suit
     }
   }
 
-  override protected def afterAll(): Unit = {
-    try {
-      super.afterAll()
-    } finally {
-      server.close()
-      try {
-        server.assertCleanShutdown()
-      } catch {
-        case NonFatal(e) =>
-          e.printStackTrace(System.err)
-      }
-    }
-  }
 }

inject/inject-server/src/test/scala/com/twitter/inject/server/tests/FeatureTestAfterAllTest.scala

@@ -0,0 +1,49 @@
+package com.twitter.inject.server.tests
+
+import com.twitter.inject.server.EmbeddedTwitterServer
+import com.twitter.inject.server.FeatureTest
+import com.twitter.inject.server.TwitterServer
+
+/** Test the `afterAll`` hooks of the [[FeatureTest]] trait */
+class FeatureTestAfterAllTest extends FeatureTest {
+
+  private[this] var hasAfterAllRun = false
+  private[this] var hasAfterAllSuppressedException = false
+  runAfterAll { hasAfterAllRun = true }
+  runAfterAll { throw new IllegalStateException("This shouldn't block other 'afterAll' hooks") }
+  runAfterAll { hasAfterAllSuppressedException = true }
+
+  override val server: EmbeddedTwitterServer =
+    new EmbeddedTwitterServer(
+      twitterServer = new TwitterServer {},
+      disableTestLogging = true
+    ).bind[String].toInstance("helloworld")
+
+  /**
+   * Explicitly start the server before all tests, close will be attempted by
+   * [[com.twitter.inject.server.FeatureTestMixin]] in `afterAll`.
+   */
+  override def beforeAll(): Unit = {
+    server.start()
+  }
+
+  /**
+   * We explicitly verify that we have run our `afterAll` and custom hooks.
+   * If any of these assertions fails, a [[org.scalatest.exceptions.TestFailedException]]
+   * will be thrown and the test will be considered a failure.
+   */
+  override def afterAll(): Unit = {
+    assert(hasAfterAllRun == false)
+    assert(hasAfterAllSuppressedException == false)
+    assert(server.closed == false)
+    super.afterAll() // we run all of the `afterAll` hooks here
+    assert(hasAfterAllRun == true)
+    assert(hasAfterAllSuppressedException)
+    assert(server.closed)
+  }
+
+  test("TwitterServer#starts up") {
+    server.assertHealthy()
+  }
+
+}