Fix with total violation count not decreasing on filtering comments o…

…ut (#208)

src/main/java/pl/touk/sputnik/engine/visitor/LimitCommentVisitor.java

@@ -22,8 +22,8 @@ public void afterReview(@NotNull Review review) {
             return;
         }
         log.info("There are {} total violations for this review, which is higher than maximum comment count {}. {} comments will be filtered out.", review.getTotalViolationCount(), maximumCount, review.getTotalViolationCount() - maximumCount);
-        filterOutComments(review);
         addMessage(review);
+        filterOutComments(review);
     }
 
     private void filterOutComments(Review review) {

src/main/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitor.java

@@ -27,7 +27,7 @@ private void addSummaryMessage(Review review) {
     }
 
     private String getSummaryMessage(@NotNull Review review) {
-        if (review.getTotalViolationCount() == 0) {
+        if (review.getTotalViolationCount() == 0L) {
             return perfectMessage;
         }
         String violationNoun = review.getTotalViolationCount() == 1 ? "violation" : "violations";

src/main/java/pl/touk/sputnik/engine/visitor/score/ScorePassIfNoErrors.java

@@ -19,8 +19,8 @@ public class ScorePassIfNoErrors implements AfterReviewVisitor {
 
     @Override
     public void afterReview(@NotNull Review review) {
-        Integer errorCount = review.getViolationCount().get(Severity.ERROR);
-        if (errorCount == null || errorCount == 0) {
+        long errorCount = review.getViolationCount(Severity.ERROR);
+        if (errorCount == 0) {
             log.info("Adding passing score {} for no errors found", passingScore);
             review.setScores(passingScore);
             return;

src/main/java/pl/touk/sputnik/review/Comment.java

@@ -10,4 +10,5 @@
 public class Comment {
     private final int line;
     private final String message;
+    private final Severity severity;
 }

src/main/java/pl/touk/sputnik/review/Review.java

@@ -13,7 +13,7 @@
 import pl.touk.sputnik.review.transformer.FileTransformer;
 
 import java.util.ArrayList;
-import java.util.EnumMap;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -24,8 +24,6 @@
 @ToString
 public class Review {
     private List<ReviewFile> files;
-    private Map<Severity, Integer> violationCount = new EnumMap<>(Severity.class);
-    private int totalViolationCount = 0;
 
     /**
      * Report problems with configuration, processors and other.
@@ -80,14 +78,20 @@ public void addError(String source, Violation violation) {
     }
 
     private void addError(@NotNull ReviewFile reviewFile, @NotNull String source, int line, @Nullable String message, Severity severity) {
-        reviewFile.getComments().add(new Comment(line, formatter.formatComment(source, severity, message)));
-        incrementCounters(severity);
+        reviewFile.getComments().add(new Comment(line, formatter.formatComment(source, severity, message), severity));
     }
 
-    private void incrementCounters(Severity severity) {
-        totalViolationCount += 1;
-        Integer currentCount = violationCount.get(severity);
-        violationCount.put(severity, currentCount == null ? 1 : currentCount + 1);
+    public long getTotalViolationCount() {
+        return files.stream().map(ReviewFile::getComments)
+                .mapToLong(Collection::size)
+                .sum();
+    }
+
+    public long getViolationCount(Severity severity) {
+        return files.stream().map(ReviewFile::getComments)
+                .flatMap(Collection::stream)
+                .filter(comment -> comment.getSeverity() == severity)
+                .count();
     }
 
     @NoArgsConstructor

src/test/java/pl/touk/sputnik/ReviewBuilder.java

@@ -5,6 +5,7 @@
 import pl.touk.sputnik.review.Review;
 import pl.touk.sputnik.review.ReviewFile;
 import pl.touk.sputnik.review.ReviewFormatterFactory;
+import pl.touk.sputnik.review.Severity;
 
 import java.util.Arrays;
 import java.util.List;
@@ -14,16 +15,15 @@ public class ReviewBuilder {
     public static Review buildReview(Configuration configuration) {
         List<ReviewFile> reviewFiles = Arrays.asList(buildReviewFile(1), buildReviewFile(2), buildReviewFile(3), buildReviewFile(4));
         Review review = new Review(reviewFiles, ReviewFormatterFactory.get(configuration));
-        review.setTotalViolationCount(8);
         review.getMessages().add("Total 8 violations found");
         review.getScores().put("Code-Review", (short) 1);
         return review;
     }
 
-    public static ReviewFile buildReviewFile(int i) {
+    private static ReviewFile buildReviewFile(int i) {
         ReviewFile reviewFile = new ReviewFile("filename" + i);
-        reviewFile.getComments().add(new Comment(0, "test1"));
-        reviewFile.getComments().add(new Comment(1, "test2"));
+        reviewFile.getComments().add(new Comment(0, "test1", Severity.INFO));
+        reviewFile.getComments().add(new Comment(1, "test2", Severity.INFO));
         return reviewFile;
     }
 }

src/test/java/pl/touk/sputnik/connector/local/LocalFacadeTest.java

@@ -17,6 +17,7 @@
 import pl.touk.sputnik.review.Comment;
 import pl.touk.sputnik.review.Review;
 import pl.touk.sputnik.review.ReviewFile;
+import pl.touk.sputnik.review.Severity;
 
 import java.io.IOException;
 import java.util.List;
@@ -106,8 +107,8 @@ void shouldWarnWithCommentsAndLineNumbers() {
         ReviewFile review2 = mock(ReviewFile.class);
         ReviewFile review3 = mock(ReviewFile.class);
         when(review3.getReviewFilename()).thenReturn("/path/to/file3");
-        when(review1.getComments()).thenReturn(ImmutableList.of(new Comment(11, "Comment 1"), new Comment(14, "Comment 2")));
-        when(review3.getComments()).thenReturn(ImmutableList.of(new Comment(15, "Comment 3")));
+        when(review1.getComments()).thenReturn(ImmutableList.of(new Comment(11, "Comment 1", Severity.INFO), new Comment(14, "Comment 2", Severity.INFO)));
+        when(review3.getComments()).thenReturn(ImmutableList.of(new Comment(15, "Comment 3", Severity.INFO)));
         when(review.getMessages()).thenReturn(ImmutableList.of("message 1", "message 2"));
         when(review.getFiles()).thenReturn(ImmutableList.of(review1, review2, review3));
 

src/test/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitorTest.java

@@ -1,55 +1,71 @@
 package pl.touk.sputnik.engine.visitor;
 
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import pl.touk.sputnik.TestEnvironment;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 import pl.touk.sputnik.review.Review;
 
+import java.util.ArrayList;
+import java.util.List;
+
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.when;
 
-class SummaryMessageVisitorTest extends TestEnvironment {
+@ExtendWith(MockitoExtension.class)
+class SummaryMessageVisitorTest {
 
     private static final String TOTAL_8_VIOLATIONS_FOUND = "Total 8 violations found";
     private static final String PROBLEM_SOURCE = "PMD";
     private static final String PROBLEM_MESSAGE = "configuration error";
     private static final String PROBLEM_FORMATTED_MESSAGE = "There is a problem with PMD: configuration error";
 
+    private final SummaryMessageVisitor summaryMessageVisitor = new SummaryMessageVisitor("Perfect");
+
+    @Mock
+    private Review review;
+    private List<String> messages = new ArrayList<>();
+
+    @BeforeEach
+    void setUp() {
+        when(review.getMessages()).thenReturn(messages);
+    }
+
     @Test
     void shouldAddSummaryMessage() {
-        Review review = review();
-        review.setTotalViolationCount(8);
+        when(review.getTotalViolationCount()).thenReturn(8L);
 
-        new SummaryMessageVisitor("Perfect").afterReview(review);
+        summaryMessageVisitor.afterReview(review);
 
         assertThat(review.getMessages()).containsOnly(TOTAL_8_VIOLATIONS_FOUND);
     }
 
     @Test
     void shouldAddSummaryMessageWithOneViolation() {
-        Review review = review();
-        review.setTotalViolationCount(1);
+        when(review.getTotalViolationCount()).thenReturn(1L);
 
-        new SummaryMessageVisitor("Perfect").afterReview(review);
+        summaryMessageVisitor.afterReview(review);
 
         assertThat(review.getMessages()).containsOnly("Total 1 violation found");
     }
 
     @Test
     void shouldAddPerfectMessageIfThereAreNoViolationsFound() {
-        Review review = review();
-        review.setTotalViolationCount(0);
+        when(review.getTotalViolationCount()).thenReturn(0L);
 
-        new SummaryMessageVisitor("Perfect").afterReview(review);
+        summaryMessageVisitor.afterReview(review);
 
         assertThat(review.getMessages()).containsOnly("Perfect");
     }
 
     @Test
     void shouldAddProblemMessagesPerfectMessageIfThereAreNoViolationsFound() {
-        Review review = review();
-        review.setTotalViolationCount(8);
-        review.addProblem(PROBLEM_SOURCE, PROBLEM_MESSAGE);
+        when(review.getTotalViolationCount()).thenReturn(8L);
+        when(review.getProblems()).thenReturn(singletonList("There is a problem with PMD: configuration error"));
 
-        new SummaryMessageVisitor("Perfect").afterReview(review);
+        summaryMessageVisitor.afterReview(review);
 
         assertThat(review.getMessages()).containsSequence(TOTAL_8_VIOLATIONS_FOUND, PROBLEM_FORMATTED_MESSAGE);
     }

src/test/java/pl/touk/sputnik/engine/visitor/score/NoScoreTest.java

@@ -1,17 +1,21 @@
 package pl.touk.sputnik.engine.visitor.score;
 
 import org.junit.jupiter.api.Test;
-import pl.touk.sputnik.TestEnvironment;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 import pl.touk.sputnik.review.Review;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-class NoScoreTest extends TestEnvironment {
+@ExtendWith(MockitoExtension.class)
+class NoScoreTest {
+
+    @Mock
+    private Review review;
 
     @Test
     void shouldAddNoScoreToReview() {
-        Review review = review();
-
         new NoScore().afterReview(review);
 
         assertThat(review.getScores()).isEmpty();

src/test/java/pl/touk/sputnik/engine/visitor/score/ScoreAlwaysPassTest.java

@@ -1,19 +1,38 @@
 package pl.touk.sputnik.engine.visitor.score;
 
 import com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import pl.touk.sputnik.TestEnvironment;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 import pl.touk.sputnik.review.Review;
+import pl.touk.sputnik.review.ReviewFile;
+import pl.touk.sputnik.review.ReviewFormatter;
+
+import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
 
-class ScoreAlwaysPassTest extends TestEnvironment {
+@ExtendWith(MockitoExtension.class)
+class ScoreAlwaysPassTest {
+
+    private Review review;
+
+    @Mock
+    private List<ReviewFile> files;
+
+    @Mock
+    private ReviewFormatter reviewFormatter;
+
+    @BeforeEach
+    void setUp() {
+        review = new Review(files, reviewFormatter);
+    }
 
     @Test
     void shouldAddScoreToReview() {
-        Review review = review();
-
         new ScoreAlwaysPass(ImmutableMap.of("Sputnik-Pass", (short) 1)).afterReview(review);
 
         assertThat(review.getScores()).containsOnly(entry("Sputnik-Pass", (short) 1));

src/test/java/pl/touk/sputnik/engine/visitor/score/ScorePassIfEmptyTest.java

@@ -2,23 +2,27 @@
 
 import com.google.common.collect.ImmutableMap;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 import pl.touk.sputnik.review.Review;
 
 import java.util.Map;
 
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+@ExtendWith(MockitoExtension.class)
 class ScorePassIfEmptyTest {
     private static final Map<String, Short> PASSING_SCORE = ImmutableMap.of("Sputnik-Pass", (short) 1);
     private static final Map<String, Short> FAILING_SCORE = ImmutableMap.of("Code-Review", (short) -2);
 
-    private Review reviewMock = mock(Review.class);
+    @Mock
+    private Review reviewMock;
 
     @Test
     void shouldPassIfViolationsIsEmpty() {
-        when(reviewMock.getTotalViolationCount()).thenReturn(0);
+        when(reviewMock.getTotalViolationCount()).thenReturn(0L);
 
         new ScorePassIfEmpty(PASSING_SCORE, FAILING_SCORE).afterReview(reviewMock);
 
@@ -27,7 +31,7 @@ void shouldPassIfViolationsIsEmpty() {
 
     @Test
     void shouldFailIfViolationsIsNotEmpty() {
-        when(reviewMock.getTotalViolationCount()).thenReturn(1);
+        when(reviewMock.getTotalViolationCount()).thenReturn(1L);
 
         new ScorePassIfEmpty(PASSING_SCORE, FAILING_SCORE).afterReview(reviewMock);
 

src/test/java/pl/touk/sputnik/engine/visitor/score/ScorePassIfNoErrorsTest.java

@@ -2,34 +2,28 @@
 
 import com.google.common.collect.ImmutableMap;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 import pl.touk.sputnik.review.Review;
 import pl.touk.sputnik.review.Severity;
 
 import java.util.Map;
 
-import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+@ExtendWith(MockitoExtension.class)
 class ScorePassIfNoErrorsTest {
     private static final Map<String, Short> PASSING_SCORE = ImmutableMap.of("Sputnik-Pass", (short) 1);
     private static final Map<String, Short> FAILING_SCORE = ImmutableMap.of("Code-Review", (short) -2);
 
-    private Review reviewMock = mock(Review.class, RETURNS_DEEP_STUBS);
-
-    @Test
-    void shouldPassIfErrorCountIsNull() {
-        when(reviewMock.getViolationCount().get(Severity.ERROR)).thenReturn(null);
-
-        new ScorePassIfNoErrors(PASSING_SCORE, FAILING_SCORE).afterReview(reviewMock);
-
-        verify(reviewMock).setScores(PASSING_SCORE);
-    }
+    @Mock
+    private Review reviewMock;
 
     @Test
     void shouldPassIfErrorCountIsZero() {
-        when(reviewMock.getViolationCount().get(Severity.ERROR)).thenReturn(0);
+        when(reviewMock.getViolationCount(Severity.ERROR)).thenReturn(0L);
 
         new ScorePassIfNoErrors(PASSING_SCORE, FAILING_SCORE).afterReview(reviewMock);
 
@@ -38,7 +32,7 @@ void shouldPassIfErrorCountIsZero() {
 
     @Test
     void shouldFailIfErrorCountIsNotZero() {
-        when(reviewMock.getViolationCount().get(Severity.ERROR)).thenReturn(1);
+        when(reviewMock.getViolationCount(Severity.ERROR)).thenReturn(1L);
 
         new ScorePassIfNoErrors(PASSING_SCORE, FAILING_SCORE).afterReview(reviewMock);