Bug in retries logic was fixed (#141)

* * Bug in retries logic was fixed. The client didn't report if a test failed after retries. We should clean failed Test hashmap after the retries count exceed
* New tests was added

* * Logic was fixed with tests according to internal code review.

Author: v1-wizard

src/main/java/com/github/invictum/reportportal/SuiteStorage.java

@@ -2,16 +2,18 @@
 
 import io.reactivex.Maybe;
 
-import java.util.HashSet;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
 
 /**
  * Abstraction used to control active suites lifecycle
  */
 public class SuiteStorage {
 
-    private ConcurrentHashMap<String, SuiteMetadata> suites = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<String, SuiteMetadata> suites = new ConcurrentHashMap<>();
 
     /**
      * Starts a new suite entity
@@ -64,9 +66,9 @@ public void finalizeActive() {
      * @param suiteId id of suite where fail test was detected
      * @param testId  id of failed test
      */
-    public void addFail(String suiteId, String testId) {
+    public void addNewFail(String suiteId, String testId) {
         SuiteMetadata meta = suites.get(suiteId);
-        meta.failedTests.add(testId);
+        meta.failedTests.put(testId, new AtomicInteger(0));
     }
 
     /**
@@ -77,7 +79,7 @@ public void addFail(String suiteId, String testId) {
      */
     public boolean isFailPresent(String suiteId, String testId) {
         SuiteMetadata meta = suites.get(suiteId);
-        return meta.failedTests.contains(testId);
+        return meta.failedTests.containsKey(testId);
     }
 
     /**
@@ -91,12 +93,25 @@ public void removeFail(String suiteId, String testId) {
         meta.failedTests.remove(testId);
     }
 
+    /**
+     * Increase count of failed test to track when exceed count of retires.
+     *
+     * @param suiteId id of suite where fail test was detected
+     * @param testId  id of failed test
+     * @return failCount count of retires after ++
+     */
+    public int incrementAndGetRetriesCount(String suiteId, String testId) {
+        SuiteMetadata meta = suites.get(suiteId);
+        AtomicInteger failCount = meta.failedTests.get(testId);
+        return failCount.incrementAndGet();
+    }
+
     /**
      * Node class that holds suite metadata
      */
     private static class SuiteMetadata {
         private Maybe<String> id;
         private Runnable finisher;
-        private final HashSet<String> failedTests = new HashSet<>();
+        private final Map<String, AtomicInteger> failedTests = new HashMap<>();
     }
 }

src/main/java/com/github/invictum/reportportal/recorder/Regular.java

@@ -83,11 +83,12 @@ private void processRetries(TestOutcome out, StartEventBuilder builder) {
         String suiteId = out.getUserStory().getId();
         if (suiteStorage.isFailPresent(suiteId, testId)) {
             builder.withRetry();
-            if (!isTestFailed(out)) {
+            int retriesCount = suiteStorage.incrementAndGetRetriesCount(suiteId, testId);
+            if (!isTestFailed(out) || retriesCount == RETRIES_COUNT) {
                 suiteStorage.removeFail(suiteId, testId);
             }
         } else if (isTestFailed(out)) {
-            suiteStorage.addFail(suiteId, testId);
+            suiteStorage.addNewFail(suiteId, testId);
         }
     }
 }

src/test/java/com/github/invictum/reportportal/SuiteStorageTest.java

@@ -0,0 +1,33 @@
+package com.github.invictum.reportportal;
+
+import io.reactivex.Maybe;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class SuiteStorageTest {
+
+    SuiteStorage storage;
+
+    @Before
+    public void setupStorage() {
+        storage = new SuiteStorage();
+        storage.start("suite", Maybe::empty);
+    }
+
+
+    @Test
+    public void testAddNewFail() {
+        storage.addNewFail("suite", "storage");
+        Assert.assertTrue(storage.isFailPresent("suite", "storage"));
+    }
+
+    @Test
+    public void testIncrementRetiresCount() {
+        storage.addNewFail("suite", "storage");
+        Assert.assertEquals(1, storage.incrementAndGetRetriesCount("suite", "storage"));
+        Assert.assertEquals(2, storage.incrementAndGetRetriesCount("suite", "storage"));
+        Assert.assertEquals(3, storage.incrementAndGetRetriesCount("suite", "storage"));
+    }
+
+}

src/test/java/com/github/invictum/reportportal/recorder/RegularTest.java

@@ -43,11 +43,9 @@ public void regularRecordTest() {
         Mockito.when(testOutcome.getName()).thenReturn("Test name");
         recorder.record(testOutcome);
         Mockito.verify(suiteStorageMock,
-                Mockito.times(1)).start(
-                Mockito.eq("story"), Mockito.any());
+                Mockito.times(1)).start(Mockito.eq("story"), Mockito.any());
         Mockito.verify(suiteStorageMock,
-                Mockito.times(1)).suiteFinisher(
-                Mockito.eq("story"), Mockito.any());
+                Mockito.times(1)).suiteFinisher(Mockito.eq("story"), Mockito.any());
     }
 
     @Test
