Added support for global problems collection

Change-Id: I187315ddf04d4e55771c8078da306ee23e869e39
TouK · Nov 14, 2014 · 75b30e1 · 75b30e1
1 parent de87616
commit 75b30e1
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 73 deletions.
diff --git a/src/main/java/pl/touk/sputnik/engine/Engine.java b/src/main/java/pl/touk/sputnik/engine/Engine.java
@@ -1,20 +1,13 @@
 package pl.touk.sputnik.engine;
 
-import com.google.common.collect.ImmutableMap;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang.math.NumberUtils;
 import org.jetbrains.annotations.NotNull;
-import pl.touk.sputnik.configuration.ConfigurationHolder;
 import pl.touk.sputnik.connector.ConnectorFacade;
+import pl.touk.sputnik.engine.visitor.AfterReviewVisitor;
+import pl.touk.sputnik.engine.visitor.BeforeReviewVisitor;
+import pl.touk.sputnik.review.*;
 
-import java.util.ArrayList;
 import java.util.List;
-import pl.touk.sputnik.configuration.GeneralOption;
-import pl.touk.sputnik.review.Review;
-import pl.touk.sputnik.review.ReviewFile;
-import pl.touk.sputnik.review.ReviewProcessor;
-import pl.touk.sputnik.review.ReviewResult;
-import pl.touk.sputnik.engine.visitor.*;
 
 @Slf4j
 public class Engine {
@@ -48,8 +41,15 @@ public void run() {
     private void review(@NotNull Review review, @NotNull ReviewProcessor processor) {
         log.info("Review started for processor {}", processor.getName());
         long start = System.currentTimeMillis();
+        ReviewResult reviewResult = null;
 
-        ReviewResult reviewResult = processor.process(review);
+        try {
+            reviewResult = processor.process(review);
+        } catch (ReviewException e) {
+            log.error("Processor {} error", processor.getName(), e);
+            review.addProblem(processor.getName(), e.getMessage());
+
+        }
         log.info("Review finished for processor {}. Took {} s", processor.getName(), (System.currentTimeMillis() - start) / THOUSAND);
 
         if (reviewResult == null) {

diff --git a/src/main/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitor.java b/src/main/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitor.java
@@ -6,11 +6,31 @@
 
 @Slf4j
 public class SummaryMessageVisitor implements AfterReviewVisitor {
+    private static final String PERFECT_MESSAGE = "Perfect!";
 
     @Override
     public void afterReview(@NotNull Review review) {
-        String message = String.format("Total %d violations found", review.getTotalViolationCount());
-        log.info("Adding summary message to review: {}", message);
-        review.getMessages().add(message);
+        addSummaryMessage(review);
+        addProblemMessages(review);
+    }
+
+    private void addSummaryMessage(Review review) {
+        String summaryMessage = getSummaryMessage(review);
+        log.info("Adding summary message to review: {}", summaryMessage);
+        review.getMessages().add(summaryMessage);
+    }
+
+    private String getSummaryMessage(@NotNull Review review) {
+        if (review.getTotalViolationCount() == 0) {
+            return PERFECT_MESSAGE;
+        }
+        return String.format("Total %d violations found", review.getTotalViolationCount());
+    }
+
+    private void addProblemMessages(@NotNull Review review) {
+        for (String problemMessage : review.getProblems()) {
+            log.info("Adding problem message to review: {}", problemMessage);
+            review.getMessages().add(problemMessage);
+        }
     }
 }
diff --git a/src/main/java/pl/touk/sputnik/processor/findbugs/FindBugsProcessor.java b/src/main/java/pl/touk/sputnik/processor/findbugs/FindBugsProcessor.java
@@ -1,27 +1,19 @@
 package pl.touk.sputnik.processor.findbugs;
 
-import java.io.IOException;
-
+import edu.umd.cs.findbugs.*;
+import edu.umd.cs.findbugs.config.UserPreferences;
 import lombok.extern.slf4j.Slf4j;
-
 import org.apache.commons.lang3.StringUtils;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-
 import pl.touk.sputnik.configuration.ConfigurationHolder;
 import pl.touk.sputnik.configuration.GeneralOption;
 import pl.touk.sputnik.review.Review;
+import pl.touk.sputnik.review.ReviewException;
 import pl.touk.sputnik.review.ReviewProcessor;
 import pl.touk.sputnik.review.ReviewResult;
 import pl.touk.sputnik.review.filter.JavaFilter;
 import pl.touk.sputnik.review.transformer.ClassNameTransformer;
-import edu.umd.cs.findbugs.ClassScreener;
-import edu.umd.cs.findbugs.DetectorFactoryCollection;
-import edu.umd.cs.findbugs.FindBugs2;
-import edu.umd.cs.findbugs.IClassScreener;
-import edu.umd.cs.findbugs.Priorities;
-import edu.umd.cs.findbugs.Project;
-import edu.umd.cs.findbugs.config.UserPreferences;
 
 @Slf4j
 public class FindBugsProcessor implements ReviewProcessor {
@@ -35,12 +27,9 @@ public ReviewResult process(@NotNull Review review) {
         FindBugs2 findBugs = createFindBugs2(review);
         try {
             findBugs.execute();
-        } catch (IOException e) {
-            // this may be thrown when there is no file to analyze
-            log.warn("FindBugs process warning or error", e);
-        } catch (InterruptedException e) {
-            // TODO this seems to be more complex issue, do we want to suppress it?
-            log.error("FindBugs process crytical error", e);
+        } catch (Exception e) {
+            log.error("FindBugs processing error", e);
+            throw new ReviewException("FindBugs processing error", e);
         }
         return collectorBugReporter.getReviewResult();
     }

diff --git a/src/main/java/pl/touk/sputnik/processor/pmd/PmdProcessor.java b/src/main/java/pl/touk/sputnik/processor/pmd/PmdProcessor.java
@@ -1,14 +1,8 @@
 package pl.touk.sputnik.processor.pmd;
 
 import com.google.common.base.Joiner;
-import net.sourceforge.pmd.PMD;
-import net.sourceforge.pmd.PMDConfiguration;
-import net.sourceforge.pmd.Rule;
-import net.sourceforge.pmd.RuleContext;
-import net.sourceforge.pmd.RuleSet;
-import net.sourceforge.pmd.RuleSetFactory;
-import net.sourceforge.pmd.RuleSets;
-import net.sourceforge.pmd.RulesetsFactoryUtils;
+import lombok.extern.slf4j.Slf4j;
+import net.sourceforge.pmd.*;
 import net.sourceforge.pmd.benchmark.Benchmark;
 import net.sourceforge.pmd.benchmark.Benchmarker;
 import net.sourceforge.pmd.lang.Language;
@@ -19,19 +13,19 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import pl.touk.sputnik.configuration.ConfigurationHolder;
+import pl.touk.sputnik.configuration.GeneralOption;
 import pl.touk.sputnik.review.Review;
+import pl.touk.sputnik.review.ReviewException;
 import pl.touk.sputnik.review.ReviewProcessor;
 import pl.touk.sputnik.review.ReviewResult;
+import pl.touk.sputnik.review.filter.PmdFilter;
+import pl.touk.sputnik.review.transformer.FileNameTransformer;
 
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
-import lombok.extern.slf4j.Slf4j;
-import pl.touk.sputnik.configuration.GeneralOption;
-import pl.touk.sputnik.review.filter.PmdFilter;
-import pl.touk.sputnik.review.transformer.FileNameTransformer;
 
 @Slf4j
 public class PmdProcessor implements ReviewProcessor {
@@ -49,8 +43,8 @@ public ReviewResult process(@NotNull Review review) {
             configuration.setInputPaths(Joiner.on(PMD_INPUT_PATH_SEPARATOR).join(review.getFiles(new PmdFilter(), new FileNameTransformer())));
             doPMD(configuration);
         } catch (RuntimeException e) {
-            log.error("PMD processor error. Something wrong with configuration or analyzed files are not in workspace.", e);
-            // TODO: there is problem with configuration: stop processing and let the user fix the problem
+            log.error("PMD processing error. Something wrong with configuration or analyzed files are not in workspace.", e);
+            throw new ReviewException("PMD processing error", e);
         }
         return renderer != null ? ((CollectorRenderer)renderer).getReviewResult() : null;
     }
@@ -71,7 +65,7 @@ private String getRulesets() {
     /**
      * PMD has terrible design of process configuration. You must use report file with it. I paste this method here and
      * improve it.
-     * 
+     *
      * @throws IllegalArgumentException
      *             if the configuration is not correct
      */
@@ -85,7 +79,7 @@ public void doPMD(@NotNull PMDConfiguration configuration) throws IllegalArgumen
         if (ruleSets == null) {
             return;
         }
-        
+
         Set<Language> languages = getApplicableLanguages(configuration, ruleSets);
         // this throws RuntimeException when modified file does not exist in workspace
         List<DataSource> files = PMD.getApplicableFiles(configuration, languages);

diff --git a/src/main/java/pl/touk/sputnik/review/Review.java b/src/main/java/pl/touk/sputnik/review/Review.java
@@ -1,34 +1,40 @@
 package pl.touk.sputnik.review;
 
-import java.util.ArrayList;
-import java.util.EnumMap;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
+import com.google.common.base.Function;
+import com.google.common.collect.Lists;
 import lombok.Getter;
+import lombok.NoArgsConstructor;
 import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
-
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-
 import pl.touk.sputnik.review.filter.FileFilter;
 import pl.touk.sputnik.review.transformer.FileTransformer;
 
-import com.google.common.base.Function;
-import com.google.common.collect.Lists;
+import java.util.*;
 
 @Slf4j
 @Getter
 @Setter
 public class Review {
     /* Source, severity, message, e.g. [Checkstyle] Info: This is bad */
     private static final String COMMENT_FORMAT = "[%s] %s: %s";
+    private static final String PROBLEM_FORMAT = "There is a problem with %s: %s";
 
     private List<ReviewFile> files;
     private Map<Severity, Integer> violationCount = new EnumMap<>(Severity.class);
     private int totalViolationCount = 0;
+
+    /**
+     * Report problems with configuration, processors and other.
+     * There problems should be displayed on review summary with your code-review tool
+     *
+     */
+    private List<String> problems = new ArrayList<>();
+
+    /**
+     * Messages that will be displayed on review summary with your code-review tool
+     */
     private List<String> messages = new ArrayList<>();
     private Map<String, Integer> scores = new HashMap<>();
 
@@ -51,6 +57,10 @@ public List<String> getSourceDirs() {
         return Lists.transform(files, new ReviewFileSourceDirFunction());
     }
 
+    public void addProblem(@NotNull String source, @NotNull String problem) {
+        problems.add(String.format(PROBLEM_FORMAT, source, problem));
+    }
+
     public void add(@NotNull String source, @NotNull ReviewResult reviewResult) {
         for (Violation violation : reviewResult.getViolations()) {
             addError(source, violation);
@@ -75,26 +85,23 @@ private void addError(@NotNull ReviewFile reviewFile, @NotNull String source, in
     }
 
     private void incrementCounters(Severity severity) {
+        totalViolationCount += 1;
         Integer currentCount = violationCount.get(severity);
         violationCount.put(severity, currentCount == null ? 1 : currentCount + 1);
     }
 
+    @NoArgsConstructor
     private static class ReviewFileBuildDirFunction implements Function<ReviewFile, String> {
 
-        ReviewFileBuildDirFunction() {
-        }
-
         @Override
         public String apply(ReviewFile from) {
             return from.getBuildDir();
         }
     }
 
+    @NoArgsConstructor
     private static class ReviewFileSourceDirFunction implements Function<ReviewFile, String> {
 
-        ReviewFileSourceDirFunction() {
-        }
-
         @Override
         public String apply(ReviewFile from) {
             return from.getSourceDir();

diff --git a/src/test/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitorTest.java b/src/test/java/pl/touk/sputnik/engine/visitor/SummaryMessageVisitorTest.java
@@ -10,14 +10,40 @@
 
 public 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";
+
     @Test
     public void shouldAddSummaryMessage() {
         Review review = new Review(Collections.<ReviewFile>emptyList());
         review.setTotalViolationCount(8);
 
         new SummaryMessageVisitor().afterReview(review);
 
-        assertThat(review.getMessages()).containsOnly("Total 8 violations found");
+        assertThat(review.getMessages()).containsOnly(TOTAL_8_VIOLATIONS_FOUND);
+    }
+
+    @Test
+    public void shouldAddPerfectMessageIfThereAreNoViolationsFound() {
+        Review review = new Review(Collections.<ReviewFile>emptyList());
+        review.setTotalViolationCount(0);
+
+        new SummaryMessageVisitor().afterReview(review);
+
+        assertThat(review.getMessages()).containsOnly("Perfect!");
+    }
+
+    @Test
+    public void shouldAddProblemMessagesPerfectMessageIfThereAreNoViolationsFound() {
+        Review review = new Review(Collections.<ReviewFile>emptyList());
+        review.setTotalViolationCount(8);
+        review.addProblem(PROBLEM_SOURCE, PROBLEM_MESSAGE);
+
+        new SummaryMessageVisitor().afterReview(review);
+
+        assertThat(review.getMessages()).containsSequence(TOTAL_8_VIOLATIONS_FOUND, PROBLEM_FORMATTED_MESSAGE);
     }
 
 }
diff --git a/src/test/java/pl/touk/sputnik/processor/findbugs/FindBugsProcessorTest.java b/src/test/java/pl/touk/sputnik/processor/findbugs/FindBugsProcessorTest.java
@@ -11,16 +11,18 @@
 import org.mockito.runners.MockitoJUnitRunner;
 import pl.touk.sputnik.configuration.ConfigurationHolder;
 import pl.touk.sputnik.review.Review;
+import pl.touk.sputnik.review.ReviewException;
 import pl.touk.sputnik.review.ReviewFile;
 import pl.touk.sputnik.review.ReviewResult;
 
+import static com.googlecode.catchexception.CatchException.caughtException;
+import static com.googlecode.catchexception.apis.CatchExceptionAssertJ.when;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
 
 @RunWith(MockitoJUnitRunner.class)
 public class FindBugsProcessorTest {
 
-    private final FindBugsProcessor fixture = new FindBugsProcessor();
+    private final FindBugsProcessor findBugsProcessor = new FindBugsProcessor();
 
     @Mock
     private Review review;
@@ -35,19 +37,16 @@ public void tearDown() throws Exception {
         ConfigurationHolder.reset();
     }
 
-
     @Test
-    public void shouldReturnEmptyResultWhenFileNotFound() {
+    public void shouldThrowWhenFileNotFound() {
         //given
         Review review = new Review(ImmutableList.of(new ReviewFile("test")));
 
         //when
-        ReviewResult reviewResult = fixture.process(mock(Review.class));
+        when(findBugsProcessor).process(review);
 
         //then
-        assertThat(reviewResult).isNotNull();
-        assert reviewResult != null;
-        assertThat(reviewResult.getViolations()).isEmpty();
+        assertThat(caughtException()).isInstanceOf(ReviewException.class);
     }
 
     @Test
@@ -57,7 +56,7 @@ public void shouldReturnBasicViolationsOnEmptyClass() {
         Review review = new Review(ImmutableList.of(new ReviewFile(Resources.getResource("TestFile.java").getFile())));
 
         //when
-        ReviewResult reviewResult = fixture.process(review);
+        ReviewResult reviewResult = findBugsProcessor.process(review);
 
         //then
         assertThat(reviewResult).isNotNull();