From d7a928b743974e18e0f08f5b723fd6fd3d46c46c Mon Sep 17 00:00:00 2001 From: Hannes Oberprantacher Date: Sat, 26 Feb 2022 21:17:58 +0100 Subject: [PATCH] add method `ArchRule.allowEmptyShould(..)` This will allow to override the configuration property `archRule.failOnEmptyShould` on a per-rule basis. We overlooked the use case for certain rules that simply expect to have no match when we introduced `failOnEmptyShould`. An example would be rules like `classes().that(areUnwanted()).should().containNumberOfElements(equalTo(0))`. So making this only configurable on a global level renders it useless for a certain set of users. Also, we broke `optionalLayers` in `LayeredArchitecture` and `OnionArchitecture` with the default configuration, since individual access checks are modelled as rules. `LayeredArchitecture` and `OnionArchitecture` already have the concept of `optionalLayers` which is pretty much the same as expressed by `allowEmptyShould`. We now simply translate `allowEmptyShould` to the existing concept, otherwise the two concepts clash (e.g. declare an `optionalLayer` for which the allowed accesses are internally modelled as separate rule, and it still rejects empty classes if `failOnEmptyShould` is set to `true).` Signed-off-by: Hannes Oberprantacher Signed-off-by: Peter Gafert (cherry picked from commit 4a3f53e96fcda893882af8c9dc2c209ff017ccaf) --- .../archunit/lang/AllowEmptyShould.java | 48 ++++++++++++ .../com/tngtech/archunit/lang/ArchRule.java | 47 +++++++---- .../archunit/lang/CompositeArchRule.java | 10 +++ .../lang/syntax/ObjectsShouldInternal.java | 5 ++ .../archunit/library/Architectures.java | 77 ++++++++++++------- .../library/dependencies/SliceRule.java | 38 +++++++-- .../library/freeze/FreezingArchRule.java | 6 ++ .../tngtech/archunit/lang/ArchRuleTest.java | 42 +++++++--- .../archunit/lang/CompositeArchRuleTest.java | 46 ++++++++++- .../syntax/elements/ClassesShouldTest.java | 33 ++++++++ .../syntax/elements/MembersShouldTest.java | 38 +++++++++ .../archunit/library/ArchitecturesTest.java | 12 +-- .../library/dependencies/SliceRuleTest.java | 31 ++++++++ .../userguide/010_Advanced_Configuration.adoc | 19 ++++- 14 files changed, 380 insertions(+), 72 deletions(-) create mode 100644 archunit/src/main/java/com/tngtech/archunit/lang/AllowEmptyShould.java diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/AllowEmptyShould.java b/archunit/src/main/java/com/tngtech/archunit/lang/AllowEmptyShould.java new file mode 100644 index 0000000000..e523d4e52b --- /dev/null +++ b/archunit/src/main/java/com/tngtech/archunit/lang/AllowEmptyShould.java @@ -0,0 +1,48 @@ +/* + * Copyright 2014-2022 TNG Technology Consulting GmbH + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.tngtech.archunit.lang; + +import com.tngtech.archunit.ArchConfiguration; + +enum AllowEmptyShould { + TRUE { + @Override + public boolean isAllowed() { + return true; + } + }, + FALSE { + @Override + public boolean isAllowed() { + return false; + } + }, + AS_CONFIGURED { + @Override + public boolean isAllowed() { + return ArchConfiguration.get().getPropertyOrDefault(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME, TRUE.toString()) + .equalsIgnoreCase(FALSE.toString()); + } + }; + + private static final String FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME = "archRule.failOnEmptyShould"; + + abstract boolean isAllowed(); + + static AllowEmptyShould fromBoolean(boolean allow) { + return allow ? TRUE : FALSE; + } +} diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java b/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java index 849bf60018..ade2990131 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java @@ -23,7 +23,6 @@ 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; @@ -41,7 +40,6 @@ 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; /** @@ -68,6 +66,18 @@ public interface ArchRule extends CanBeEvaluated, CanOverrideDescription + * Note that this method will override the configuration property {@code archRule.failOnEmptyShould}. + * + * @param allowEmptyShould Whether the rule fails if the should-clause is evaluated with an empty set of elements + * @return A (new) {@link ArchRule} with adjusted {@code allowEmptyShould} behavior + */ + @PublicAPI(usage = ACCESS) + ArchRule allowEmptyShould(boolean allowEmptyShould); + @PublicAPI(usage = ACCESS) final class Assertions { private static final ArchUnitExtensions extensions = new ArchUnitExtensions(); @@ -166,7 +176,7 @@ public EvaluationResult getResult() { @Internal class Factory { public static ArchRule create(final ClassesTransformer classesTransformer, final ArchCondition condition, final Priority priority) { - return new SimpleArchRule<>(priority, classesTransformer, condition, Optional.empty()); + return new SimpleArchRule<>(priority, classesTransformer, condition, Optional.empty(), AllowEmptyShould.AS_CONFIGURED); } public static ArchRule withBecause(ArchRule rule, String reason) { @@ -184,18 +194,20 @@ private static class SimpleArchRule implements ArchRule { private final ClassesTransformer classesTransformer; private final ArchCondition condition; private final Optional overriddenDescription; + private final AllowEmptyShould allowEmptyShould; private SimpleArchRule(Priority priority, ClassesTransformer classesTransformer, ArchCondition condition, - Optional overriddenDescription) { + Optional overriddenDescription, AllowEmptyShould allowEmptyShould) { this.priority = priority; this.classesTransformer = classesTransformer; this.condition = condition; this.overriddenDescription = overriddenDescription; + this.allowEmptyShould = allowEmptyShould; } @Override public ArchRule as(String newDescription) { - return new SimpleArchRule<>(priority, classesTransformer, condition, Optional.of(newDescription)); + return new SimpleArchRule<>(priority, classesTransformer, condition, Optional.of(newDescription), allowEmptyShould); } @Override @@ -208,6 +220,11 @@ public ArchRule because(String reason) { return withBecause(this, reason); } + @Override + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return new SimpleArchRule<>(priority, classesTransformer, condition, overriddenDescription, AllowEmptyShould.fromBoolean(allowEmptyShould)); + } + @Override public EvaluationResult evaluate(JavaClasses classes) { Iterable allObjects = classesTransformer.transform(classes); @@ -223,20 +240,18 @@ public EvaluationResult evaluate(JavaClasses classes) { } private void verifyNoEmptyShouldIfEnabled(Iterable 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); + if (isEmpty(allObjects) && !allowEmptyShould.isAllowed()) { + throw new AssertionError(String.format( + "Rule '%s' 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 either " + + "use `%s.allowEmptyShould(true)` on a single rule or set the configuration property `%s = false` " + + "to change the behavior globally.", + getDescription(), ArchRule.class.getSimpleName(), FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME)); } } - 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() ? diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/CompositeArchRule.java b/archunit/src/main/java/com/tngtech/archunit/lang/CompositeArchRule.java index 5a48a15e41..56395b26cc 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/CompositeArchRule.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/CompositeArchRule.java @@ -82,6 +82,16 @@ public CompositeArchRule because(String reason) { return new CompositeArchRule(priority, rules, createBecauseDescription(this, reason)); } + @Override + @PublicAPI(usage = ACCESS) + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + ImmutableList.Builder rulesWithOverriddenAllowEmptyShould = ImmutableList.builder(); + for (ArchRule rule : rules) { + rulesWithOverriddenAllowEmptyShould.add(rule.allowEmptyShould(allowEmptyShould)); + } + return new CompositeArchRule(priority, rulesWithOverriddenAllowEmptyShould.build(), description); + } + @Override @PublicAPI(usage = ACCESS) public EvaluationResult evaluate(JavaClasses classes) { diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/ObjectsShouldInternal.java b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/ObjectsShouldInternal.java index f449c04296..57fb96df98 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/ObjectsShouldInternal.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/ObjectsShouldInternal.java @@ -86,6 +86,11 @@ public ArchRule because(String reason) { return ArchRule.Factory.withBecause(this, reason); } + @Override + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return finishedRule.get().allowEmptyShould(allowEmptyShould); + } + @Override public ArchRule as(String newDescription) { return finishedRule.get().as(newDescription); diff --git a/archunit/src/main/java/com/tngtech/archunit/library/Architectures.java b/archunit/src/main/java/com/tngtech/archunit/library/Architectures.java index a01709ce3f..d430efb02a 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/Architectures.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/Architectures.java @@ -222,6 +222,8 @@ private void checkEmptyLayers(JavaClasses classes, EvaluationResult result) { private EvaluationResult evaluateLayersShouldNotBeEmpty(JavaClasses classes, LayerDefinition layerDefinition) { return classes().that(layerDefinitions.containsPredicateFor(layerDefinition.name)) .should(notBeEmptyFor(layerDefinition)) + // we need to set `allowEmptyShould(true)` to allow the layer not empty check to be evaluated. This will provide a nicer error message. + .allowEmptyShould(true) .evaluate(classes); } @@ -231,6 +233,7 @@ private EvaluationResult evaluateDependenciesShouldBeSatisfied(JavaClasses class : onlyHaveDependenciesWhere(targetMatchesIfDependencyIsRelevant(specification.layerName, specification.allowedLayers)); return classes().that(layerDefinitions.containsPredicateFor(specification.layerName)) .should(satisfyLayerDependenciesCondition) + .allowEmptyShould(true) .evaluate(classes); } @@ -256,32 +259,6 @@ private DescribedPredicate ifDependencyIsRelevant(DescribedPredicate originPackageMatches; } - private static ArchCondition notBeEmptyFor(final LayeredArchitecture.LayerDefinition layerDefinition) { - return new LayerShouldNotBeEmptyCondition(layerDefinition); - } - - private static class LayerShouldNotBeEmptyCondition extends ArchCondition { - private final LayeredArchitecture.LayerDefinition layerDefinition; - private boolean empty = true; - - LayerShouldNotBeEmptyCondition(final LayeredArchitecture.LayerDefinition layerDefinition) { - super("not be empty"); - this.layerDefinition = layerDefinition; - } - - @Override - public void check(JavaClass item, ConditionEvents events) { - empty = false; - } - - @Override - public void finish(ConditionEvents events) { - if (empty) { - events.add(violated(layerDefinition, String.format("Layer '%s' is empty", layerDefinition.name))); - } - } - } - @Override @PublicAPI(usage = ACCESS) public void check(JavaClasses classes) { @@ -294,6 +271,15 @@ public ArchRule because(String reason) { return ArchRule.Factory.withBecause(this, reason); } + /** + * This method is equivalent to calling {@link #withOptionalLayers(boolean)}, which should be preferred in this context + * as the meaning is easier to understand. + */ + @Override + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return withOptionalLayers(allowEmptyShould); + } + @Override @PublicAPI(usage = ACCESS) public LayeredArchitecture as(String newDescription) { @@ -352,6 +338,32 @@ private void checkLayerNamesExist(String... layerNames) { } } + private static ArchCondition notBeEmptyFor(final LayeredArchitecture.LayerDefinition layerDefinition) { + return new LayerShouldNotBeEmptyCondition(layerDefinition); + } + + private static class LayerShouldNotBeEmptyCondition extends ArchCondition { + private final LayeredArchitecture.LayerDefinition layerDefinition; + private boolean empty = true; + + LayerShouldNotBeEmptyCondition(final LayeredArchitecture.LayerDefinition layerDefinition) { + super("not be empty"); + this.layerDefinition = layerDefinition; + } + + @Override + public void check(JavaClass item, ConditionEvents events) { + empty = false; + } + + @Override + public void finish(ConditionEvents events) { + if (empty) { + events.add(violated(layerDefinition, String.format("Layer '%s' is empty", layerDefinition.name))); + } + } + } + private static final class LayerDefinitions implements Iterable { private final Map layerDefinitions = new LinkedHashMap<>(); @@ -570,6 +582,10 @@ public OnionArchitecture adapter(String name, String... packageIdentifiers) { return this; } + /** + * @param optionalLayers Whether the different parts of the Onion Architecture (domain models, domain services, ...) should be allowed to be empty. + * If set to {@code false} the {@link OnionArchitecture OnionArchitecture} will fail if any such layer does not contain any class. + */ @PublicAPI(usage = ACCESS) public OnionArchitecture withOptionalLayers(boolean optionalLayers) { this.optionalLayers = optionalLayers; @@ -637,6 +653,15 @@ public ArchRule because(String reason) { return ArchRule.Factory.withBecause(this, reason); } + /** + * This method is equivalent to calling {@link #withOptionalLayers(boolean)}, which should be preferred in this context + * as the meaning is easier to understand. + */ + @Override + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return withOptionalLayers(allowEmptyShould); + } + @Override public OnionArchitecture as(String newDescription) { return new OnionArchitecture(domainModelPackageIdentifiers, domainServicePackageIdentifiers, diff --git a/archunit/src/main/java/com/tngtech/archunit/library/dependencies/SliceRule.java b/archunit/src/main/java/com/tngtech/archunit/library/dependencies/SliceRule.java index 91f5c714f7..d35a55351f 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/dependencies/SliceRule.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/dependencies/SliceRule.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.tngtech.archunit.PublicAPI; import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.base.Optional; import com.tngtech.archunit.core.domain.Dependency; import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaClasses; @@ -44,18 +45,32 @@ public final class SliceRule implements ArchRule { private final List transformations; private final DescribedPredicate ignoreDependency; private final ConditionFactory conditionFactory; + private final Optional allowEmptyShould; SliceRule(Slices.Transformer inputTransformer, Priority priority, ConditionFactory conditionFactory) { - this(inputTransformer, priority, Collections.emptyList(), DescribedPredicate.alwaysFalse(), conditionFactory); + this( + inputTransformer, + priority, + Collections.emptyList(), + DescribedPredicate.alwaysFalse(), + conditionFactory, + Optional.empty()); } - private SliceRule(Slices.Transformer inputTransformer, Priority priority, List transformations, - DescribedPredicate ignoreDependency, ConditionFactory conditionFactory) { + private SliceRule( + Slices.Transformer inputTransformer, + Priority priority, + List transformations, + DescribedPredicate ignoreDependency, + ConditionFactory conditionFactory, + Optional allowEmptyShould + ) { this.inputTransformer = inputTransformer; this.priority = priority; this.transformations = transformations; this.ignoreDependency = ignoreDependency; this.conditionFactory = conditionFactory; + this.allowEmptyShould = allowEmptyShould; } @Override @@ -70,6 +85,12 @@ public SliceRule because(String reason) { return copyWithTransformation(new Because(reason)); } + @Override + @PublicAPI(usage = ACCESS) + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return new SliceRule(inputTransformer, priority, transformations, ignoreDependency, conditionFactory, Optional.of(allowEmptyShould)); + } + @Override @PublicAPI(usage = ACCESS) public EvaluationResult evaluate(JavaClasses classes) { @@ -100,17 +121,22 @@ public SliceRule ignoreDependency(String origin, String target) { @PublicAPI(usage = ACCESS) public SliceRule ignoreDependency(DescribedPredicate origin, DescribedPredicate target) { - return new SliceRule(inputTransformer, priority, transformations, ignoreDependency.or(dependency(origin, target)), conditionFactory); + return new SliceRule(inputTransformer, priority, transformations, ignoreDependency.or(dependency(origin, target)), conditionFactory, allowEmptyShould); } private SliceRule copyWithTransformation(Transformation transformation) { List newTransformations = ImmutableList.builder().addAll(transformations).add(transformation).build(); - return new SliceRule(inputTransformer, priority, newTransformations, ignoreDependency, conditionFactory); + return new SliceRule(inputTransformer, priority, newTransformations, ignoreDependency, conditionFactory, allowEmptyShould); } private ArchRule getArchRule() { - ArchRule rule = priority(priority).all(inputTransformer).should(conditionFactory.create(inputTransformer, not(ignoreDependency))); + ArchRule rule = priority(priority) + .all(inputTransformer) + .should(conditionFactory.create(inputTransformer, not(ignoreDependency))); + if (allowEmptyShould.isPresent()) { + rule = rule.allowEmptyShould(allowEmptyShould.get()); + } for (Transformation transformation : transformations) { rule = transformation.apply(rule); } diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java index 609ba4649f..ad382137fb 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java @@ -102,6 +102,12 @@ public FreezingArchRule because(String reason) { return new FreezingArchRule(delegate.because(reason), store, matcher); } + @Override + @PublicAPI(usage = ACCESS) + public ArchRule allowEmptyShould(boolean allowEmptyShould) { + return new FreezingArchRule(delegate.allowEmptyShould(allowEmptyShould), store, matcher); + } + @Override @PublicAPI(usage = ACCESS) public FreezingArchRule as(String newDescription) { diff --git a/archunit/src/test/java/com/tngtech/archunit/lang/ArchRuleTest.java b/archunit/src/test/java/com/tngtech/archunit/lang/ArchRuleTest.java index b4a8ddf8f8..05bb37c0f0 100644 --- a/archunit/src/test/java/com/tngtech/archunit/lang/ArchRuleTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/lang/ArchRuleTest.java @@ -10,6 +10,7 @@ import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.io.Files; +import com.tngtech.archunit.base.DescribedPredicate; import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaClasses; import com.tngtech.archunit.core.domain.JavaClassesTest; @@ -167,28 +168,47 @@ public void finish(ConditionEvents events) { } @Test - public void evaluation_fails_because_of_empty_set_of_classes_with_default_fail_on_empty_should() { + public void evaluation_fails_because_of_empty_set_of_elements_with_default_fail_on_empty_should() { thrown.expect(AssertionError.class); - thrown.expectMessage("Rule failed to check any classes"); + thrown.expectMessage("failed to check any classes"); thrown.expectMessage(FAIL_ON_EMPTY_SHOULD_PROPERTY_NAME); - classes().should(ALWAYS_BE_VALID).evaluate(importClasses()); + createPassingArchRule().evaluate(importEmptyClasses()); } @Test - public void evaluation_fails_because_of_empty_set_of_classes_after_filter_with_default_fail_on_empty_should() { + public void evaluation_fails_because_of_empty_set_of_elements_after_that_clause_with_default_fail_on_empty_should() { thrown.expect(AssertionError.class); - thrown.expectMessage("Rule failed to check any classes"); + thrown.expectMessage("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)); + createPassingArchRule(strings().that(DescribedPredicate.alwaysFalse())).evaluate(importClasses(SomeClass.class)); } @Test - public void evaluation_passes_on_empty_set_of_classes_with_deactivated_fail_on_empty_should() { + public void evaluation_passes_on_empty_set_of_elements_with_deactivated_fail_on_empty_should_by_configuration() { archConfigurationRule.setFailOnEmptyShould(false); - classes().should(ALWAYS_BE_VALID).evaluate(importClasses()); + createPassingArchRule().evaluate(importEmptyClasses()); + } + + @Test + public void evaluation_passes_on_empty_set_of_elements_with_activated_fail_on_empty_should_by_configuration_but_overridden_by_rule() { + archConfigurationRule.setFailOnEmptyShould(true); + + createPassingArchRule().allowEmptyShould(true).evaluate(importEmptyClasses()); + } + + private JavaClasses importEmptyClasses() { + return importClasses(); + } + + private ArchRule createPassingArchRule() { + return createPassingArchRule(strings()); + } + + private ArchRule createPassingArchRule(ClassesTransformer classesTransformer) { + return ArchRule.Factory.create(classesTransformer, ALWAYS_BE_VALID.forSubtype(), Priority.MEDIUM); } private ClassesTransformer strings() { @@ -282,10 +302,10 @@ public void check(JavaClass item, ConditionEvents events) { } }; - private static final ArchCondition ALWAYS_BE_VALID = - new ArchCondition("always be valid") { + private static final ArchCondition ALWAYS_BE_VALID = + new ArchCondition("always be valid") { @Override - public void check(JavaClass item, ConditionEvents events) { + public void check(Object item, ConditionEvents events) { } }; diff --git a/archunit/src/test/java/com/tngtech/archunit/lang/CompositeArchRuleTest.java b/archunit/src/test/java/com/tngtech/archunit/lang/CompositeArchRuleTest.java index 849c08dad5..68ca4d2d94 100644 --- a/archunit/src/test/java/com/tngtech/archunit/lang/CompositeArchRuleTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/lang/CompositeArchRuleTest.java @@ -1,16 +1,21 @@ package com.tngtech.archunit.lang; +import java.util.List; + import com.google.common.collect.ImmutableList; +import com.tngtech.archunit.base.DescribedPredicate; import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.testutil.ArchConfigurationRule; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import java.util.List; - import static com.tngtech.archunit.core.domain.TestUtils.importClasses; import static com.tngtech.archunit.lang.Priority.HIGH; import static com.tngtech.archunit.lang.Priority.MEDIUM; @@ -18,12 +23,16 @@ import static com.tngtech.java.junit.dataprovider.DataProviders.$; import static com.tngtech.java.junit.dataprovider.DataProviders.$$; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; @RunWith(DataProviderRunner.class) public class CompositeArchRuleTest { private static final boolean SATISFIED = true; private static final boolean UNSATISFIED = false; + @Rule + public final ArchConfigurationRule archConfigurationRule = new ArchConfigurationRule(); + @DataProvider public static Object[][] rules_to_AND() { return $$( @@ -83,6 +92,37 @@ public void priority_is_passed() { assertPriority(failureMessage, priority); } + @Test + public void fails_on_empty_should_by_default() { + assertThatThrownBy(new ThrowingCallable() { + @Override + public void call() { + compositeRuleWithPartialEmptyShould().check(new ClassFileImporter().importClasses(Object.class)); + } + }).isInstanceOf(AssertionError.class) + .hasMessageContaining("failed to check any classes"); + } + + @Test + public void should_allow_empty_should_if_configured() { + archConfigurationRule.setFailOnEmptyShould(false); + + compositeRuleWithPartialEmptyShould().check(new ClassFileImporter().importClasses(Object.class)); + } + + @Test + public void allows_empty_should_if_overridden_by_rule() { + archConfigurationRule.setFailOnEmptyShould(true); + + compositeRuleWithPartialEmptyShould().allowEmptyShould(true).check(new ClassFileImporter().importClasses(Object.class)); + } + + private static CompositeArchRule compositeRuleWithPartialEmptyShould() { + return CompositeArchRule + .of(classes().should().bePublic()) + .and(classes().that(DescribedPredicate.alwaysFalse()).should().bePublic()); + } + private void assertPriority(String failureMessage, Priority priority) { assertThat(failureMessage).contains(String.format("[Priority: %s]", priority)); } @@ -108,4 +148,4 @@ public void check(JavaClass item, ConditionEvents events) { } }, Priority.MEDIUM).as(String.format("%s rule", satisfied ? "satisfied" : "failing")); } -} \ No newline at end of file +} diff --git a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldTest.java b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldTest.java index 427e2de7c5..439f564474 100644 --- a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldTest.java @@ -19,12 +19,14 @@ import com.tngtech.archunit.core.domain.JavaAccess; import com.tngtech.archunit.core.domain.JavaAnnotation; import com.tngtech.archunit.core.domain.JavaCall; +import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaModifier; import com.tngtech.archunit.core.domain.properties.CanBeAnnotatedTest; import com.tngtech.archunit.core.domain.properties.CanBeAnnotatedTest.ClassRetentionAnnotation; import com.tngtech.archunit.core.domain.properties.CanBeAnnotatedTest.DefaultClassRetentionAnnotation; import com.tngtech.archunit.core.domain.properties.CanBeAnnotatedTest.RuntimeRetentionAnnotation; import com.tngtech.archunit.core.domain.properties.CanBeAnnotatedTest.SourceRetentionAnnotation; +import com.tngtech.archunit.core.importer.ClassFileImporter; import com.tngtech.archunit.lang.ArchCondition; import com.tngtech.archunit.lang.ArchRule; import com.tngtech.archunit.lang.EvaluationResult; @@ -35,6 +37,7 @@ import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -72,6 +75,7 @@ import static com.tngtech.java.junit.dataprovider.DataProviders.$$; import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach; import static java.util.regex.Pattern.quote; +import static org.assertj.core.api.Assertions.assertThatThrownBy; @RunWith(DataProviderRunner.class) public class ClassesShouldTest { @@ -1767,6 +1771,35 @@ public void onlyCall_should_report_success_if_targets_are_non_resolvable(ArchRul assertThatRule(rule).checking(importClasses(classCallingUnresolvableTarget)).hasNoViolation(); } + @Test + public void should_fail_on_empty_should_by_default() { + assertThatThrownBy(new ThrowingCallable() { + @Override + public void call() { + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + }).isInstanceOf(AssertionError.class) + .hasMessageContaining("failed to check any classes"); + } + + @Test + public void should_allow_empty_should_if_configured() { + configurationRule.setFailOnEmptyShould(false); + + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + + @Test + public void should_allow_empty_should_if_overridden_by_rule() { + configurationRule.setFailOnEmptyShould(true); + + ruleWithEmptyShould().allowEmptyShould(true).check(new ClassFileImporter().importClasses(getClass())); + } + + private static ArchRule ruleWithEmptyShould() { + return classes().that(DescribedPredicate.alwaysFalse()).should().bePublic(); + } + static String locationPattern(Class clazz) { return String.format("\\(%s.java:\\d+\\)", quote(clazz.getSimpleName())); } diff --git a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/MembersShouldTest.java b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/MembersShouldTest.java index 14d7bf08f2..52a32fbaac 100644 --- a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/MembersShouldTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/MembersShouldTest.java @@ -10,6 +10,8 @@ import com.google.common.collect.ImmutableSet; import com.tngtech.archunit.base.DescribedPredicate; import com.tngtech.archunit.base.Function; +import com.tngtech.archunit.core.domain.JavaMember; +import com.tngtech.archunit.core.importer.ClassFileImporter; import com.tngtech.archunit.lang.ArchRule; import com.tngtech.archunit.lang.EvaluationResult; import com.tngtech.archunit.lang.conditions.ArchConditions; @@ -20,9 +22,12 @@ import com.tngtech.archunit.lang.syntax.elements.GivenMembersTest.MetaAnnotation; import com.tngtech.archunit.lang.syntax.elements.GivenMembersTest.OtherClassWithMembers; import com.tngtech.archunit.lang.syntax.elements.testclasses.SimpleFieldAndMethod; +import com.tngtech.archunit.testutil.ArchConfigurationRule; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import org.assertj.core.api.ThrowableAssert; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -88,10 +93,14 @@ import static java.util.Collections.emptySet; import static java.util.regex.Pattern.quote; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; @RunWith(DataProviderRunner.class) public class MembersShouldTest { + @Rule + public final ArchConfigurationRule configurationRule = new ArchConfigurationRule(); + @Test public void complex_members_syntax() { EvaluationResult result = members() @@ -672,6 +681,35 @@ public void containNumberOfElements_fails_on_mismatching_predicate() { + "com.tngtech.archunit.lang.syntax.elements.testclasses.SimpleFieldAndMethod.violated()]"); } + @Test + public void should_fail_on_empty_should_by_default() { + assertThatThrownBy(new ThrowableAssert.ThrowingCallable() { + @Override + public void call() { + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + }).isInstanceOf(AssertionError.class) + .hasMessageContaining("failed to check any classes"); + } + + @Test + public void should_allow_empty_should_if_configured() { + configurationRule.setFailOnEmptyShould(false); + + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + + @Test + public void should_allow_empty_should_if_overridden_by_rule() { + configurationRule.setFailOnEmptyShould(true); + + ruleWithEmptyShould().allowEmptyShould(true).check(new ClassFileImporter().importClasses(getClass())); + } + + private static ArchRule ruleWithEmptyShould() { + return members().that(DescribedPredicate.alwaysFalse()).should().bePublic(); + } + private Set parseMembers(List details) { return parseMembers(ImmutableList.of(ClassWithVariousMembers.class, OtherClassWithMembers.class), details); } diff --git a/archunit/src/test/java/com/tngtech/archunit/library/ArchitecturesTest.java b/archunit/src/test/java/com/tngtech/archunit/library/ArchitecturesTest.java index 71e80bbad7..95328100bc 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/ArchitecturesTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/ArchitecturesTest.java @@ -29,7 +29,6 @@ import com.tngtech.archunit.library.testclasses.second.three.any.SecondThreeAnyClass; import com.tngtech.archunit.library.testclasses.some.pkg.SomePkgClass; import com.tngtech.archunit.library.testclasses.some.pkg.sub.SomePkgSubclass; -import com.tngtech.archunit.testutil.ArchConfigurationRule; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.DataProviders; @@ -48,6 +47,7 @@ import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name; import static com.tngtech.archunit.library.Architectures.layeredArchitecture; import static com.tngtech.archunit.library.Architectures.onionArchitecture; +import static com.tngtech.archunit.testutil.Assertions.assertThatRule; import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach; import static java.beans.Introspector.decapitalize; import static java.lang.System.lineSeparator; @@ -61,9 +61,6 @@ public class ArchitecturesTest { @Rule public final ExpectedException thrown = ExpectedException.none(); - @Rule - public final ArchConfigurationRule archConfigurationRule = new ArchConfigurationRule().setFailOnEmptyShould(false); - @DataProvider public static Object[][] layeredArchitectureDefinitions() { return testForEach( @@ -162,9 +159,7 @@ public void layered_architecture_allows_empty_layers_if_all_layers_are_optional( JavaClasses classes = new ClassFileImporter().importPackages(absolute("")); - EvaluationResult result = architecture.evaluate(classes); - assertThat(result.hasViolation()).as("result of evaluating empty layers has violation").isFalse(); - assertThat(result.getFailureReport().isEmpty()).as("failure report").isTrue(); + assertThatRule(architecture).checking(classes).hasNoViolation(); } @Test @@ -181,7 +176,8 @@ private LayeredArchitecture aLayeredArchitectureWithEmptyLayers() { return layeredArchitecture() .layer("Some").definedBy(absolute("should.not.be.found..")) .layer("Other").definedBy(absolute("also.not.found")) - .layer("Okay").definedBy("..testclasses.."); + .layer("Okay").definedBy("..testclasses..") + .whereLayer("Other").mayOnlyBeAccessedByLayers("Some"); } private void assertFailureLayeredArchitectureWithEmptyLayers(EvaluationResult result) { diff --git a/archunit/src/test/java/com/tngtech/archunit/library/dependencies/SliceRuleTest.java b/archunit/src/test/java/com/tngtech/archunit/library/dependencies/SliceRuleTest.java index 7be30e5521..9debb606b6 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/dependencies/SliceRuleTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/dependencies/SliceRuleTest.java @@ -16,6 +16,7 @@ import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; import org.assertj.core.api.Condition; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -28,6 +29,7 @@ import static com.tngtech.java.junit.dataprovider.DataProviders.$$; import static java.lang.System.lineSeparator; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; @RunWith(DataProviderRunner.class) public class SliceRuleTest { @@ -147,6 +149,35 @@ public void limits_number_of_reported_dependencies_per_edge_by_default_to_20() { "Dependencies of Slice threedependencies")); } + @Test + public void forbids_empty_should_by_default() { + assertThatThrownBy(new ThrowingCallable() { + @Override + public void call() { + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + }).isInstanceOf(AssertionError.class) + .hasMessageContaining("failed to check any classes"); + } + + @Test + public void should_allow_empty_should_if_configured() { + configurationRule.setFailOnEmptyShould(false); + + ruleWithEmptyShould().check(new ClassFileImporter().importClasses(getClass())); + } + + @Test + public void allows_empty_should_if_overridden() { + configurationRule.setFailOnEmptyShould(true); + + ruleWithEmptyShould().allowEmptyShould(true).check(new ClassFileImporter().importClasses(getClass())); + } + + private static SliceRule ruleWithEmptyShould() { + return slices().matching("nothing_because_there_is_no_capture_group").should().beFreeOfCycles(); + } + private List filterLinesMatching(String text, final String regex) { return FluentIterable.from(Splitter.on(lineSeparator()).split(text)) .filter(new Predicate() { diff --git a/docs/userguide/010_Advanced_Configuration.adoc b/docs/userguide/010_Advanced_Configuration.adoc index fdeb839fed..b626d7fbf4 100644 --- a/docs/userguide/010_Advanced_Configuration.adoc +++ b/docs/userguide/010_Advanced_Configuration.adoc @@ -158,8 +158,23 @@ 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: +i.e. where the actual input to the should-clause is empty, +you can use one of the following ways: + +*Allow Empty Should on a Per-Rule Basis* + +On each `ArchRule` you can use the method `ArchRule.allowEmptyShould(..)` to override the behavior +for a single rule, e.g. + +[source,java,options="nowrap"] +---- +// create a rule that allows that no classes are passed to the should-clause +classes().that()...should()...allowEmptyShould(true) +---- + +*Allow Empty Should Globally* + +To allow all rules to be evaluated without checking any classes you can set the following property: [source,options="nowrap"] .archunit.properties