Verify that the rule input is not empty #774

The input for a rule should normally not be empty, since then the test will always be green because it doesn't test anything.
With this change, it is now possible to verify that the input of a rule is not empty. If the input is empty, an `AssertionError` is thrown.

The verification if the input is empty can also be controlled via the configuration.
By setting the value `archRule.verifyInputNotEmpty` to `true` or `false`, we can turn on or off the verification.
The default value is `true` which means this is a breaking change.
However, with the configuration, the old behavior can easily be restored.

Resolve #178

Author: codecholeric

.gitignore

@@ -13,3 +13,4 @@ _site
 .jekyll-cache
 .asciidoctor
 verification-results
+.java-version

archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java

@@ -23,6 +23,7 @@
 
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableSet;
+import com.tngtech.archunit.ArchConfiguration;
 import com.tngtech.archunit.Internal;
 import com.tngtech.archunit.PublicAPI;
 import com.tngtech.archunit.base.Optional;
@@ -36,9 +37,11 @@
 import com.tngtech.archunit.lang.syntax.elements.ClassesThat;
 import com.tngtech.archunit.lang.syntax.elements.GivenClasses;
 
+import static com.google.common.collect.Iterables.isEmpty;
 import static com.google.common.io.Resources.readLines;
 import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
 import static com.tngtech.archunit.base.ClassLoaders.getCurrentClassLoader;
