Disabled top level tests are not marked as ignored in JUnit #656

kotlintest-core/src/main/kotlin/io/kotlintest/internal/topLevelTests.kt

@@ -24,6 +24,8 @@ fun topLevelTests(spec: Spec): List<TopLevelTest> {
 
   val focused = tests.find { it.name.startsWith("f:") }
 
+  // if we have no focused tests, then we default to the standard is active logic
+  // otherwise focused overrides
   return if (focused == null) {
     tests.map { TopLevelTest(it, isActive(it)) }
   } else {

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

@@ -168,7 +168,7 @@ class JUnitTestRunnerListener(private val listener: EngineExecutionListener,
               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 ?: "Test Ignored")
+                TestStatus.Ignored -> 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

@@ -44,24 +44,24 @@ class TestCaseExecutor(private val listener: TestEngineListener,
 
   private val logger = LoggerFactory.getLogger(this.javaClass)
 
-  suspend fun execute(testCase: TestCase, context: TestContext, onResult: (TestResult) -> Unit = { }) {
+  suspend fun execute(testCase: TestCase, active: Boolean, context: TestContext, onResult: (TestResult) -> Unit = { }) {
 
     try {
 
       // invoke the "before" callbacks here on the main executor
       context.launch(executor.asCoroutineDispatcher()) {
-        before(testCase)
+        before(testCase, active)
       }.join()
 
       val extensions = testCase.config.extensions +
           testCase.spec.extensions().filterIsInstance<TestCaseExtension>() +
           Project.testCaseExtensions()
 
-      // get active status here in case calling this function is expensive (eg
-      runExtensions(testCase, context, extensions) { result ->
+      // get active status here in case calling this function is expensive
+      runExtensions(testCase, active, context, extensions) { result ->
         // invoke the "after" callbacks here on the main executor
         context.launch(executor.asCoroutineDispatcher()) {
-          after(testCase, result)
+          after(testCase, active, result)
         }.join()
         onResult(result)
       }
@@ -73,103 +73,101 @@ class TestCaseExecutor(private val listener: TestEngineListener,
   }
 
   private suspend fun runExtensions(testCase: TestCase,
+                                    active: Boolean,
                                     context: TestContext,
                                     remaining: List<TestCaseExtension>,
                                     onComplete: suspend (TestResult) -> Unit) {
     when {
       remaining.isEmpty() -> {
-        val result = executeTestIfActive(testCase, context)
+        val result = executeTestIfActive(testCase, active, context)
         onComplete(result)
       }
       else -> {
         remaining.first().intercept(
             testCase,
-            { test, callback -> runExtensions(test, context, remaining.drop(1), callback) },
+            { test, callback -> runExtensions(test, active, context, remaining.drop(1), callback) },
             { onComplete(it) }
         )
       }
     }
   }
 
-  // exectues the test case or if the test is not active then returns a ignored test result
-  private suspend fun executeTestIfActive(testCase: TestCase, context: TestContext): TestResult {
-
-    return if (isActive(testCase)) {
-
-      listener.beforeTestCaseExecution(testCase)
+  private suspend fun executeTestIfActive(testCase: TestCase, active: Boolean, context: TestContext): TestResult {
+    return if (active) executeTest(testCase, context) else TestResult.Ignored
+  }
 
-      // if we have more than one requested thread, we run the tests inside a clean executor;
-      // otherwise we run on the same thread as the listeners to avoid issues where the before and
-      // after listeners  require the same thread as the test case.
-      // @see https://github.com/kotlintest/kotlintest/issues/447
-      val executor = when (testCase.config.threads) {
-        1 -> executor
-        else -> Executors.newFixedThreadPool(testCase.config.threads)!!
-      }
+  // exectues the test case or if the test is not active then returns a ignored test result
+  private suspend fun executeTest(testCase: TestCase, context: TestContext): TestResult {
+    listener.beforeTestCaseExecution(testCase)
+
+    // if we have more than one requested thread, we run the tests inside a clean executor;
+    // otherwise we run on the same thread as the listeners to avoid issues where the before and
+    // after listeners  require the same thread as the test case.
+    // @see https://github.com/kotlintest/kotlintest/issues/447
+    val executor = when (testCase.config.threads) {
+      1 -> executor
+      else -> Executors.newFixedThreadPool(testCase.config.threads)!!
+    }
 
-      val dispatcher = executor.asCoroutineDispatcher()
+    val dispatcher = executor.asCoroutineDispatcher()
 
-      // captures an error from the test case closures
-      val error = AtomicReference<Throwable?>(null)
+    // captures an error from the test case closures
+    val error = AtomicReference<Throwable?>(null)
 
-      val supervisorJob = context.launch {
+    val supervisorJob = context.launch {
 
-        val testCaseJobs = (0 until testCase.config.invocations).map {
-          async(dispatcher) {
-            listener.invokingTestCase(testCase, 1)
-            try {
-              testCase.test(context)
-              null
-            } catch(t: Throwable) {
-              t.unwrapIfReflectionCall()
-            }
+      val testCaseJobs = (0 until testCase.config.invocations).map {
+        async(dispatcher) {
+          listener.invokingTestCase(testCase, 1)
+          try {
+            testCase.test(context)
+            null
+          } catch (t: Throwable) {
+            t.unwrapIfReflectionCall()
           }
         }
-
-        testCaseJobs.forEach {
-          val testError = it.await()
-          error.compareAndSet(null, testError)
-        }
       }
 
-      // we schedule a timeout, (if timeout has been configured) which will fail the test with a timed-out status
-      if (testCase.config.timeout.nano > 0) {
-        scheduler.schedule({
-          error.compareAndSet(null, TimeoutException("Execution of test took longer than ${testCase.config.timeout}"))
-        }, testCase.config.timeout.toMillis(), TimeUnit.MILLISECONDS)
+      testCaseJobs.forEach {
+        val testError = it.await()
+        error.compareAndSet(null, testError)
       }
+    }
 
-      supervisorJob.invokeOnCompletion { e ->
-        error.compareAndSet(null, e)
-      }
+    // we schedule a timeout, (if timeout has been configured) which will fail the test with a timed-out status
+    if (testCase.config.timeout.nano > 0) {
+      scheduler.schedule({
+        error.compareAndSet(null, TimeoutException("Execution of test took longer than ${testCase.config.timeout}"))
+      }, testCase.config.timeout.toMillis(), TimeUnit.MILLISECONDS)
+    }
 
-      supervisorJob.join()
+    supervisorJob.invokeOnCompletion { e ->
+      error.compareAndSet(null, e)
+    }
 
-      // if the tests had their own special executor (ie threads > 1) then we need to shut it down
-      if (testCase.config.threads > 1) {
-        executor.shutdown()
-      }
+    supervisorJob.join()
 
-      val result = buildTestResult(error.get(), context.metaData())
+    // if the tests had their own special executor (ie threads > 1) then we need to shut it down
+    if (testCase.config.threads > 1) {
+      executor.shutdown()
+    }
 
-      listener.afterTestCaseExecution(testCase, result)
-      return result
+    val result = buildTestResult(error.get(), context.metaData())
 
-    } else {
-      TestResult.Ignored
-    }
+    listener.afterTestCaseExecution(testCase, result)
+    return result
   }
 
   /**
    * Handles all "before" listeners.
    */
-  private fun before(testCase: TestCase) {
+  private fun before(testCase: TestCase, active: Boolean) {
     listener.enterTestCase(testCase)
 
     val userListeners = testCase.spec.listeners() + testCase.spec + Project.listeners()
     userListeners.forEach {
       it.beforeTest(testCase.description)
-      if (isActive(testCase)) {
+      if (active) {
         it.beforeTest(testCase)
       }
     }
@@ -178,11 +176,11 @@ class TestCaseExecutor(private val listener: TestEngineListener,
   /**
    * Handles all "after" listeners.
    */
-  private fun after(testCase: TestCase, result: TestResult) {
+  private fun after(testCase: TestCase, active: Boolean, result: TestResult) {
     val userListeners = testCase.spec.listeners() + testCase.spec + Project.listeners()
     userListeners.reversed().forEach {
       it.afterTest(testCase.description, result)
-      if (isActive(testCase)) {
+      if (active) {
         it.afterTest(testCase, result)
       }
     }

kotlintest-runner/kotlintest-runner-jvm/src/main/kotlin/io/kotlintest/runner/jvm/spec/InstancePerLeafSpecRunner.kt

@@ -9,6 +9,7 @@ import io.kotlintest.TestContext
 import io.kotlintest.TestResult
 import io.kotlintest.TestType
 import io.kotlintest.extensions.TopLevelTest
+import io.kotlintest.internal.isActive
 import io.kotlintest.runner.jvm.TestCaseExecutor
 import io.kotlintest.runner.jvm.TestEngineListener
 import io.kotlintest.runner.jvm.instantiateSpec
@@ -58,7 +59,7 @@ class InstancePerLeafSpecRunner(listener: TestEngineListener,
   private val logger = LoggerFactory.getLogger(this.javaClass)
   private val counter = AtomicInteger(0)
 
-  data class Enqueued(val testCase: TestCase, val count: Int)
+  data class Enqueued(val testCase: TestCase, val active: Boolean, val count: Int)
 
   // the queue contains tests discovered to run next. We always run the tests with the "furthest" path first.
   private val queue = PriorityQueue<Enqueued>(Comparator<Enqueued> { o1, o2 ->
@@ -71,17 +72,17 @@ class InstancePerLeafSpecRunner(listener: TestEngineListener,
   private val results = mutableMapOf<TestCase, TestResult>()
 
   override fun execute(spec: Spec, topLevelTests: List<TopLevelTest>): Map<TestCase, TestResult> {
-    topLevelTests.filter { it.active }.forEach { enqueue(it.testCase) }
+    topLevelTests.filter { it.active }.forEach { enqueue(it.testCase, it.active) }
     while (queue.isNotEmpty()) {
       val element = queue.remove()
       execute(element.testCase)
     }
     return results
   }
 
-  private fun enqueue(testCase: TestCase) {
+  private fun enqueue(testCase: TestCase, active: Boolean) {
     logger.debug("Enqueuing test ${testCase.description.fullName()}")
-    queue.add(Enqueued(testCase, counter.getAndIncrement()))
+    queue.add(Enqueued(testCase, active, counter.getAndIncrement()))
   }
 
   // starts executing an enqueued test case
@@ -131,7 +132,7 @@ class InstancePerLeafSpecRunner(listener: TestEngineListener,
   }
 
   private suspend fun executeTarget(testCase: TestCase, scope: CoroutineScope) {
-    executor.execute(testCase, context(testCase, scope)) { result -> results[testCase] = result }
+    executor.execute(testCase, isActive(testCase), context(testCase, scope)) { result -> results[testCase] = result }
   }
 
   private suspend fun executeAncestor(testCase: TestCase, target: Description, scope: CoroutineScope) {
@@ -148,9 +149,9 @@ class InstancePerLeafSpecRunner(listener: TestEngineListener,
     private var first = false
     override fun description(): Description = current.description
     override suspend fun registerTestCase(testCase: TestCase) {
-      if (first) enqueue(testCase) else {
+      if (first) enqueue(testCase, isActive(testCase)) else {
         first = true
-        executor.execute(testCase, context(testCase, scope)) { result -> results[testCase] = result }
+        executor.execute(testCase, isActive(testCase), context(testCase, scope)) { result -> results[testCase] = result }
       }
     }
   }

kotlintest-runner/kotlintest-runner-jvm/src/main/kotlin/io/kotlintest/runner/jvm/spec/InstancePerTestSpecRunner.kt

@@ -9,6 +9,7 @@ import io.kotlintest.TestContext
 import io.kotlintest.TestResult
 import io.kotlintest.TestType
 import io.kotlintest.extensions.TopLevelTest
+import io.kotlintest.internal.isActive
 import io.kotlintest.runner.jvm.TestCaseExecutor
 import io.kotlintest.runner.jvm.TestEngineListener
 import io.kotlintest.runner.jvm.instantiateSpec
@@ -82,20 +83,20 @@ class InstancePerTestSpecRunner(listener: TestEngineListener,
    * stack, and begin executing that.
    */
   override fun execute(spec: Spec, topLevelTests: List<TopLevelTest>): Map<TestCase, TestResult> {
-    topLevelTests.filter { it.active }.forEach { enqueue(it.testCase) }
+    topLevelTests.forEach { enqueue(it.testCase, it.active) }
     while (queue.isNotEmpty()) {
       val element = queue.remove()
       execute(element.testCase)
     }
     return results
   }
 
-  private fun enqueue(testCase: TestCase) {
+  private fun enqueue(testCase: TestCase, active: Boolean) {
     if (discovered.contains(testCase.description))
       throw IllegalStateException("Cannot add duplicate test name ${testCase.name}")
     discovered.add(testCase.description)
     logger.debug("Enqueuing test ${testCase.description.fullName()}")
-    queue.add(InstancePerLeafSpecRunner.Enqueued(testCase, counter.getAndIncrement()))
+    queue.add(InstancePerLeafSpecRunner.Enqueued(testCase, active, counter.getAndIncrement()))
   }
 
   /**
@@ -136,12 +137,12 @@ class InstancePerTestSpecRunner(listener: TestEngineListener,
     if (target == current.description) {
       val context = object : TestContext(scope.coroutineContext) {
         override fun description(): Description = target
-        override suspend fun registerTestCase(testCase: TestCase) = enqueue(testCase)
+        override suspend fun registerTestCase(testCase: TestCase) = enqueue(testCase, isActive(testCase))
       }
       if (executed.contains(target))
         throw  IllegalStateException("Attempting to execute duplicate test")
       executed.add(target)
-      executor.execute(current, context) { results[current] = it }
+      executor.execute(current, isActive(current), context) { results[current] = it }
       // otherwise if it's an ancestor then we want to search it recursively
     } else if (current.description.isAncestorOf(target)) {
       current.test.invoke(object : TestContext(scope.coroutineContext) {

kotlintest-runner/kotlintest-runner-jvm/src/main/kotlin/io/kotlintest/runner/jvm/spec/SingleInstanceSpecRunner.kt

@@ -6,6 +6,7 @@ import io.kotlintest.TestCase
 import io.kotlintest.TestContext
 import io.kotlintest.TestResult
 import io.kotlintest.extensions.TopLevelTest
+import io.kotlintest.internal.isActive
 import io.kotlintest.runner.jvm.TestCaseExecutor
 import io.kotlintest.runner.jvm.TestEngineListener
 import kotlinx.coroutines.runBlocking
@@ -41,7 +42,7 @@ class SingleInstanceSpecRunner(listener: TestEngineListener,
       if (seen.containsKey(testCase.name) && seen[testCase.name] != testCase.line)
         throw IllegalStateException("Cannot add duplicate test name ${testCase.name}")
       seen[testCase.name] = testCase.line
-      executor.execute(testCase, Context(testCase.description, coroutineContext)) { result -> results[testCase] = result }
+      executor.execute(testCase, isActive(testCase), Context(testCase.description, coroutineContext)) { result -> results[testCase] = result }
     }
   }
 
@@ -51,10 +52,10 @@ class SingleInstanceSpecRunner(listener: TestEngineListener,
     // in the top level test cases being available on the spec class
     runBlocking {
       interceptSpec(spec) {
-        topLevelTests.filter { it.active }.map { it.testCase }.forEach { testCase ->
+        topLevelTests.forEach { (testCase, active) ->
           // each spec is allocated it's own thread so we can block here safely
           // allowing us to enter the coroutine world
-          executor.execute(testCase, Context(testCase.description, this.coroutineContext)) { result -> results[testCase] = result }
+          executor.execute(testCase, active, Context(testCase.description, this.coroutineContext)) { result -> results[testCase] = result }
         }
       }
     }