Skip to content

Commit

Permalink
Restructure test name formatting to fix intermittent regression of is…
Browse files Browse the repository at this point in the history
…olated method runs (#341)

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
  • Loading branch information
mannodermaus committed Jun 15, 2024
1 parent 08c30f0 commit a71c2d0
Show file tree
Hide file tree
Showing 8 changed files with 371 additions and 43 deletions.
2 changes: 1 addition & 1 deletion instrumentation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
@@ -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(
Expand Down Expand Up @@ -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)
Original file line number Diff line number Diff line change
@@ -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("()", "")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<String>()
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<String>()
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) =
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
Loading

0 comments on commit a71c2d0

Please sign in to comment.