Merge pull request #189 from hankem/freeze_rules

Review comments to PR #181

archunit/src/main/java/com/tngtech/archunit/ArchConfiguration.java

@@ -58,14 +58,14 @@ public ArchConfiguration get() {
             return new ArchConfiguration();
         }
     });
-    private final Properties properties = new Properties();
 
     @PublicAPI(usage = ACCESS)
     public static ArchConfiguration get() {
         return INSTANCE.get();
     }
 
     private final String propertiesResourceName;
+    private final Properties properties = new Properties();
 
     private ArchConfiguration() {
         this(ARCHUNIT_PROPERTIES_RESOURCE_NAME);
@@ -220,7 +220,7 @@ public boolean containsProperty(String propertyName) {
 
     /**
      * @param propertyName Full name of a property
-     * @return A property of the global ArchUnit configuration. This method will throw an exception if the property ins not set within the configuration.
+     * @return A property of the global ArchUnit configuration. This method will throw an exception if the property is not set within the configuration.
      * @see #containsProperty(String)
      * @see #setProperty(String, String)
      */

archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java

@@ -36,23 +36,30 @@
 
 /**
  * A decorator around an existing {@link ArchRule} that "freezes" the state of all violations on the first call instead of failing the test.
- * This means in particular that the first run of a {@link FreezingArchRule} will always pass, consecutive calls will only fail if "unknown"
- * violations are introduced (read below for further explanations when a violation is "unknown").
+ * This means in particular that the first run of a {@link FreezingArchRule} will always pass.
+ * Consecutive calls will only fail if "unknown" violations are introduced (read below for further explanations when a violation is "unknown").
+ * Once resolved, initially "known" violations will fail again if they were re-introduced.
  * <br><br>
- * You might consider using this class, if you introduce a new {@link ArchRule} to an existing project that causes too many violations to solve
+ * You might consider using this class when introducing a new {@link ArchRule} to an existing project that causes too many violations to solve
  * at the current time. A typical example is a huge legacy project where a new rule might cause thousands of violations. Even if it is impossible
  * to fix all those violations at the moment, it is typically a good idea to a) make sure no further violations are introduced and
  * b) incrementally fix those violations over time one by one.
  * <br><br>
- * {@link FreezingArchRule} uses two concepts to support this use case. First a {@link ViolationStore} to store the result of the current
- * evaluation of this rule (compare {@link #persistIn(ViolationStore)}). Second a {@link ViolationLineMatcher} to decide which violations are "known",
- * i.e. have been present from the beginning (compare {@link #associateViolationLinesVia(ViolationLineMatcher)}).
- * The reason to adjust the {@link ViolationLineMatcher} and not simply check for equality might be to make the comparison for known violations
- * more resilient, e.g. by ignoring the current line number (assume a class has 500 lines, adding a line at the beginning should maybe not affect an
- * unrelated known violation in line 490).
- * <br><br>
- * If you do not configure {@link ViolationStore} or {@link ViolationLineMatcher}, a default will be used (compare the javadoc of the
- * respective class).
+ * {@link FreezingArchRule} uses two concepts to support this use case:
+ * <ul>
+ * <li>
+ *   a {@link ViolationStore} to store the result of the current evaluation and retrieve the result of the previous evaluation of this rule.<br>
+ *   It can be configured via {@link #persistIn(ViolationStore)}.<br>
+ *   The default {@link ViolationStore} stores violations in plain text files
+ *   within the {@code freeze.store.default.path} path of {@value com.tngtech.archunit.ArchConfiguration#ARCHUNIT_PROPERTIES_RESOURCE_NAME}
+ *   (default: {@code archunit_store})
+ * </li>
+ * <li>
+ *   a {@link ViolationLineMatcher} to decide which violations are "known", i.e. have already been present in the previous evaluation.<br>
+ *   It can be configured via {@link #associateViolationLinesVia(ViolationLineMatcher)}.<br>
+ *   The default {@link ViolationLineMatcher} compares violations ignoring the line number of their source code location.
+ * </li>
+ * </ul>
  */
 @PublicAPI(usage = ACCESS)
 public final class FreezingArchRule implements ArchRule {
@@ -108,15 +115,14 @@ private EvaluationResult storeViolationsAndReturnSuccess(EvaluationResult result
     private EvaluationResult removeObsoleteViolationsFromStoreAndReturnNewViolations(EvaluationResult result) {
         log.debug("Found frozen result for rule '{}'", delegate.getDescription());
         final List<String> knownViolations = store.getViolations(delegate);
-        CategorizedViolations categorizedViolations = removeObsoleteViolationsFromStore(result, knownViolations);
+        CategorizedViolations categorizedViolations = new CategorizedViolations(matcher, result, knownViolations);
+        removeObsoleteViolationsFromStore(categorizedViolations);
         return filterOutKnownViolations(result, categorizedViolations.getKnownActualViolations());
     }
 
-    private CategorizedViolations removeObsoleteViolationsFromStore(EvaluationResult result, List<String> knownViolations) {
-        CategorizedViolations categorizedViolations = new CategorizedViolations(matcher, result, knownViolations);
+    private void removeObsoleteViolationsFromStore(CategorizedViolations categorizedViolations) {
         log.debug("Removing obsolete violations from store: {}", categorizedViolations.getStoredSolvedViolations());
         store.save(delegate, categorizedViolations.getStoredUnsolvedViolations());
-        return categorizedViolations;
     }
 
     private EvaluationResult filterOutKnownViolations(EvaluationResult result, final Set<String> knownActualViolations) {
@@ -150,9 +156,7 @@ public FreezingArchRule persistIn(ViolationStore store) {
     }
 
     /**
-     * Allows to reconfigure how this {@link FreezingArchRule} will decide if an occurring violation is known or not. For example a
-     * {@link ViolationLineMatcher} that filters out the line number of a violation will consider all violations known that have the same
-     * textual description regardless of the concrete lines in which those violations have occurred.
+     * Allows to reconfigure how this {@link FreezingArchRule} will decide if an occurring violation is known or not.
      *
      * @param matcher A {@link ViolationLineMatcher} that decides which lines of a violation description are known and which are unknown and should
      *                cause a failure of this rule

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationLineMatcher.java

@@ -21,21 +21,15 @@
 import static com.tngtech.archunit.PublicAPI.Usage.INHERITANCE;
 
 /**
- * Allows to decide when two lines of two violations count as "equivalent". I.e. when {@link FreezingArchRule} determines if the description of an
- * occurring violation matches the description of an already stored one, it will use the configured {@link ViolationLineMatcher} to do so
- * by checking all the description lines of those two violations against each other.
- * <br><br>
- * A simple example could be to match any (xxx).java from lines and count all violations equivalent if they appear in the same class (as long as the
- * violations comply to the default description pattern adding 'in (ClassName.java:y) to the end of the lines). This would then effectively count all
- * violations in a class with any previous violation as known and not fail the check.
+ * Allows {@link FreezingArchRule} to decide when two lines of two violations count as "equivalent".
  */
 @PublicAPI(usage = INHERITANCE)
 public interface ViolationLineMatcher {
 
     /**
-     * @param lineFromFirstViolation A line from the description of a violation of an {@link ArchRule}
-     * @param lineFromSecondViolation A line from the description of another violation of an {@link ArchRule}
-     * @return true, if and only if those two lines should be considered equivalent (e.g. because only the line number differs)
+     * @param lineFromFirstViolation A line from the description of a violation of an {@link ArchRule} being evaluated
+     * @param lineFromSecondViolation A line from the description of a stored violation of an {@link ArchRule}
+     * @return true, if and only if those two lines should be considered equivalent
      */
     boolean matches(String lineFromFirstViolation, String lineFromSecondViolation);
 }

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationLineMatcherFactory.java

@@ -48,21 +48,21 @@ private static ViolationLineMatcher createInstance(String lineMatcherClassName)
     private static class FuzzyLineNumberMatcher implements ViolationLineMatcher {
         @Override
         public boolean matches(String lineFromFirstViolation, String lineFromSecondViolation) {
-            return ignoreLineNumber(lineFromFirstViolation).equals(ignoreLineNumber(lineFromSecondViolation));
+            return ignorePotentialLineNumbers(lineFromFirstViolation).equals(ignorePotentialLineNumbers(lineFromSecondViolation));
         }
 
         // Would be nicer to use a regex here, but unfortunately that would have a massive performance impact.
         // So we do some low level character processing instead.
-        private String ignoreLineNumber(String violation) {
+        private String ignorePotentialLineNumbers(String violation) {
             Iterator<String> parts = Splitter.on(":").split(violation).iterator();
             StringBuilder removedLineNumbers = new StringBuilder(parts.next());
             while (parts.hasNext()) {
-                removedLineNumbers.append(removeLineNumberIfPresent(parts.next()));
+                removedLineNumbers.append(removeNumberIfPresent(parts.next()));
             }
             return removedLineNumbers.toString();
         }
 
-        private String removeLineNumberIfPresent(String part) {
+        private String removeNumberIfPresent(String part) {
             int i = 0;
             while (part.length() > i && isDigit(part.charAt(i))) {
                 i++;

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStore.java

@@ -24,16 +24,17 @@
 import static com.tngtech.archunit.PublicAPI.Usage.INHERITANCE;
 
 /**
- * Allows to provide some sort of storage for existing violations. In particular on the first check of a {@link FreezingArchRule}, all existing
- * violations will be persisted to the configured {@link ViolationStore}.
+ * Provides some sort of storage for violations to {@link FreezingArchRule}.
  */
 @PublicAPI(usage = INHERITANCE)
 public interface ViolationStore {
 
     /**
-     * Provides custom initialization. The properties will be derived from
-     * {@value com.tngtech.archunit.ArchConfiguration#ARCHUNIT_PROPERTIES_RESOURCE_NAME} by considering the sub properties of {@code freeze.store}.
-     * I.e. if {@value com.tngtech.archunit.ArchConfiguration#ARCHUNIT_PROPERTIES_RESOURCE_NAME} contains
+     * Provides custom initialization with properties derived from
+     * {@value com.tngtech.archunit.ArchConfiguration#ARCHUNIT_PROPERTIES_RESOURCE_NAME}
+     * by considering the sub properties of {@code freeze.store}.
+     * <br><br>
+     * If {@value com.tngtech.archunit.ArchConfiguration#ARCHUNIT_PROPERTIES_RESOURCE_NAME} contains, e.g.,
      *
      * <pre><code>
      * freeze.store.propOne=valueOne

archunit/src/test/java/com/tngtech/archunit/ArchConfigurationTest.java

@@ -184,7 +184,7 @@ public void creates_extension_properties_from_prefix() {
 
     @Test
     public void allows_to_specify_custom_properties() {
-        writeProperties(ImmutableMap.<String, Object>of(
+        writeProperties(ImmutableMap.of(
                 "some.custom.booleanproperty", "true",
                 "some.custom.stringproperty", "value",
                 "toignore", "toignore"

archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java

@@ -196,7 +196,7 @@ public void fails_on_an_increased_violation_count_of_the_same_violation_compared
                 .withViolations("violation"));
 
         ArchRule frozen = freeze(rule("some description")
-                .withViolations("violation", "second"))
+                .withViolations("violation", "equivalent one"))
                 .persistIn(violationStore)
                 .associateViolationLinesVia(new ViolationLineMatcher() {
                     @Override
@@ -207,7 +207,8 @@ public boolean matches(String lineFromFirstViolation, String lineFromSecondViola
 
         assertThat(frozen)
                 .checking(importClasses(getClass()))
-                .hasAnyViolationOf("violation", "second");
+                .hasViolations(1)
+                .hasAnyViolationOf("violation", "equivalent one");
     }
 
     @Test

archunit/src/test/java/com/tngtech/archunit/testutil/assertion/ArchRuleCheckAssertion.java

@@ -5,63 +5,49 @@
 import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.lang.ArchRule;
 import com.tngtech.archunit.lang.EvaluationResult;
-import org.assertj.core.api.AbstractObjectAssert;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.guava.api.Assertions.assertThat;
 
-public class ArchRuleCheckAssertion extends AbstractObjectAssert<ArchRuleCheckAssertion, ArchRuleCheckAssertion.Check> {
+public class ArchRuleCheckAssertion {
+    private final EvaluationResult evaluationResult;
+    private final Optional<AssertionError> error;
+
     ArchRuleCheckAssertion(ArchRule rule, JavaClasses classes) {
-        super(new Check(rule, classes), ArchRuleCheckAssertion.class);
+        evaluationResult = rule.evaluate(classes);
+        error = checkRule(rule, classes);
+    }
+
+    private Optional<AssertionError> checkRule(ArchRule rule, JavaClasses classes) {
+        try {
+            rule.check(classes);
+            return Optional.absent();
+        } catch (AssertionError error) {
+            return Optional.of(error);
+        }
     }
 
     public ArchRuleCheckAssertion hasOnlyViolations(String... violations) {
-        actual.checkContainsOnlyViolations(violations);
+        assertThat(evaluationResult.getFailureReport().getDetails()).containsOnly(violations);
+        for (String violation : violations) {
+            assertThat(error.get().getMessage()).contains(violation);
+        }
         return this;
     }
 
     public ArchRuleCheckAssertion hasAnyViolationOf(String... violations) {
-        actual.checkContainsAnyViolationOf(violations);
+        assertThat(evaluationResult.getFailureReport().getDetails()).containsAnyOf(violations);
+        assertThat(error.get().getMessage()).containsPattern(Joiner.on("|").join(violations));
         return this;
     }
 
-    public void hasNoViolation() {
-        actual.checkNoViolation();
+    public ArchRuleCheckAssertion hasViolations(int numberOfViolations) {
+        assertThat(evaluationResult.getFailureReport().getDetails()).as("number of violation").hasSize(numberOfViolations);
+        return this;
     }
 
-    static class Check {
-        private final EvaluationResult evaluationResult;
-        private final Optional<AssertionError> error;
-
-        Check(ArchRule rule, JavaClasses classes) {
-            evaluationResult = rule.evaluate(classes);
-            error = checkRule(rule, classes);
-        }
-
-        private Optional<AssertionError> checkRule(ArchRule rule, JavaClasses classes) {
-            try {
-                rule.check(classes);
-                return Optional.absent();
-            } catch (AssertionError error) {
-                return Optional.of(error);
-            }
-        }
-
-        void checkContainsOnlyViolations(String[] violations) {
-            assertThat(evaluationResult.getFailureReport().getDetails()).containsOnly(violations);
-            for (String violation : violations) {
-                assertThat(error.get().getMessage()).contains(violation);
-            }
-        }
-
-        void checkContainsAnyViolationOf(String[] violations) {
-            assertThat(evaluationResult.getFailureReport().getDetails()).containsAnyOf(violations);
-            assertThat(error.get().getMessage()).containsPattern(Joiner.on("|").join(violations));
-        }
-
-        void checkNoViolation() {
-            assertThat(evaluationResult.hasViolation()).as("result has violation").isFalse();
-            assertThat(error).as("error").isAbsent();
-        }
+    public void hasNoViolation() {
+        assertThat(evaluationResult.hasViolation()).as("result has violation").isFalse();
+        assertThat(error).as("error").isAbsent();
     }
 }

archunit/src/test/java/com/tngtech/archunit/testutil/assertion/ConditionEventsAssertion.java

@@ -7,17 +7,16 @@
 import java.util.Set;
 import java.util.regex.Pattern;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.tngtech.archunit.lang.ConditionEvent;
 import com.tngtech.archunit.lang.ConditionEvents;
-import org.assertj.core.api.AbstractCharSequenceAssert;
 import org.assertj.core.api.AbstractIterableAssert;
 import org.assertj.core.api.Assertions;
 import org.assertj.core.api.ObjectAssert;
 import org.assertj.core.api.ObjectAssertFactory;
 
 import static com.google.common.collect.Iterables.getOnlyElement;
-import static com.google.common.collect.Lists.newArrayList;
 
 public class ConditionEventsAssertion
         extends AbstractIterableAssert<ConditionEventsAssertion, ConditionEvents, ConditionEvent, ObjectAssert<ConditionEvent>> {
@@ -36,7 +35,7 @@ public void containViolations(String violation, String... additional) {
     }
 
     public void containAllowed(String message, String... additional) {
-        Assertions.assertThat(actual.getAllowed().size()).as("Allowed events occurred").isGreaterThan(0);
+        Assertions.assertThat(actual.getAllowed()).as("Allowed events").isNotEmpty();
 
         List<String> expected = concat(message, additional);
         if (!sorted(messagesOf(actual.getAllowed())).equals(sorted(expected))) {
@@ -53,13 +52,14 @@ private List<String> messagesOf(Collection<? extends ConditionEvent> events) {
     }
 
     private List<String> concat(String violation, String[] additional) {
-        ArrayList<String> list = newArrayList(additional);
-        list.add(0, violation);
-        return list;
+        return ImmutableList.<String>builder()
+                .add(violation)
+                .add(additional)
+                .build();
     }
 
     private List<String> sorted(Collection<String> collection) {
-        ArrayList<String> result = new ArrayList<>(collection);
+        List<String> result = new ArrayList<>(collection);
         Collections.sort(result);
         return result;
     }
@@ -75,9 +75,9 @@ public ConditionEventsAssertion haveOneViolationMessageContaining(String... mess
 
     public ConditionEventsAssertion haveOneViolationMessageContaining(Set<String> messageParts) {
         Assertions.assertThat(messagesOf(actual.getViolating())).as("Number of violations").hasSize(1);
-        AbstractCharSequenceAssert<?, String> assertion = Assertions.assertThat(getOnlyElement(messagesOf(actual.getViolating())));
+        String singleViolationMessage = getOnlyElement(messagesOf(actual.getViolating()));
         for (String part : messageParts) {
-            assertion.as("Violation message").contains(part);
+            Assertions.assertThat(singleViolationMessage).as("Violation message").contains(part);
         }
         return this;
     }