@@ -62,8 +60,7 @@ public void retryRecordCallIsFailPresentTest() {
         Mockito.when(testOutcome.getName()).thenReturn("Test name");
         recorder.record(testOutcome);
         Mockito.verify(suiteStorageMock,
-                Mockito.times(1)).isFailPresent(
-                Mockito.eq("story"), Mockito.eq("testId"));
+                Mockito.times(1)).isFailPresent("story", "testId");
     }
 
     @Test
@@ -82,8 +79,7 @@ public void retryRecordStoreNewFailTest() {
         Mockito.when(testOutcome.getResult()).thenReturn(TestResult.FAILURE);
         recorder.record(testOutcome);
         Mockito.verify(suiteStorageMock,
-                Mockito.times(2)).addFail(
-                Mockito.eq("story"), Mockito.eq("testId"));
+                Mockito.times(2)).addNewFail("story", "testId");
     }
 
     @Test
@@ -100,9 +96,26 @@ public void retryRecordClearSuiteIsRetryPassedTest() {
         Mockito.when(suiteStorageMock.isFailPresent(Mockito.any(), Mockito.any())).thenReturn(true);
         recorder.record(testOutcome);
         Mockito.verify(suiteStorageMock,
-                Mockito.times(1)).removeFail(
-                Mockito.eq("story"), Mockito.eq("testId"));
+                Mockito.times(1)).removeFail("story", "testId");
     }
 
 
+    @Test
+    public void retryRecordClearSuiteIfRetryCountExceed() {
+        System.setProperty(FAILSAFE_RERUN_KEY, "5");
+        TestRecorder recorder = new Regular(suiteStorageMock, launchMock, logUnitsHolderMock);
+        TestOutcome testOutcome = Mockito.mock(TestOutcome.class);
+        Mockito.when(testOutcome.getUserStory()).thenReturn(Story.called("story").withNarrative("narrative"));
+        Mockito.when(testOutcome.getId()).thenReturn("testId");
+        Mockito.when(testOutcome.getResult()).thenReturn(TestResult.UNSUCCESSFUL);
+        ZonedDateTime start = ZonedDateTime.now();
+        Mockito.when(testOutcome.getStartTime()).thenReturn(start);
+        Mockito.when(testOutcome.getName()).thenReturn("Test name");
+        Mockito.when(suiteStorageMock.isFailPresent(Mockito.any(), Mockito.any())).thenReturn(true);
+        Mockito.when(suiteStorageMock.incrementAndGetRetriesCount(Mockito.any(), Mockito.any())).thenReturn(5);
+        recorder.record(testOutcome);
+        Mockito.verify(suiteStorageMock,
+                Mockito.times(1)).removeFail("story", "testId");
+    }
+
 }