From 75de00de92ebc2ad6b392bbf81a39286ec26fa31 Mon Sep 17 00:00:00 2001 From: Marcel Schnelle Date: Sat, 15 Jun 2024 10:13:17 +0900 Subject: [PATCH] Restructure test name formatting to fix intermittent regression of isolated method runs Since relying fully on JUnit 4's test discovery mechanism, we need to adhere to its expectations when it comes to method filtering. For dynamic tests with JUnit 5, this means that we have to remove the parameters and brackets from the display name when invoking an individual test through CLI or IDE, otherwise it cannot be found --- instrumentation/CHANGELOG.md | 2 +- .../internal/extensions/TestIdentifierExt.kt | 10 ++ .../internal/formatters/TestNameFormatter.kt | 42 +++++ .../runners/AndroidJUnitPlatformTestTree.kt | 72 ++++----- .../de/mannodermaus/junit5/TestClasses.kt | 2 + .../de/mannodermaus/junit5/TestHelpers.kt | 30 ++++ .../formatters/TestNameFormatterTests.kt | 108 +++++++++++++ .../AndroidJUnitPlatformTestTreeTests.kt | 148 ++++++++++++++++++ 8 files changed, 371 insertions(+), 43 deletions(-) create mode 100644 instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatter.kt create mode 100644 instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestHelpers.kt create mode 100644 instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatterTests.kt create mode 100644 instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTreeTests.kt diff --git a/instrumentation/CHANGELOG.md b/instrumentation/CHANGELOG.md index e74604e5..4ba7c679 100644 --- a/instrumentation/CHANGELOG.md +++ b/instrumentation/CHANGELOG.md @@ -5,7 +5,7 @@ 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) +- Fix invalid naming of dynamic tests when executing only a singular test method from the IDE (#317, #339) - 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) diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/extensions/TestIdentifierExt.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/extensions/TestIdentifierExt.kt index f7169987..ab2cf710 100644 --- a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/extensions/TestIdentifierExt.kt +++ b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/extensions/TestIdentifierExt.kt @@ -1,5 +1,6 @@ package de.mannodermaus.junit5.internal.extensions +import de.mannodermaus.junit5.internal.formatters.TestNameFormatter import org.junit.platform.launcher.TestIdentifier private val DYNAMIC_TEST_PREFIXES = listOf( @@ -29,3 +30,12 @@ internal val TestIdentifier.isDynamicTest: Boolean val shortId = this.shortId return DYNAMIC_TEST_PREFIXES.any { shortId.startsWith(it) } } + +/** + * Returns a formatted version of this identifier's name, + * which is compatible with the quirks and limitations + * of the Android Instrumentation, esp. when the [isIsolatedMethodRun] + * flag is enabled. + */ +internal fun TestIdentifier.format(isIsolatedMethodRun: Boolean = false): String = + TestNameFormatter.format(this, isIsolatedMethodRun) diff --git a/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatter.kt b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatter.kt new file mode 100644 index 00000000..62220a0d --- /dev/null +++ b/instrumentation/runner/src/main/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatter.kt @@ -0,0 +1,42 @@ +package de.mannodermaus.junit5.internal.formatters + +import org.junit.platform.launcher.TestIdentifier + +/** + * A class for naming Jupiter test methods in a compatible manner, + * taking into account several limitations imposed by the + * Android instrumentation (e.g. on isolated test runs). + */ +internal object TestNameFormatter { + fun format(identifier: TestIdentifier, isIsolatedMethodRun: Boolean = false): String { + // During isolated executions of a single test method, + // construct a technical version of its name for backwards compatibility + // with the JUnit 4-based instrumentation of Android by stripping the brackets of parameterized tests completely. + // If this didn't happen, running them from the IDE will cause "No tests found" errors. + // See AndroidX's TestRequestBuilder$MethodFilter for where this is cross-referenced in the instrumentation! + // + // History: + // - #199 & #207 (the original unearthing of this behavior) + // - #317 (making an exception for dynamic tests) + // - #339 (retain indices of parameterized methods to avoid premature filtering by JUnit 4's test discovery) + if (isIsolatedMethodRun) { + val reportName = identifier.legacyReportingName + val paramStartIndex = reportName.indexOf('(') + if (paramStartIndex > -1) { + val result = reportName.substring(0, paramStartIndex) + + val paramEndIndex = reportName.lastIndexOf('[') + + return if (paramEndIndex > -1) { + // Retain suffix of parameterized methods (i.e. "[1]", "[2]" etc) + // so that they won't be filtered out by JUnit 4 on isolated method runs + result + reportName.substring(paramEndIndex) + } else { + result + } + } + } + + return identifier.displayName.replace("()", "") + } +} 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 index 0719e209..b489baa2 100644 --- 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 @@ -3,6 +3,7 @@ package de.mannodermaus.junit5.internal.runners import android.annotation.SuppressLint +import de.mannodermaus.junit5.internal.extensions.format import de.mannodermaus.junit5.internal.extensions.isDynamicTest import org.junit.platform.commons.util.AnnotationUtils import org.junit.platform.engine.UniqueId @@ -35,53 +36,40 @@ internal class AndroidJUnitPlatformTestTree( val suiteDescription = generateSuiteDescription(testPlan, testClass) - fun getTestName(identifier: TestIdentifier): String = - when { - identifier.isContainer -> getTechnicalName(identifier) - - identifier.isDynamicTest -> { - // Collect all dynamic tests' IDs from this identifier, - // all the way up to the first non-dynamic test. - // Collect the name of all these into a list, then finally - // compose the final name from this list. Note that, because we - // move upwards the test plan, the elements must be reversed - // before the final name can be composed. - val nameComponents = mutableListOf() - var currentNode: TestIdentifier? = identifier - while (currentNode != null && currentNode.isDynamicTest) { - nameComponents.add(formatTestName(currentNode)) - currentNode = modifiedTestPlan.getRealParent(currentNode).orElse(null) - } - nameComponents.reverse() - - // Android's Unified Test Platform (AGP 7.0+) is using literal test names - // to create files when capturing Logcat output during execution. - // Ergo, make sure that only legal characters are being used in the test names - // (ref. https://github.com/mannodermaus/android-junit5/issues/263) - nameComponents.joinToString(" - ") + // Order matters here, since all dynamic tests are also containers, + // but not all containers are dynamic tests + fun getTestName(identifier: TestIdentifier): String = when { + identifier.isDynamicTest -> if (isIsolatedMethodRun) { + // In isolated method runs, there is no need to compose + // dynamic test names from multiple pieces, as the + // Android Instrumentation only looks at the raw method name + // anyway and all information about parameter types is lost + identifier.format(true) + } else { + // Collect all dynamic tests' IDs from this identifier, + // all the way up to the first non-dynamic test. + // Collect the name of all these into a list, then finally + // compose the final name from this list. Note that, because we + // move upwards the test plan, the elements must be reversed + // before the final name can be composed. + val nameComponents = mutableListOf() + var currentNode: TestIdentifier? = identifier + while (currentNode != null && currentNode.isDynamicTest) { + nameComponents.add(currentNode.format(false)) + currentNode = modifiedTestPlan.getRealParent(currentNode).orElse(null) } + nameComponents.reverse() - else -> formatTestName(identifier) + // Android's Unified Test Platform (AGP 7.0+) is using literal test names + // to create files when capturing Logcat output during execution. + // Ergo, make sure that only legal characters are being used in the test names + // (ref. https://github.com/mannodermaus/android-junit5/issues/263) + nameComponents.joinToString(" - ") } - private fun formatTestName(identifier: TestIdentifier): String { - // During isolated executions of non-dynamic @Test methods, - // construct a technical version of the test name for backwards compatibility - // with the JUnit 4-based instrumentation of Android by stripping the brackets and parameters completely. - // If this didn't happen, running them from the IDE will cause "No tests found" errors. - // See AndroidX's TestRequestBuilder$MethodFilter for where this is cross-referenced in the instrumentation! - // - // Ref issues #199 & #207 (the original unearthing of this behavior), - // as well as #317 (making an exception for dynamic tests). - if (isIsolatedMethodRun && !identifier.isDynamicTest) { - val reportName = identifier.legacyReportingName - val bracketIndex = reportName.indexOf('(') - if (bracketIndex > -1) { - return reportName.substring(0, bracketIndex) - } - } + identifier.isContainer -> getTechnicalName(identifier) - return identifier.displayName.replace("()", "") + else -> identifier.format(isIsolatedMethodRun) } // Do not expose our custom TestPlan, because JUnit Platform wouldn't like that very much. diff --git a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestClasses.kt b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestClasses.kt index f8155ab0..3e75ab2d 100644 --- a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestClasses.kt +++ b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestClasses.kt @@ -46,6 +46,8 @@ class HasTestTemplate { private fun context(param: String): TestTemplateInvocationContext = object : TestTemplateInvocationContext { + override fun getDisplayName(invocationIndex: Int): String = param + override fun getAdditionalExtensions() = listOf( object : ParameterResolver { override fun supportsParameter(parameterContext: ParameterContext, extensionContext: ExtensionContext) = diff --git a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestHelpers.kt b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestHelpers.kt new file mode 100644 index 00000000..26be67cb --- /dev/null +++ b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/TestHelpers.kt @@ -0,0 +1,30 @@ +package de.mannodermaus.junit5 + +import org.junit.platform.engine.discovery.DiscoverySelectors +import org.junit.platform.launcher.Launcher +import org.junit.platform.launcher.TestPlan +import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder +import org.junit.platform.launcher.core.LauncherFactory +import kotlin.reflect.KClass + +/** + * A quick one-liner for executing a Jupiter discover-and-execute pass + * from inside of a Jupiter test. Useful for testing runner code + * that needs to work with the innards of the [TestPlan], such as + * individual test identifiers and such. + */ +fun discoverTests( + cls: KClass<*>, + launcher: Launcher = LauncherFactory.create(), + executeAsWell: Boolean = true, +): TestPlan { + return launcher.discover( + LauncherDiscoveryRequestBuilder.request() + .selectors(DiscoverySelectors.selectClass(cls.java)) + .build() + ).also { plan -> + if (executeAsWell) { + launcher.execute(plan) + } + } +} diff --git a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatterTests.kt b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatterTests.kt new file mode 100644 index 00000000..7ec92ab1 --- /dev/null +++ b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/formatters/TestNameFormatterTests.kt @@ -0,0 +1,108 @@ +package de.mannodermaus.junit5.internal.formatters + +import com.google.common.truth.Truth.assertThat +import de.mannodermaus.junit5.HasParameterizedTest +import de.mannodermaus.junit5.HasRepeatedTest +import de.mannodermaus.junit5.HasTest +import de.mannodermaus.junit5.HasTestFactory +import de.mannodermaus.junit5.HasTestTemplate +import de.mannodermaus.junit5.discoverTests +import de.mannodermaus.junit5.internal.extensions.format +import org.junit.jupiter.api.Test +import org.junit.platform.engine.discovery.DiscoverySelectors +import org.junit.platform.launcher.TestIdentifier +import org.junit.platform.launcher.TestPlan +import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder +import org.junit.platform.launcher.core.LauncherFactory +import kotlin.reflect.KClass + +class TestNameFormatterTests { + + @Test + fun `normal junit5 test`() = runTestWith(HasTest::class) { identifier -> + assertThat(identifier.format(false)).isEqualTo("method") + assertThat(identifier.format(true)).isEqualTo("method") + } + + @Test + fun `repeated test`() = runTestWith(HasRepeatedTest::class) { identifier -> + assertThat(identifier.format(false)).isEqualTo("method(RepetitionInfo)") + assertThat(identifier.format(true)).isEqualTo("method") + + // Inspect individual executions, too + assertChildren(identifier, expectedCount = 5) { index, child -> + val number = index + 1 + assertThat(child.format(false)).isEqualTo("repetition $number of 5") + assertThat(child.format(true)).isEqualTo("method[$number]") + } + } + + @Test + fun `test factory`() = runTestWith(HasTestFactory::class) { identifier -> + assertThat(identifier.format(false)).isEqualTo("method") + assertThat(identifier.format(true)).isEqualTo("method") + + // Inspect individual executions, too + assertChildren(identifier, expectedCount = 2) { index, child -> + val number = index + 1 + assertThat(child.format(false)).isEqualTo(if (index == 0) "a" else "b") + assertThat(child.format(true)).isEqualTo("method[$number]") + } + } + + @Test + fun `test template`() = runTestWith(HasTestTemplate::class) { identifier -> + assertThat(identifier.format(false)).isEqualTo("method(String)") + assertThat(identifier.format(true)).isEqualTo("method") + + // Inspect individual executions, too + assertChildren(identifier, expectedCount = 2) { index, child -> + val number = index + 1 + assertThat(child.format(false)).isEqualTo("param$number") + assertThat(child.format(true)).isEqualTo("method[$number]") + } + } + + @Test + fun `parameterized test`() = runTestWith(HasParameterizedTest::class) { identifier -> + assertThat(identifier.format(false)).isEqualTo("method(String)") + assertThat(identifier.format(true)).isEqualTo("method") + + // Inspect individual executions, too + assertChildren(identifier, expectedCount = 2) { index, child -> + val number = index + 1 + assertThat(child.format(false)).isEqualTo("[$number] " + if (index == 0) "a" else "b") + assertThat(child.format(true)).isEqualTo("method[$number]") + } + } + + /* Private */ + + private fun runTestWith(cls: KClass<*>, block: TestPlan.(TestIdentifier) -> Unit) { + // Discover and execute the test plan of the given class + // (execution is important to resolve any dynamic tests + // that aren't generated until the test plan actually runs) + val plan = discoverTests(cls, executeAsWell = true) + + // Validate common behavior of formatter against class names + val root = plan.roots.first() + val classIdentifier = plan.getChildren(root).first() + assertThat(classIdentifier.format(false)).isEqualTo(cls.simpleName) + assertThat(classIdentifier.format(true)).isEqualTo(cls.simpleName) + + // Delegate to the provided block for the test method of the class + val methodIdentifier = plan.getChildren(classIdentifier).first() + plan.block(methodIdentifier) + } + + private fun TestPlan.assertChildren( + of: TestIdentifier, + expectedCount: Int, + block: (Int, TestIdentifier) -> Unit + ) { + with(getChildren(of)) { + assertThat(size).isEqualTo(expectedCount) + forEachIndexed(block) + } + } +} diff --git a/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTreeTests.kt b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTreeTests.kt new file mode 100644 index 00000000..a999ac03 --- /dev/null +++ b/instrumentation/runner/src/test/kotlin/de/mannodermaus/junit5/internal/runners/AndroidJUnitPlatformTestTreeTests.kt @@ -0,0 +1,148 @@ +package de.mannodermaus.junit5.internal.runners + +import com.google.common.truth.Truth.assertThat +import de.mannodermaus.junit5.HasParameterizedTest +import de.mannodermaus.junit5.HasRepeatedTest +import de.mannodermaus.junit5.HasTest +import de.mannodermaus.junit5.HasTestFactory +import de.mannodermaus.junit5.HasTestTemplate +import de.mannodermaus.junit5.discoverTests +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource +import org.junit.platform.launcher.TestExecutionListener +import org.junit.platform.launcher.TestIdentifier +import org.junit.platform.launcher.core.LauncherFactory +import kotlin.reflect.KClass + +class AndroidJUnitPlatformTestTreeTests { + @CsvSource( + "false, method", + "true, method", + ) + @ParameterizedTest + fun test(isolated: Boolean, expected: String) = + runTestWith(HasTest::class, isolated) { identifier -> + val description = getDescription(identifier) + assertThat(description.methodName).isEqualTo(expected) + } + + @CsvSource( + "false, method(RepetitionInfo) - repetition %d of 5", + "true, method[%d]", + ) + @ParameterizedTest + fun `repeated test`(isolated: Boolean, expected: String) = + runTestWith(HasRepeatedTest::class, isolated) { identifier -> + assertChildren(identifier, expectedCount = 5) { index, child -> + val num = index + 1 + val childDescription = getDescription(child) + assertThat(childDescription.methodName).isEqualTo(expected.format(num)) + } + } + + @CsvSource( + "false, method - %s", + "true, method[%d]", + ) + @ParameterizedTest + fun `test factory`(isolated: Boolean, expected: String) = + runTestWith(HasTestFactory::class, isolated) { identifier -> + val childMethodNames = listOf("a", "b") + + assertChildren(identifier, expectedCount = 2) { index, child -> + val num = index + 1 + val childDescription = getDescription(child) + + if (isolated) { + assertThat(childDescription.methodName).isEqualTo(expected.format(num)) + } else { + assertThat(childDescription.methodName).isEqualTo(expected.format(childMethodNames[index])) + } + } + } + + @CsvSource( + "false, method(String) - %s", + "true, method[%d]", + ) + @ParameterizedTest + fun `test template`(isolated: Boolean, expected: String) = + runTestWith(HasTestTemplate::class, isolated) { identifier -> + val childMethodNames = listOf("param1", "param2") + + assertChildren(identifier, expectedCount = 2) { index, child -> + val num = index + 1 + val childDescription = getDescription(child) + + if (isolated) { + assertThat(childDescription.methodName).isEqualTo(expected.format(num)) + } else { + assertThat(childDescription.methodName).isEqualTo(expected.format(childMethodNames[index])) + } + } + } + + @CsvSource( + "false, method(String) - [%d] %s", + "true, method[%d]", + ) + @ParameterizedTest + fun `parameterized test`(isolated: Boolean, expected: String) = + runTestWith(HasParameterizedTest::class, isolated) { identifier -> + val childMethodNames = listOf("a", "b") + + assertChildren(identifier, expectedCount = 2) { index, child -> + val num = index + 1 + val childDescription = getDescription(child) + + if (isolated) { + assertThat(childDescription.methodName).isEqualTo(expected.format(num)) + } else { + assertThat(childDescription.methodName).isEqualTo(expected.format(num, childMethodNames[index])) + } + } + } + + /* Private */ + + private fun runTestWith( + cls: KClass<*>, + isIsolatedMethodRun: Boolean = false, + block: AndroidJUnitPlatformTestTree.(TestIdentifier) -> Unit, + ) { + // Prepare a test plan to launch + val launcher = LauncherFactory.create() + val plan = discoverTests(cls, launcher, executeAsWell = false) + val tree = AndroidJUnitPlatformTestTree( + testPlan = plan, + testClass = cls.java, + isIsolatedMethodRun = isIsolatedMethodRun, + isParallelExecutionEnabled = false, + ) + + // Execute the test plan, adding dynamic tests with the tree + // as they are registered during execution + launcher.execute(plan, object : TestExecutionListener { + override fun dynamicTestRegistered(testIdentifier: TestIdentifier) { + tree.addDynamicDescription(testIdentifier, testIdentifier.parentId.get()) + } + }) + + // For concrete assertions, delegate to the given block + val root = plan.roots.first() + val classIdentifier = plan.getChildren(root).first() + val methodIdentifier = plan.getChildren(classIdentifier).first() + tree.block(methodIdentifier) + } + + private fun AndroidJUnitPlatformTestTree.assertChildren( + identifier: TestIdentifier, + expectedCount: Int, + block: (Int, TestIdentifier) -> Unit + ) { + with(getChildren(identifier)) { + assertThat(size).isEqualTo(expectedCount) + forEachIndexed(block) + } + } +}