Fix for before/after test listeners not failing tests #842 (#865)

kotlintest-runner/kotlintest-runner-console/src/main/kotlin/io/kotlintest/runner/console/TeamCityConsoleWriter.kt

@@ -99,4 +99,11 @@ class TeamCityConsoleWriter : ConsoleWriter {
       }
     }
   }
+
+  override fun exitTestCase(testCase: TestCase, result: TestResult) {
+    if(result.status == TestStatus.Error || result.status == TestStatus.Failure) {
+      errors = true
+      insertDummyFailure(testCase.description, result.error)
+    }
+  }
 }

kotlintest-runner/kotlintest-runner-junit5/src/main/kotlin/io/kotlintest/runner/junit5/JUnitTestRunnerListener.kt

@@ -70,18 +70,18 @@ class JUnitTestRunnerListener(private val listener: EngineExecutionListener,
   data class ResultState(val testCase: TestCase, val result: TestResult)
 
   // contains a mapping of a Description to a junit TestDescription
-  private val descriptors = HashMap<Description, TestDescriptor>()
+  private val descriptors = mutableMapOf<Description, TestDescriptor>()
 
   // contains every test that was discovered but not necessarily executed
-  private val discovered = HashSet<Pair<Description, TestType>>()
+  private val discovered = linkedSetOf<Pair<Description, TestType>>()
 
   // contains a set of all the tests we have notified as started, to avoid
   // double notification when a test is set to run multiple times
-  private val started = HashSet<Description>()
+  private val started = mutableSetOf<Description>()
 
   // contains all the results generated by tests in this spec
   // we store them all and mark the tests as finished only when we exit the spec
-  private val results = HashSet<ResultState>()
+  private val results = mutableSetOf<ResultState>()
 
   override fun engineStarted(classes: List<KClass<out Spec>>) {
     logger.debug("Engine started; classes=[$classes]")
@@ -170,8 +170,6 @@ class JUnitTestRunnerListener(private val listener: EngineExecutionListener,
     // for each description we can grab the best result and use that
     discovered
         .filter { description.isAncestorOf(it.first) }
-        .sortedBy { it.first.depth() }
-        .reversed()
         .forEach {
           val descriptor = descriptors.getOrPut(it.first) { createTestCaseDescriptor(it.first, it.second) }
           // find an error by priority
@@ -184,8 +182,13 @@ class JUnitTestRunnerListener(private val listener: EngineExecutionListener,
             try {
               when (result.status) {
                 TestStatus.Success -> listener.executionFinished(descriptor, TestExecutionResult.successful())
-                TestStatus.Error, TestStatus.Failure -> listener.executionFinished(descriptor, TestExecutionResult.failed(result.error))
-                TestStatus.Ignored -> listener.executionSkipped(descriptor, result.reason ?: "No reason given")
+                TestStatus.Error, TestStatus.Failure -> listener.executionFinished(descriptor,
+                    TestExecutionResult.failed(result.error))
+                TestStatus.Ignored -> {
+                  if (started.contains(it.first))
+                    throw RuntimeException("A skipped test should never have been started")
+                  listener.executionSkipped(descriptor, result.reason ?: "No reason given")
+                }
               }
             } catch (t: Throwable) {
               logger.error("Error in JUnit Platform listener", t)

kotlintest-runner/kotlintest-runner-jvm/src/main/kotlin/io/kotlintest/runner/jvm/TestCaseExecutor.kt

@@ -8,6 +8,7 @@ import io.kotlintest.internal.unwrapIfReflectionCall
 import kotlinx.coroutines.asCoroutineDispatcher
 import kotlinx.coroutines.async
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.slf4j.LoggerFactory
 import java.util.concurrent.*
 import java.util.concurrent.atomic.AtomicReference
@@ -41,9 +42,17 @@ class TestCaseExecutor(private val listener: TestEngineListener,
     try {
 
       // invoke the "before" callbacks here on the main executor
-      context.launch(executor.asCoroutineDispatcher()) {
-        before(testCase)
-      }.join()
+      try {
+        withContext(context.coroutineContext + executor.asCoroutineDispatcher()) {
+          before(testCase)
+        }
+        // an exception in the before block means the "invokingTestCase" listener will never be invoked
+        // we should do so here to ensure junit is happy as it requires tests to be started or skipped
+        // todo this functionality should be handled by the junit listener when we refactor for 4.0
+      } catch (t: Throwable) {
+        listener.invokingTestCase(testCase, 1)
+        throw t
+      }
 
       val extensions = testCase.config.extensions +
           testCase.spec.extensions().filterIsInstance<TestCaseExtension>() +
@@ -52,9 +61,9 @@ class TestCaseExecutor(private val listener: TestEngineListener,
       // get active status here in case calling this function is expensive
       runExtensions(testCase, context, extensions) { result ->
         // invoke the "after" callbacks here on the main executor
-        context.launch(executor.asCoroutineDispatcher()) {
+        withContext(context.coroutineContext + executor.asCoroutineDispatcher()) {
           after(testCase, result)
-        }.join()
+        }
         onResult(result)
       }
 

kotlintest-tests/kotlintest-tests-junit5/src/test/kotlin/com/sksamuel/kotlintest/junit5/StringSpecEngineKitTest.kt

@@ -34,9 +34,10 @@ class StringSpecEngineKitTest : FunSpec({
         .selectors(selectClass(StringSpecExceptionInInit::class.java))
         .execute()
 
+    results.all().list().size shouldBe 5
+
     results.all().list().apply {
       assertSoftly {
-        size shouldBe 5
 
         this[0].type shouldBe EventType.STARTED
         this[1].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
@@ -74,10 +75,10 @@ class StringSpecEngineKitTest : FunSpec({
         .selectors(selectClass(StringSpecExceptionInBeforeSpec::class.java))
         .execute()
 
+    results.all().list().size shouldBe 5
+
     results.all().list().apply {
       assertSoftly {
-        size shouldBe 5
-
         this[0].type shouldBe EventType.STARTED
         this[1].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
         this[2].type shouldBe EventType.STARTED
@@ -115,10 +116,10 @@ class StringSpecEngineKitTest : FunSpec({
         .selectors(selectClass(StringSpecExceptionInAfterSpec::class.java))
         .execute()
 
+    results.all().list().size shouldBe 11
+
     results.all().list().apply {
       assertSoftly {
-        size shouldBe 11
-
         this[0].type shouldBe EventType.STARTED
         this[1].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
         this[2].type shouldBe EventType.STARTED
@@ -138,8 +139,8 @@ class StringSpecEngineKitTest : FunSpec({
         this[4].testDescriptor.displayName shouldBe "a failing test"
         this[5].testDescriptor.displayName shouldBe "a passing test"
         this[6].testDescriptor.displayName shouldBe "a passing test"
-        this[7].testDescriptor.displayName shouldBe "a passing test"
-        this[8].testDescriptor.displayName shouldBe "a failing test"
+        this[7].testDescriptor.displayName shouldBe "a failing test"
+        this[8].testDescriptor.displayName shouldBe "a passing test"
         this[9].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInAfterSpec"
         this[10].testDescriptor.displayName shouldBe "KotlinTest"
       }
@@ -161,4 +162,109 @@ class StringSpecEngineKitTest : FunSpec({
       }
     }
   }
+
+  test("exception in before test") {
+
+    val results = EngineTestKit
+        .engine("kotlintest")
+        .selectors(selectClass(StringSpecExceptionInBeforeTest::class.java))
+        .execute()
+
+    results.all().list().size shouldBe 9
+
+    results.all().list().apply {
+      assertSoftly {
+
+        this[0].type shouldBe EventType.STARTED
+        this[1].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[2].type shouldBe EventType.STARTED
+        this[3].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[4].type shouldBe EventType.FINISHED
+        this[5].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[6].type shouldBe EventType.FINISHED
+        this[7].type shouldBe EventType.FINISHED
+        this[8].type shouldBe EventType.FINISHED
+
+        this[0].testDescriptor.displayName shouldBe "KotlinTest"
+        this[1].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInBeforeTest"
+        this[2].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInBeforeTest"
+        this[3].testDescriptor.displayName shouldBe "a failing test"
+        this[4].testDescriptor.displayName shouldBe "a failing test"
+        this[5].testDescriptor.displayName shouldBe "a passing test"
+        this[6].testDescriptor.displayName shouldBe "a passing test"
+        this[7].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInBeforeTest"
+        this[8].testDescriptor.displayName shouldBe "KotlinTest"
+      }
+    }
+
+    results.all().failed().list().apply {
+      assertSoftly {
+        size shouldBe 2
+        this[0].testDescriptor.displayName shouldBe "a failing test"
+        this[1].testDescriptor.displayName shouldBe "a passing test"
+      }
+    }
+
+    results.all().succeeded().list().apply {
+      assertSoftly {
+        size shouldBe 2
+        this[0].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInBeforeTest"
+        this[1].testDescriptor.displayName shouldBe "KotlinTest"
+      }
+    }
+  }
+
+  test("exception in after test") {
+
+    val results = EngineTestKit
+        .engine("kotlintest")
+        .selectors(selectClass(StringSpecExceptionInAfterTest::class.java))
+        .execute()
+
+    results.all().list().size shouldBe 11
+
+    results.all().list().apply {
+      assertSoftly {
+        this[0].type shouldBe EventType.STARTED
+        this[1].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[2].type shouldBe EventType.STARTED
+        this[3].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[4].type shouldBe EventType.STARTED
+        this[5].type shouldBe EventType.DYNAMIC_TEST_REGISTERED
+        this[6].type shouldBe EventType.STARTED
+        this[7].type shouldBe EventType.FINISHED
+        this[8].type shouldBe EventType.FINISHED
+        this[9].type shouldBe EventType.FINISHED
+        this[10].type shouldBe EventType.FINISHED
+
+        this[0].testDescriptor.displayName shouldBe "KotlinTest"
+        this[1].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInAfterTest"
+        this[2].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInAfterTest"
+        this[3].testDescriptor.displayName shouldBe "a failing test"
+        this[4].testDescriptor.displayName shouldBe "a failing test"
+        this[5].testDescriptor.displayName shouldBe "a passing test"
+        this[6].testDescriptor.displayName shouldBe "a passing test"
+        this[7].testDescriptor.displayName shouldBe "a failing test"
+        this[8].testDescriptor.displayName shouldBe "a passing test"
+        this[9].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInAfterTest"
+        this[10].testDescriptor.displayName shouldBe "KotlinTest"
+      }
+    }
+
+    results.all().failed().list().apply {
+      assertSoftly {
+        size shouldBe 2
+        this[0].testDescriptor.displayName shouldBe "a failing test"
+        this[1].testDescriptor.displayName shouldBe "a passing test"
+      }
+    }
+
+    results.all().succeeded().list().apply {
+      assertSoftly {
+        size shouldBe 2
+        this[0].testDescriptor.displayName shouldBe "com.sksamuel.kotlintest.junit5.StringSpecExceptionInAfterTest"
+        this[1].testDescriptor.displayName shouldBe "KotlinTest"
+      }
+    }
+  }
 })

kotlintest-tests/kotlintest-tests-junit5/src/test/kotlin/com/sksamuel/kotlintest/junit5/StringSpecExceptionInAfterTest.kt

@@ -0,0 +1,23 @@
+package com.sksamuel.kotlintest.junit5
+
+import io.kotlintest.TestCase
+import io.kotlintest.TestResult
+import io.kotlintest.shouldBe
+import io.kotlintest.specs.StringSpec
+
+class StringSpecExceptionInAfterTest : StringSpec() {
+
+  init {
+    "a failing test" {
+      1 shouldBe 2
+    }
+
+    "a passing test" {
+      1 shouldBe 1
+    }
+  }
+
+  override fun afterTest(testCase: TestCase, result: TestResult) {
+    throw RuntimeException("craack!!")
+  }
+}

kotlintest-tests/kotlintest-tests-junit5/src/test/kotlin/com/sksamuel/kotlintest/junit5/StringSpecExceptionInBeforeTest.kt

@@ -0,0 +1,22 @@
+package com.sksamuel.kotlintest.junit5
+
+import io.kotlintest.TestCase
+import io.kotlintest.shouldBe
+import io.kotlintest.specs.StringSpec
+
+class StringSpecExceptionInBeforeTest : StringSpec() {
+
+  init {
+    "a failing test" {
+      1 shouldBe 2
+    }
+
+    "a passing test" {
+      1 shouldBe 1
+    }
+  }
+
+  override fun beforeTest(testCase: TestCase) {
+    throw RuntimeException("oooff!!")
+  }
+}