Flank should not skip tests annotated with @Ignored

test_runner/src/main/kotlin/ftl/args/AndroidTestShard.kt

@@ -3,8 +3,10 @@ package ftl.args
 import com.linkedin.dex.parser.DexParser
 import com.linkedin.dex.parser.TestMethod
 import ftl.config.FtlConstants
+import ftl.filter.TestFilter
 import ftl.filter.TestFilters
 import ftl.gc.GcStorage
+import ftl.util.FlankTestMethod
 import ftl.util.Utils
 import kotlinx.coroutines.runBlocking
 
@@ -24,7 +26,7 @@ object AndroidTestShard {
         return ArgsHelper.calculateShards(filteredTests, args)
     }
 
-    private fun getTestMethods(args: AndroidArgs, testLocalApk: String): List<String> {
+    private fun getTestMethods(args: AndroidArgs, testLocalApk: String): List<FlankTestMethod> {
         val allTestMethods = DexParser.findTestMethods(testLocalApk)
         val shouldIgnoreMissingTests = allTestMethods.isEmpty() && args.disableSharding
         val shouldThrowErrorIfMissingTests = allTestMethods.isEmpty() && !args.disableSharding
@@ -33,14 +35,18 @@ object AndroidTestShard {
             shouldThrowErrorIfMissingTests -> throw IllegalStateException(Utils.fatalError("Test APK has no tests"))
         }
         val testFilter = TestFilters.fromTestTargets(args.testTargets)
-        val filteredTests = allTestMethods
-            .asSequence()
-            .distinct()
-            .filter(testFilter.shouldRun)
-            .map(TestMethod::testName)
-            .map { "class $it" }
-            .toList()
-        require(FtlConstants.useMock || filteredTests.isNotEmpty()) { Utils.fatalError("All tests filtered out") }
-        return filteredTests
+        return allTestMethods filterWith testFilter
     }
+
+    private infix fun List<TestMethod>.filterWith(filter: TestFilter) = asSequence()
+        .distinct()
+        .filter(filter.shouldRun)
+        .map { FlankTestMethod("class ${it.testName}", it.isIgnored) }
+        .toList()
+        .also {
+            require(FtlConstants.useMock || it.isNotEmpty()) { Utils.fatalError("All tests filtered out") }
+        }
 }
+
+private val TestMethod.isIgnored: Boolean
+    get() = annotations.map { it.name }.contains("org.junit.Ignore")

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

@@ -23,6 +23,7 @@ import ftl.reports.xml.model.JUnitTestResult
 import ftl.shard.Shard
 import ftl.shard.StringShards
 import ftl.shard.stringShards
+import ftl.util.FlankTestMethod
 import ftl.util.Utils
 import java.io.File
 import java.net.URI
