Discard JUnit 5 test runners even after creation, if further filters result in it not contributing any tests (#326)

mannodermaus · web-flow · commit d082fce0e1bd · 2024-04-13T14:15:32.000+02:00
This is interesting for `@Tag`-annotated classes that are skipped completely.
If their runner is not prevented from being returned to the instrumentation,
Android expects something reported back to it for the class, but in reality
JUnit 5 won't be reporting anything, causing a mismatch error
diff --git a/instrumentation/CHANGELOG.md b/instrumentation/CHANGELOG.md
@@ -6,7 +6,8 @@ Change Log
 - Fix inheritance hierarchy of `ComposeExtension` to avoid false-positive warning regarding `@RegisterExtension` (#318)
 - Improve parallel test execution for Android instrumentation tests
 - Fix invalid naming of dynamic tests when executing only a singular test method from the IDE (#317)
-- Prevent test methods incorrectly defined as Kotlin top-level functions from messing up Android's internal test counting, causing issues like "Expected N+1 tests, received N" (#316)  
+- Prevent test methods incorrectly defined as Kotlin top-level functions from messing up Android's internal test counting, causing issues like "Expected N+1 tests, received N" (#316)
+- Prevent test classes ignored by a tag from being considered for test execution, causing issues like "Expected N+1 tests, received N" (#298)
 
 ## 1.4.0 (2023-11-05)
 
diff --git a/instrumentation/core/build.gradle.kts b/instrumentation/core/build.gradle.kts
@@ -51,8 +51,10 @@ android {
 }
 
 junitPlatform {
-  // Using local dependency instead of Maven coordinates
-  instrumentationTests.enabled = false
+  filters {
+    // See TaggedTests.kt for usage of this tag
+    excludeTags("nope")
+  }
 }
 
 tasks.withType<KotlinCompile> {
@@ -68,6 +70,20 @@ tasks.withType<Test> {
   }
 }
 
+// Use local project dependencies on android-test instrumentation libraries
+// instead of relying on their Maven coordinates for this module
+val instrumentationLibraryRegex = Regex("de\\.mannodermaus\\.junit5:android-test-(.+):")
+
+configurations.all {
+  if ("debugAndroidTestRuntimeClasspath" in name) {
+    resolutionStrategy.dependencySubstitution.all {
+      instrumentationLibraryRegex.find(requested.toString())?.let { result ->
+        useTarget(project(":${result.groupValues[1]}"))
+      }
+    }
+  }
+}
+
 dependencies {
   implementation(libs.kotlinStdLib)
   implementation(libs.junitJupiterApi)
@@ -87,7 +103,6 @@ dependencies {
   androidTestImplementation(libs.junitJupiterParams)
   androidTestImplementation(libs.espressoCore)
   androidTestRuntimeOnly(project(":runner"))
-  androidTestRuntimeOnly(libs.junitJupiterEngine)
 
   testImplementation(project(":testutil"))
 }
diff --git a/instrumentation/core/src/androidTest/java/de/mannodermaus/junit5/TaggedTests.kt b/instrumentation/core/src/androidTest/java/de/mannodermaus/junit5/TaggedTests.kt
@@ -0,0 +1,25 @@
+package de.mannodermaus.junit5
+
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Tag
+import org.junit.jupiter.api.Test
+
+class TaggedTests {
+    @Test
+    fun includedTest() {
+    }
+
+    @Tag("nope")
+    @Test
+    fun taggedTestDisabledOnMethodLevel() {
+        assertEquals(5, 2 + 2)
+    }
+}
+
+@Tag("nope")
+class TaggedTestsDisabledOnClassLevel {
+    @Test
+    fun excludedTest() {
+        assertEquals(5, 2 + 2)
+    }
+}
diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/AndroidJUnit5Builder.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/AndroidJUnit5Builder.kt
@@ -1,9 +1,8 @@
 package de.mannodermaus.junit5
 
 import android.util.Log
-import de.mannodermaus.junit5.internal.runners.JUnit5RunnerFactory.createJUnit5Runner
 import de.mannodermaus.junit5.internal.LOG_TAG
-import de.mannodermaus.junit5.internal.extensions.jupiterTestMethods
+import de.mannodermaus.junit5.internal.runners.tryCreateJUnit5Runner
 import org.junit.runner.Runner
 import org.junit.runners.model.RunnerBuilder
 
@@ -60,16 +59,11 @@ public class AndroidJUnit5Builder : RunnerBuilder() {
     @Throws(Throwable::class)
     override fun runnerForClass(testClass: Class<*>): Runner? {
         try {
-            if (!junit5Available) {
-                return null
+            return if (junit5Available) {
+                tryCreateJUnit5Runner(testClass)
+            } else {
+                null
             }
-
-            if (testClass.jupiterTestMethods().isEmpty()) {
-                return null
-            }
-
-            return createJUnit5Runner(testClass)
-
         } catch (e: NoClassDefFoundError) {
             Log.e(LOG_TAG, "JUnitPlatform not found on runtime classpath")
             throw IllegalStateException(
diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformRunnerListener.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformRunnerListener.kt
@@ -63,11 +63,14 @@ internal class AndroidJUnitPlatformRunnerListener(
     ) {
         val description = testTree.getDescription(testIdentifier)
         val status = testExecutionResult.status
-        if (status == TestExecutionResult.Status.ABORTED) {
-            notifier.fireTestAssumptionFailed(toFailure(testExecutionResult, description))
-        } else if (status == TestExecutionResult.Status.FAILED) {
-            notifier.fireTestFailure(toFailure(testExecutionResult, description))
-        } else if (testIdentifier.isTest) {
+
+        if (testIdentifier.isTest) {
+            if (status == TestExecutionResult.Status.ABORTED) {
+                notifier.fireTestAssumptionFailed(toFailure(testExecutionResult, description))
+            } else if (status == TestExecutionResult.Status.FAILED) {
+                notifier.fireTestFailure(toFailure(testExecutionResult, description))
+            }
+
             notifier.fireTestFinished(description)
         }
     }
diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTree.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTree.kt
@@ -131,12 +131,19 @@ internal class AndroidJUnitPlatformTestTree(
         parent: Description,
         testPlan: TestPlan
     ) {
-        val newDescription = createJUnit4Description(identifier, testPlan)
-        parent.addChild(newDescription)
-        descriptions[identifier] = newDescription
+        val newDescription = createJUnit4Description(identifier, testPlan).also {
+            descriptions[identifier] = it
+        }
+
+        val newParent = if (identifier.isTest || identifier.isDynamicTest) {
+            parent.addChild(newDescription)
+            newDescription
+        } else {
+            parent
+        }
 
         testPlan.getChildren(identifier).forEach { child ->
-            buildDescription(child, newDescription, testPlan)
+            buildDescription(child, newParent, testPlan)
         }
     }
 
diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/DummyJUnit5.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/DummyJUnit5.kt
@@ -2,18 +2,19 @@ package de.mannodermaus.junit5.internal.runners
 
 import android.util.Log
 import de.mannodermaus.junit5.internal.LOG_TAG
-import de.mannodermaus.junit5.internal.extensions.jupiterTestMethods
 import org.junit.runner.Description
 import org.junit.runner.Runner
 import org.junit.runner.notification.RunNotifier
+import java.lang.reflect.Method
 
 /**
  * Fake Runner that marks all JUnit 5 methods as ignored,
  * used for old devices without Java 8 capabilities.
  */
-internal class DummyJUnit5(private val testClass: Class<*>) : Runner() {
-
-    private val testMethods = testClass.jupiterTestMethods()
+internal class DummyJUnit5(
+    private val testClass: Class<*>,
+    private val testMethods: Set<Method>,
+) : Runner() {
 
     override fun run(notifier: RunNotifier) {
         Log.w(
@@ -27,5 +28,9 @@ internal class DummyJUnit5(private val testClass: Class<*>) : Runner() {
         }
     }
 
-    override fun getDescription(): Description = Description.createSuiteDescription(testClass)
+    override fun getDescription(): Description = Description.createSuiteDescription(testClass).also {
+        testMethods.forEach { method ->
+            it.addChild(Description.createTestDescription(testClass, method.name))
+        }
+    }
 }
diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/JUnit5RunnerFactory.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/runners/JUnit5RunnerFactory.kt
@@ -1,20 +1,36 @@
 package de.mannodermaus.junit5.internal.runners
 
 import android.os.Build
+import de.mannodermaus.junit5.internal.extensions.jupiterTestMethods
 import org.junit.runner.Runner
 
-internal object JUnit5RunnerFactory {
-    /**
-     * Since we can't reference AndroidJUnit5 directly, use this factory for instantiation.
-     *
-     * On API 26 and above, delegate to the real implementation to drive JUnit 5 tests.
-     * Below that however, they wouldn't work; for this case, delegate a dummy runner
-     * which will highlight these tests as ignored.
-     */
-    internal fun createJUnit5Runner(klass: Class<*>): Runner =
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
-            AndroidJUnit5(klass)
-        } else {
-            DummyJUnit5(klass)
-        }
+/**
+ * Since we can't reference AndroidJUnit5 directly, use this factory for instantiation.
+ *
+ * On API 26 and above, delegate to the real implementation to drive JUnit 5 tests.
+ * Below that however, they wouldn't work; for this case, delegate a dummy runner
+ * which will highlight these tests as ignored.
+ */
+internal fun tryCreateJUnit5Runner(klass: Class<*>): Runner? {
+    val testMethods = klass.jupiterTestMethods()
+
+    if (testMethods.isEmpty()) {
+        return null
+    }
+
+    val runner = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+        AndroidJUnit5(klass)
+    } else {
+        DummyJUnit5(klass, testMethods)
+    }
+
+    // It's still possible for the runner to not be relevant to the test run,
+    // which is related to how further filters are applied (e.g. via @Tag).
+    // Only return the runner to the instrumentation if it has any tests to contribute,
+    // otherwise there would be a mismatch between the number of test classes reported
+    // to Android, and the number of test classes actually tested with JUnit 5 (ref #298)
+    return runner.takeIf(Runner::hasExecutableTests)
 }
+
+private fun Runner.hasExecutableTests() =
+    this.description.children.isNotEmpty()
diff --git a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/AndroidJUnit5BuilderTests.kt b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/AndroidJUnit5BuilderTests.kt
@@ -1,60 +1,71 @@
 package de.mannodermaus.junit5
 
+import android.os.Build
 import com.google.common.truth.Truth.assertThat
-import org.junit.jupiter.api.Test
-import org.junit.jupiter.params.ParameterizedTest
-import org.junit.jupiter.params.provider.ValueSource
+import de.mannodermaus.junit5.testutil.AndroidBuildUtils.withApiLevel
+import de.mannodermaus.junit5.testutil.AndroidBuildUtils.withMockedInstrumentation
+import org.junit.jupiter.api.DynamicContainer.dynamicContainer
+import org.junit.jupiter.api.DynamicNode
+import org.junit.jupiter.api.DynamicTest.dynamicTest
+import org.junit.jupiter.api.TestFactory
 
 class AndroidJUnit5BuilderTests {
 
     private val builder = AndroidJUnit5Builder()
 
-    @Test
-    fun `no runner is created if class only contains top-level test methods`() {
+    @TestFactory
+    fun `no runner is created if class only contains top-level test methods`() = runTest(
+        expectSuccess = false,
         // In Kotlin, a 'Kt'-suffixed class of top-level functions cannot be referenced
         // via the ::class syntax, so construct a reference to the class directly
-        val cls = Class.forName(javaClass.packageName + ".TestClassesKt")
-
-        // Top-level tests should be discarded, so no runner must be created for this class
-        runTest(cls, expectSuccess = false)
-    }
+        Class.forName(javaClass.packageName + ".TestClassesKt")
+    )
 
-    @ValueSource(
-        classes = [
-            HasTest::class,
-            HasRepeatedTest::class,
-            HasTestFactory::class,
-            HasTestTemplate::class,
-            HasParameterizedTest::class,
-            HasInnerClassWithTest::class,
-            HasTaggedTest::class,
-            HasInheritedTestsFromClass::class,
-            HasInheritedTestsFromInterface::class,
-        ]
+    @TestFactory
+    fun `runner is created correctly for classes with valid jupiter test methods`() = runTest(
+        expectSuccess = true,
+        HasTest::class.java,
+        HasRepeatedTest::class.java,
+        HasTestFactory::class.java,
+        HasTestTemplate::class.java,
+        HasParameterizedTest::class.java,
+        HasInnerClassWithTest::class.java,
+        HasTaggedTest::class.java,
+        HasInheritedTestsFromClass::class.java,
+        HasInheritedTestsFromInterface::class.java,
     )
-    @ParameterizedTest
-    fun `runner is created correctly for classes with valid jupiter test methods`(cls: Class<*>) =
-        runTest(cls, expectSuccess = true)
-
-    @ValueSource(
-        classes = [
-            DoesntHaveTestMethods::class,
-            HasJUnit4Tests::class,
-            kotlin.time.Duration::class,
-        ]
+
+    @TestFactory
+    fun `no runner is created if class has no jupiter test methods`() = runTest(
+        expectSuccess = false,
+        DoesntHaveTestMethods::class.java,
+        HasJUnit4Tests::class.java,
+        kotlin.time.Duration::class.java,
     )
-    @ParameterizedTest
-    fun `no runner is created if class has no jupiter test methods`(cls: Class<*>) =
-        runTest(cls, expectSuccess = false)
 
     /* Private */
 
-    private fun runTest(cls: Class<*>, expectSuccess: Boolean) {
-        val runner = builder.runnerForClass(cls)
-        if (expectSuccess) {
-            assertThat(runner).isNotNull()
-        } else {
-            assertThat(runner).isNull()
+    private fun runTest(expectSuccess: Boolean, vararg classes: Class<*>): List<DynamicNode> {
+        // Generate a test container for each given class,
+        // then create two sub-variants for testing both DummyJUnit5 and AndroidJUnit5
+        return classes.map { cls ->
+            dynamicContainer(
+                /* displayName = */ cls.name,
+                /* dynamicNodes = */ setOf(Build.VERSION_CODES.M, Build.VERSION_CODES.TIRAMISU).map { apiLevel ->
+                    dynamicTest("API Level $apiLevel") {
+                        withMockedInstrumentation {
+                            withApiLevel(apiLevel) {
+                                val runner = builder.runnerForClass(cls)
+                                if (expectSuccess) {
+                                    assertThat(runner).isNotNull()
+                                } else {
+                                    assertThat(runner).isNull()
+                                }
+                            }
+                        }
+                    }
+                }
+            )
         }
     }
 }
