fix(core): don't schedule retries according to RetryStrategy if test …

…reached terminal state

core/src/main/kotlin/com/malinskiy/marathon/execution/progress/PoolProgressAccumulator.kt

@@ -219,7 +219,7 @@ class PoolProgressAccumulator(
                 transitionTo(TestState.Failed(total = total, running = running - 1, done = done))
             }
             on<TestEvent.AddRetry> {
-                transitionTo(TestState.Failed(total = total + 1, running = running, done = done))
+                transitionTo(TestState.Failed(total = total, running = running, done = done), TestAction.Complete)
             }
             on<TestEvent.RemoveAttempts> {
                 transitionTo(TestState.Failed(total = total - it.count, running = running, done = done))
@@ -239,7 +239,7 @@ class PoolProgressAccumulator(
                 transitionTo(TestState.Passed(total = total, running = running - 1, done = done))
             }
             on<TestEvent.AddRetry> {
-                transitionTo(TestState.Passed(total = total + 1, running = running, done = done))
+                transitionTo(TestState.Passed(total = total, running = running, done = done), TestAction.Complete)
             }
             on<TestEvent.RemoveAttempts> {
                 transitionTo(TestState.Passed(total = total - it.count, running = running, done = done))

core/src/main/kotlin/com/malinskiy/marathon/execution/queue/QueueActor.kt

@@ -186,10 +186,9 @@ class QueueActor(
         device: DeviceInfo
     ) {
         logger.debug { "handle failed tests ${device.serialNumber}" }
-        val retryList = retryStrategy.process(poolId, failed, testShard)
+        val retryList = retryStrategy.process(poolId, failed, testShard, poolProgressAccumulator)
 
         retryList.forEach {
-            poolProgressAccumulator.retryTest(it.test)
             val testAction = poolProgressAccumulator.testEnded(device, it)
             processTestAction(testAction, it)
             rerunTest(it.test)

core/src/main/kotlin/com/malinskiy/marathon/execution/strategy/RetryStrategy.kt

@@ -3,7 +3,13 @@ package com.malinskiy.marathon.execution.strategy
 import com.malinskiy.marathon.device.DevicePoolId
 import com.malinskiy.marathon.execution.TestResult
 import com.malinskiy.marathon.execution.TestShard
+import com.malinskiy.marathon.execution.progress.PoolProgressAccumulator
 
 interface RetryStrategy {
-    fun process(devicePoolId: DevicePoolId, tests: Collection<TestResult>, testShard: TestShard): List<TestResult>
+    fun process(
+        devicePoolId: DevicePoolId,
+        tests: Collection<TestResult>,
+        testShard: TestShard,
+        poolProgressAccumulator: PoolProgressAccumulator
+    ): List<TestResult>
 }

core/src/main/kotlin/com/malinskiy/marathon/execution/strategy/impl/retry/NoRetryStrategy.kt

@@ -3,10 +3,16 @@ package com.malinskiy.marathon.execution.strategy.impl.retry
 import com.malinskiy.marathon.device.DevicePoolId
 import com.malinskiy.marathon.execution.TestResult
 import com.malinskiy.marathon.execution.TestShard
+import com.malinskiy.marathon.execution.progress.PoolProgressAccumulator
 import com.malinskiy.marathon.execution.strategy.RetryStrategy
 
 class NoRetryStrategy : RetryStrategy {
-    override fun process(devicePoolId: DevicePoolId, tests: Collection<TestResult>, testShard: TestShard): List<TestResult> {
+    override fun process(
+        devicePoolId: DevicePoolId,
+        tests: Collection<TestResult>,
+        testShard: TestShard,
+        poolProgressAccumulator: PoolProgressAccumulator
+    ): List<TestResult> {
         return emptyList()
     }
 

core/src/main/kotlin/com/malinskiy/marathon/execution/strategy/impl/retry/fixedquota/FixedQuotaRetryStrategy.kt

@@ -4,17 +4,29 @@ import com.malinskiy.marathon.config.strategy.RetryStrategyConfiguration
 import com.malinskiy.marathon.device.DevicePoolId
 import com.malinskiy.marathon.execution.TestResult
 import com.malinskiy.marathon.execution.TestShard
+import com.malinskiy.marathon.execution.progress.PoolProgressAccumulator
+import com.malinskiy.marathon.execution.queue.TestAction
 import com.malinskiy.marathon.execution.strategy.RetryStrategy
 
 class FixedQuotaRetryStrategy(private val cnf: RetryStrategyConfiguration.FixedQuotaRetryStrategyConfiguration) : RetryStrategy {
     private val retryWatchdog = RetryWatchdog(cnf.totalAllowedRetryQuota, cnf.retryPerTestQuota)
     private val poolTestCaseFailureAccumulator = PoolTestFailureAccumulator()
 
-    override fun process(devicePoolId: DevicePoolId, tests: Collection<TestResult>, testShard: TestShard): List<TestResult> {
+    override fun process(
+        devicePoolId: DevicePoolId,
+        tests: Collection<TestResult>,
+        testShard: TestShard,
+        poolProgressAccumulator: PoolProgressAccumulator
+    ): List<TestResult> {
         return tests.filter { testResult ->
             poolTestCaseFailureAccumulator.record(devicePoolId, testResult.test)
             val flakinessResultCount = testShard.flakyTests.count { it == testResult.test }
-            retryWatchdog.requestRetry(poolTestCaseFailureAccumulator.getCount(devicePoolId, testResult.test) + flakinessResultCount)
+            val failuresCount = poolTestCaseFailureAccumulator.getCount(devicePoolId, testResult.test) + flakinessResultCount
+            if (retryWatchdog.retryPossible(failuresCount) && poolProgressAccumulator.retryTest(testResult.test) != TestAction.Complete) {
+                retryWatchdog.requestRetry(failuresCount)
+            } else {
+                false
+            }
         }
     }
 

core/src/main/kotlin/com/malinskiy/marathon/execution/strategy/impl/retry/fixedquota/RetryWatchdog.kt

@@ -15,6 +15,12 @@ internal class RetryWatchdog(
         return totalAllowedRetryAvailable && singleTestAllowed
     }
 
+    fun retryPossible(failuresCount: Int): Boolean {
+        val totalAllowedRetryAvailable = totalAllowedRetryLeft.get() >= 0
+        val singleTestAllowed = failuresCount <= maxRetryPerTestQuota
+        return totalAllowedRetryAvailable && singleTestAllowed
+    }
+
     private fun totalAllowedRetryAvailable(): Boolean {
         return totalAllowedRetryLeft.decrementAndGet() >= 0
     }

core/src/test/kotlin/com/malinskiy/marathon/execution/progress/PoolProgressAccumulatorTest.kt

@@ -12,6 +12,7 @@ import com.malinskiy.marathon.execution.TestShard
 import com.malinskiy.marathon.execution.TestStatus
 import com.malinskiy.marathon.generateTest
 import com.malinskiy.marathon.report.getDevice
+import org.amshove.kluent.shouldBe
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.reset
 import org.amshove.kluent.shouldBeEqualTo
@@ -727,17 +728,16 @@ class PoolProgressAccumulatorTest {
          * test 2 failed
          */
         reporter.testStarted(device, test2)
-        reporter.testEnded(device, TestResult(test2, device, "2", TestStatus.FAILURE, 1, 2))
-        reporter.progress().shouldBeEqualTo(2 / 3f)
-
         /**
          * adding 4 retries for test2 and then test 2 passes once
          */
-        reporter.retryTest(test2)
-        reporter.retryTest(test2)
-        reporter.retryTest(test2)
-        reporter.retryTest(test2)
+        reporter.retryTest(test2) shouldBe null
+        reporter.retryTest(test2) shouldBe null
+        reporter.retryTest(test2) shouldBe null
+        reporter.retryTest(test2) shouldBe null
+        reporter.testEnded(device, TestResult(test2, device, "2", TestStatus.FAILURE, 1, 2))
         reporter.progress().shouldBeEqualTo(2 / 7f)
+
         reporter.testStarted(device, test2)
         reporter.testEnded(device, TestResult(test2, device, "2", TestStatus.PASSED, 2, 3))
         reporter.progress().shouldBeEqualTo(3 / 7f)

core/src/test/kotlin/com/malinskiy/marathon/execution/strategy/impl/retry/NoRetryStrategyTest.kt

@@ -1,21 +1,48 @@
 package com.malinskiy.marathon.execution.strategy.impl.retry
 
+import com.malinskiy.marathon.analytics.internal.pub.Track
+import com.malinskiy.marathon.config.Configuration
+import com.malinskiy.marathon.config.strategy.ExecutionMode
+import com.malinskiy.marathon.config.strategy.ExecutionStrategyConfiguration
+import com.malinskiy.marathon.config.vendor.VendorConfiguration
 import com.malinskiy.marathon.device.DevicePoolId
 import com.malinskiy.marathon.execution.TestShard
+import com.malinskiy.marathon.execution.progress.PoolProgressAccumulator
 import com.malinskiy.marathon.generateTestResults
 import com.malinskiy.marathon.generateTests
 import org.amshove.kluent.shouldBeEmpty
 import org.junit.jupiter.api.Test
+import org.mockito.kotlin.mock
+import java.io.File
 
 class NoRetryStrategyTest {
+    private val anySuccessConfig = Configuration.Builder(
+        name = "",
+        outputDir = File("")
+    ).apply {
+        vendorConfiguration = VendorConfiguration.StubVendorConfiguration
+        debug = false
+        analyticsTracking = false
+        executionStrategy = ExecutionStrategyConfiguration(ExecutionMode.ANY_SUCCESS, fast = false)
+    }.build()
+
+    private val track = mock<Track>()
+
     @Test
     fun `should return empty list`() {
         val tests = generateTests(50)
         val testResults = generateTestResults(tests)
         val strategy = NoRetryStrategy()
         val devicePoolId = DevicePoolId("devicePoolId")
         val testShard = TestShard(tests)
-        val result = strategy.process(devicePoolId, testResults, testShard)
+        val accumulator = PoolProgressAccumulator(
+            devicePoolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
+
+        val result = strategy.process(devicePoolId, testResults, testShard, accumulator)
         result.shouldBeEmpty()
     }
 }

core/src/test/kotlin/com/malinskiy/marathon/execution/strategy/impl/retry/fixedquota/FixedQuotaRetryStrategyTest.kt

@@ -1,23 +1,51 @@
 package com.malinskiy.marathon.execution.strategy.impl.retry.fixedquota
 
+import com.malinskiy.marathon.analytics.internal.pub.Track
+import com.malinskiy.marathon.config.Configuration
+import com.malinskiy.marathon.config.strategy.ExecutionMode
+import com.malinskiy.marathon.config.strategy.ExecutionStrategyConfiguration
 import com.malinskiy.marathon.config.strategy.RetryStrategyConfiguration
+import com.malinskiy.marathon.config.vendor.VendorConfiguration
 import com.malinskiy.marathon.device.DevicePoolId
+import com.malinskiy.marathon.execution.TestResult
 import com.malinskiy.marathon.execution.TestShard
+import com.malinskiy.marathon.execution.progress.PoolProgressAccumulator
 import com.malinskiy.marathon.extension.toRetryStrategy
 import com.malinskiy.marathon.generateTestResults
 import com.malinskiy.marathon.generateTests
+import com.malinskiy.marathon.report.getDevice
 import org.amshove.kluent.shouldBe
 import org.junit.jupiter.api.Test
+import org.mockito.kotlin.mock
+import java.io.File
 
 class FixedQuotaRetryStrategyTest {
+    private val anySuccessConfig = Configuration.Builder(
+        name = "",
+        outputDir = File("")
+    ).apply {
+        vendorConfiguration = VendorConfiguration.StubVendorConfiguration
+        debug = false
+        analyticsTracking = false
+        executionStrategy = ExecutionStrategyConfiguration(ExecutionMode.ANY_SUCCESS, fast = false)
+    }.build()
+
+    private val track = mock<Track>()
 
     @Test
     fun `total quota tests, total quota is 1`() {
         val strategy = RetryStrategyConfiguration.FixedQuotaRetryStrategyConfiguration(totalAllowedRetryQuota = 1).toRetryStrategy()
         val poolId = DevicePoolId("DevicePoolId-1")
         val tests = generateTests(10)
         val testResults = generateTestResults(tests)
-        strategy.process(poolId, testResults, TestShard(tests)).size shouldBe 1
+        val accumulator = PoolProgressAccumulator(
+            poolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
+
+        strategy.process(poolId, testResults, TestShard(tests), accumulator).size shouldBe 1
     }
 
     @Test
@@ -26,7 +54,13 @@ class FixedQuotaRetryStrategyTest {
         val poolId = DevicePoolId("DevicePoolId-1")
         val tests = generateTests(10)
         val testResults = generateTestResults(tests)
-        strategy.process(poolId, testResults, TestShard(tests)).size shouldBe 10
+        val accumulator = PoolProgressAccumulator(
+            poolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
+        strategy.process(poolId, testResults, TestShard(tests), accumulator).size shouldBe 10
     }
 
     @Test
@@ -35,8 +69,14 @@ class FixedQuotaRetryStrategyTest {
         val poolId = DevicePoolId("DevicePoolId-1")
         val tests = generateTests(50)
         val testResults = generateTestResults(tests)
+        val accumulator = PoolProgressAccumulator(
+            poolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
 
-        strategy.process(poolId, testResults, TestShard(tests)).size shouldBe 50
+        strategy.process(poolId, testResults, TestShard(tests), accumulator).size shouldBe 50
     }
 
     @Test
@@ -45,11 +85,44 @@ class FixedQuotaRetryStrategyTest {
         val poolId = DevicePoolId("DevicePoolId-1")
         val tests = generateTests(50)
         val testResults = generateTestResults(tests)
+        val accumulator = PoolProgressAccumulator(
+            poolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
+
+        strategy.process(
+            poolId,
+            testResults,
+            TestShard(tests, flakyTests = tests + tests + tests),
+            accumulator
+        ).size shouldBe 0
+    }
+
+    @Test
+    fun `should return 0 tests if test reached terminal state`() {
+        val strategy = RetryStrategyConfiguration.FixedQuotaRetryStrategyConfiguration().toRetryStrategy()
+        val poolId = DevicePoolId("DevicePoolId-1")
+        val tests = generateTests(1)
+        val testResults = generateTestResults(tests)
+        val accumulator = PoolProgressAccumulator(
+            poolId,
+            TestShard(tests),
+            anySuccessConfig,
+            track
+        )
+        val deviceInfo = getDevice()
+
+        val test = tests.first()
+        accumulator.testStarted(deviceInfo, test)
+        accumulator.testEnded(deviceInfo, testResults.first())
 
         strategy.process(
             poolId,
             testResults,
-            TestShard(tests, flakyTests = tests + tests + tests)
+            TestShard(tests),
+            accumulator
         ).size shouldBe 0
     }
 }