@@ -55,7 +56,7 @@ object ArgsHelper {
     fun assertCommonProps(args: IArgs) {
         Utils.assertNotEmpty(
             args.project, "The project is not set. Define GOOGLE_CLOUD_PROJECT, set project in flank.yml\n" +
-                    "or save service account credential to ${FtlConstants.defaultCredentialPath}\n" +
+                    "or save service account credential to ${defaultCredentialPath}\n" +
                     " See https://github.com/GoogleCloudPlatform/google-cloud-java#specifying-a-project-id"
         )
 
@@ -114,7 +115,7 @@ object ArgsHelper {
         testTargets: List<String>,
         validTestMethods: Collection<String>,
         from: String,
-        skipValidation: Boolean = FtlConstants.useMock
+        skipValidation: Boolean = useMock
     ) {
         val missingMethods = testTargets - validTestMethods
 
@@ -123,7 +124,7 @@ object ArgsHelper {
     }
 
     fun createJunitBucket(projectId: String, junitGcsPath: String) {
-        if (FtlConstants.useMock || junitGcsPath.isEmpty()) return
+        if (useMock || junitGcsPath.isEmpty()) return
         val bucket = junitGcsPath.drop(GCS_PREFIX.length).substringBefore('/')
         createGcsBucket(projectId, bucket)
     }
@@ -190,7 +191,7 @@ object ArgsHelper {
     }
 
     fun getDefaultProjectId(): String? {
-        if (FtlConstants.useMock) return "mockProjectId"
+        if (useMock) return "mockProjectId"
 
         // Allow users control over project by checking using Google's logic first before falling back to JSON.
         return ServiceOptions.getDefaultProjectId() ?: serviceAccountProjectId()
@@ -220,9 +221,9 @@ object ArgsHelper {
         return ArgsFileVisitor("glob:$filePath").walk(searchDir)
     }
 
-    fun calculateShards(filteredTests: List<String>, args: IArgs): List<List<String>> {
+    fun calculateShards(filteredTests: List<FlankTestMethod>, args: IArgs): List<List<String>> {
         val shards = if (args.disableSharding) {
-            mutableListOf(filteredTests as MutableList<String>)
+            mutableListOf(filteredTests.map { it.testName } as MutableList<String>)
         } else {
             val oldTestResult = GcStorage.downloadJunitXml(args) ?: JUnitTestResult(mutableListOf())
             val shardCount = Shard.shardCountByTime(filteredTests, oldTestResult, args)

test_runner/src/main/kotlin/ftl/args/IosArgs.kt

@@ -21,6 +21,7 @@ import ftl.config.Device
 import ftl.config.FtlConstants
 import ftl.ios.IosCatalog
 import ftl.ios.Xctestrun
+import ftl.util.FlankTestMethod
 import ftl.util.Utils.fatalError
 import java.nio.file.Files
 import java.nio.file.Path
@@ -69,7 +70,7 @@ class IosArgs(
         if (disableSharding) return@lazy listOf(emptyList<String>())
 
         val validTestMethods = Xctestrun.findTestNames(xctestrunFile)
-        val testsToShard = filterTests(validTestMethods, testTargets).distinct()
+        val testsToShard = filterTests(validTestMethods, testTargets).distinct().map { FlankTestMethod(it, ignored = false) }
 
         ArgsHelper.calculateShards(testsToShard, this)
     }

test_runner/src/main/kotlin/ftl/filter/TestFilters.kt

@@ -43,9 +43,6 @@ object TestFilters {
     private const val ARGUMENT_TEST_FILE = "testFile"
     private const val ARGUMENT_NOT_TEST_FILE = "notTestFile"
 
-    // JUnit @Ignore tests are removed.
-    private const val ANNOTATION_IGNORE = "org.junit.Ignore"
-
     private val FILTER_ARGUMENT by lazy {
 
         val pattern = listOf(
@@ -75,24 +72,20 @@ object TestFilters {
     }
 
     fun fromTestTargets(targets: List<String>): TestFilter {
-        return if (targets.isEmpty()) {
-            notIgnored()
-        } else {
-            val parsedFilters =
-                targets
-                    .asSequence()
-                    .map(String::trim)
-                    .map(TestFilters::parseSingleFilter)
-                    .toList()
-
-            // select test method name filters and short circuit if they match ex: class a.b#c
-            val annotationFilters = parsedFilters.filter { it.isAnnotation }.toTypedArray()
-            val otherFilters = parsedFilters.filterNot { it.isAnnotation }.toTypedArray()
-
-            val result = allOf(notIgnored(), *annotationFilters, anyOf(*otherFilters))
-            if (FtlConstants.useMock) println(result.describe)
-            result
-        }
+        val parsedFilters =
+            targets
+                .asSequence()
+                .map(String::trim)
+                .map(TestFilters::parseSingleFilter)
+                .toList()
+
+        // select test method name filters and short circuit if they match ex: class a.b#c
+        val annotationFilters = parsedFilters.filter { it.isAnnotation }.toTypedArray()
+        val otherFilters = parsedFilters.filterNot { it.isAnnotation }.toTypedArray()
+
+        val result = allOf(*annotationFilters, anyOf(*otherFilters))
+        if (FtlConstants.useMock) println(result.describe)
+        return result
     }
 
     private fun parseSingleFilter(target: String): TestFilter {
@@ -156,14 +149,6 @@ object TestFilters {
         isAnnotation = true
     )
 
-    private fun notIgnored(): TestFilter = TestFilter(
-        describe = "notIgnored",
-        shouldRun = { testMethod ->
-            withAnnotation(listOf(ANNOTATION_IGNORE)).shouldRun(testMethod).not()
-        },
-        isAnnotation = true
-    )
-
     private fun not(filter: TestFilter): TestFilter = TestFilter(
         describe = "not ${filter.describe}",
         shouldRun = { testMethod ->

test_runner/src/main/kotlin/ftl/shard/Shard.kt

@@ -6,6 +6,7 @@ import ftl.args.IArgs
 import ftl.args.IosArgs
 import ftl.reports.xml.model.JUnitTestCase
 import ftl.reports.xml.model.JUnitTestResult
+import ftl.util.FlankTestMethod
 import ftl.util.Utils.fatalError
 import kotlin.math.ceil
 import kotlin.math.min
@@ -47,7 +48,8 @@ class com.foo.ClassName#testMethodToSkip
 object Shard {
     // When a test does not have previous results to reference, fall back to this run time.
     @VisibleForTesting
-    const val DEFAULT_TEST_TIME_SEC: Double = 120.0
+    const val DEFAULT_TEST_TIME_SEC = 120.0
+    private const val IGNORE_TEST_TIME = 0.0
 
     private fun JUnitTestCase.androidKey(): String {
         return "class $classname#$name"
@@ -62,15 +64,15 @@ object Shard {
 
     // take in the XML with timing info then return the shard count based on execution time
     fun shardCountByTime(
-        testsToRun: List<String>,
+        testsToRun: List<FlankTestMethod>,
         oldTestResult: JUnitTestResult,
         args: IArgs
     ): Int {
         if (args.shardTime == -1) return -1
         if (args.shardTime < -1 || args.shardTime == 0) fatalError("Invalid shard time ${args.shardTime}")
 
         val oldDurations = createTestMethodDurationMap(oldTestResult, args)
-        val testsTotalTime = testsToRun.sumByDouble { oldDurations[it] ?: DEFAULT_TEST_TIME_SEC }
+        val testsTotalTime = testsToRun.sumByDouble { if (it.ignored) IGNORE_TEST_TIME else oldDurations[it.testName] ?: DEFAULT_TEST_TIME_SEC }
 
         val shardsByTime = ceil(testsTotalTime / args.shardTime).toInt()
 
@@ -93,7 +95,7 @@ object Shard {
 
     // take in the XML with timing info then return list of shards based on the amount of shards to use
     fun createShardsByShardCount(
-        testsToRun: List<String>,
+        testsToRun: List<FlankTestMethod>,
         oldTestResult: JUnitTestResult,
         args: IArgs,
         forcedShardCount: Int = -1
@@ -104,19 +106,19 @@ object Shard {
         val previousMethodDurations = createTestMethodDurationMap(oldTestResult, args)
 
         var cacheMiss = 0
-        val testcases: List<TestMethod> = testsToRun
-            .map { methodName ->
+        val testCases: List<TestMethod> = testsToRun
+            .map {
                 TestMethod(
-                    name = methodName,
-                    time = previousMethodDurations[methodName] ?: DEFAULT_TEST_TIME_SEC.also {
+                    name = it.testName,
+                    time = if (it.ignored) IGNORE_TEST_TIME else previousMethodDurations[it.testName] ?: DEFAULT_TEST_TIME_SEC.also {
                         cacheMiss += 1
                     }
                 )
             }
             // We want to iterate over testcase going from slowest to fastest
             .sortedByDescending(TestMethod::time)
 
-        val testCount = testcases.size
+        val testCount = testCases.size
 
         // If maxShards is infinite or we have more shards than tests, let's match it
         val shardsCount = if (maxShards == -1 || maxShards > testCount) testCount else maxShards
@@ -135,7 +137,7 @@ object Shard {
         }
         var shards = List(shardsCount) { TestShard(0.0, mutableListOf()) }
 
-        testcases.forEach { testMethod ->
+        testCases.forEach { testMethod ->
             val shard = shards.first()
 
             shard.testMethods.add(testMethod)

test_runner/src/main/kotlin/ftl/util/FlankTestMethod.kt

@@ -0,0 +1,3 @@
+package ftl.util
+
+data class FlankTestMethod(val testName: String, val ignored: Boolean = false)