+import static java.lang.Boolean.TRUE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
@@ -175,6 +178,8 @@ static String createBecauseDescription(ArchRule rule, String reason) {
         }
 
         private static class SimpleArchRule<T> implements ArchRule {
+            private static final String FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME = "archRule.failOnEmptyShould";
+
             private final Priority priority;
             private final ClassesTransformer<T> classesTransformer;
             private final ArchCondition<T> condition;
@@ -206,6 +211,8 @@ public ArchRule because(String reason) {
             @Override
             public EvaluationResult evaluate(JavaClasses classes) {
                 Iterable<T> allObjects = classesTransformer.transform(classes);
+                verifyNoEmptyShouldIfEnabled(allObjects);
+
                 condition.init(allObjects);
                 ConditionEvents events = new ConditionEvents();
                 for (T object : allObjects) {
@@ -215,6 +222,21 @@ public EvaluationResult evaluate(JavaClasses classes) {
                 return new EvaluationResult(this, events, priority);
             }
 
+            private void verifyNoEmptyShouldIfEnabled(Iterable<T> allObjects) {
+                if (isEmpty(allObjects) && isFailOnEmptyShouldEnabled()) {
+                    throw new AssertionError("Rule failed to check any classes. " +
+                            "This means either that no classes have been passed to the rule at all, " +
+                            "or that no classes passed to the rule matched the `that()` clause. " +
+                            "To allow rules being evaluated without checking any classes you can set the ArchUnit property " +
+                            FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME + " = " + false);
+                }
+            }
+
+            private boolean isFailOnEmptyShouldEnabled() {
+                return ArchConfiguration.get().getPropertyOrDefault(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME, TRUE.toString())
+                        .equals(TRUE.toString());
+            }
+
             @Override
             public String getDescription() {
                 return overriddenDescription.isPresent() ?

archunit/src/test/java/com/tngtech/archunit/lang/ArchRuleTest.java

@@ -1,13 +1,23 @@
 package com.tngtech.archunit.lang;
 
+import java.io.File;
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.List;
+import java.util.SortedSet;
+import java.util.TreeSet;
+
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.io.Files;
+import com.tngtech.archunit.ArchConfiguration;
 import com.tngtech.archunit.core.domain.JavaClass;
 import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.core.domain.JavaClassesTest;
+import com.tngtech.archunit.core.importer.testexamples.SomeClass;
 import com.tngtech.archunit.lang.ArchConditionTest.ConditionWithInitAndFinish;
 import com.tngtech.archunit.lang.syntax.ArchRuleDefinition;
+import com.tngtech.archunit.testutil.ArchConfigurationRule;
 import org.hamcrest.Description;
 import org.hamcrest.TypeSafeMatcher;
 import org.junit.After;
@@ -16,13 +26,6 @@
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
-import java.io.File;
-import java.io.IOException;
-import java.util.Iterator;
-import java.util.List;
-import java.util.SortedSet;
-import java.util.TreeSet;
-
 import static com.google.common.collect.Lists.newArrayList;
 import static com.tngtech.archunit.core.domain.TestUtils.importClasses;
 import static com.tngtech.archunit.core.domain.TestUtils.importClassesWithContext;
@@ -31,13 +34,19 @@
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.all;
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
 import static com.tngtech.archunit.testutil.TestUtils.toUri;
+import static java.lang.Boolean.FALSE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class ArchRuleTest {
+    private static final String FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME = "archRule.failOnEmptyShould";
+
     @Rule
     public final ExpectedException thrown = ExpectedException.none();
 
+    @Rule
+    public final ArchConfigurationRule archConfigurationRule = new ArchConfigurationRule();
+
     @Before
     public void setUp() {
         ignoreFile().delete();
@@ -159,6 +168,35 @@ public void finish(ConditionEvents events) {
         assertThat(condition.eventsFromFinish.getViolating()).hasSize(1);
     }
 
+    @Test
+    public void evaluation_fails_because_of_empty_set_of_classes_with_default_fail_on_empty_should() {
+        archConfigurationRule.removeProperty(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME);
+
+        thrown.expect(AssertionError.class);
+        thrown.expectMessage("Rule failed to check any classes");
+        thrown.expectMessage(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME);
+
+        classes().should(ALWAYS_BE_VALID).evaluate(importClasses());
+    }
+
+    @Test
+    public void evaluation_fails_because_of_empty_set_of_classes_after_filter_with_default_fail_on_empty_should() {
+        archConfigurationRule.removeProperty(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME);
+
+        thrown.expect(AssertionError.class);
+        thrown.expectMessage("Rule failed to check any classes");
+        thrown.expectMessage(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME);
+
+        classes().that().doNotHaveSimpleName(SomeClass.class.getSimpleName()).should(ALWAYS_BE_VALID).evaluate(importClasses(SomeClass.class));
+    }
+
+    @Test
+    public void evaluation_passes_on_empty_set_of_classes_with_deactivated_fail_on_empty_should() {
+        ArchConfiguration.get().setProperty(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME, FALSE.toString());
+
+        classes().should(ALWAYS_BE_VALID).evaluate(importClasses());
+    }
+
     private ClassesTransformer<String> strings() {
         return new AbstractClassesTransformer<String>("strings") {
             @Override
@@ -250,10 +288,18 @@ public void check(JavaClass item, ConditionEvents events) {
                 }
             };
 
+    private static final ArchCondition<JavaClass> ALWAYS_BE_VALID =
+            new ArchCondition<JavaClass>("always be valid") {
+                @Override
+                public void check(JavaClass item, ConditionEvents events) {
+                }
+            };
+
+    @SuppressWarnings({"unused", "ResultOfMethodCallIgnored"})
     private static class ClassAccessingStringTwoTimes {
         void execute() {
             "foo".length();
             "bar".replaceAll("a", "b");
         }
     }
-}
+}

archunit/src/test/java/com/tngtech/archunit/testutil/ArchConfigurationRule.java

@@ -1,10 +1,15 @@
 package com.tngtech.archunit.testutil;
 
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.InvocationTargetException;
 import java.util.concurrent.Callable;
 
 import com.tngtech.archunit.ArchConfiguration;
 import org.junit.rules.ExternalResource;
 
+import static com.tngtech.archunit.testutil.ReflectionTestUtils.field;
+import static com.tngtech.archunit.testutil.ReflectionTestUtils.method;
+
 public class ArchConfigurationRule extends ExternalResource {
     private boolean resolveMissingDependenciesFromClassPath = ArchConfiguration.get().resolveMissingDependenciesFromClassPath();
 
@@ -24,6 +29,20 @@ public ArchConfigurationRule resolveAdditionalDependenciesFromClassPath(boolean
         return this;
     }
 
+    public void removeProperty(String propertyName) {
+        try {
+            Object properties = accessible(field(ArchConfiguration.class, "properties")).get(ArchConfiguration.get());
+            accessible(method(properties.getClass(), "remove", String.class)).invoke(properties, propertyName);
+        } catch (IllegalAccessException | InvocationTargetException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private <T extends AccessibleObject> T accessible(T member) {
+        member.setAccessible(true);
+        return member;
+    }
+
     @Override
     protected void before() {
         ArchConfiguration.get().reset();

archunit/src/test/resources/archunit.properties

@@ -1 +1,2 @@
 enableMd5InClassSources=true
+archRule.failOnEmptyShould=false

docs/userguide/010_Advanced_Configuration.adoc

@@ -98,6 +98,32 @@ If this feature is enabled, the MD5 sum can be queried as
 javaClass.getSource().get().getMd5sum()
 ----
 
+=== Fail Rules on Empty Should
+
+By default, ArchUnit will forbid the should-part of rules to be evaluated against an empty set of classes.
+The reason is that this can lead to rules that by accident do not check any classes at all.
+Take for example
+
+[source,java,options="nowrap"]
+----
+classes().that().resideInAPackage("com.myapp.old").should()...
+----
+
+Now consider somebody renames the package `old` to `newer`.
+The rule will now always evaluate successfully without any reported error.
+However, it actually does not check any classes at all anymore.
+This is likely not what most users want.
+Thus, by default ArchUnit will fail checking the rule in this case.
+If you want to allow evaluating such rules,
+i.e. where the actual input to the should-conditionis empty,
+you can set the following property:
+
+[source,options="nowrap"]
+.archunit.properties
+----
+archRule.failOnEmptyShould=false
+----
+
 === Custom Error Messages
 
 You can configure a custom format to display the failures of a rule.