From d221fada906a4214a07bb8c5b8b32a489927ea1a Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Wed, 1 Jun 2022 10:54:23 +0200 Subject: [PATCH] Improve detection of JSR 305 annotations. --- .../edu/hm/hafner/util/ArchitectureRules.java | 74 +++++++++-------- .../hm/hafner/util/ArchitectureRulesTest.java | 79 ++++++++++++++++--- .../edu/hm/hafner/util/ArchitectureTest.java | 2 +- 3 files changed, 106 insertions(+), 49 deletions(-) diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java index 1da9bc56..7cca8a48 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java @@ -4,7 +4,6 @@ import java.util.Arrays; import java.util.List; -import org.apache.commons.lang3.StringUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -18,7 +17,9 @@ import com.tngtech.archunit.junit.ArchTest; import com.tngtech.archunit.lang.ArchRule; -import static com.tngtech.archunit.core.domain.JavaClass.Predicates.*; +import static com.tngtech.archunit.core.domain.JavaAccess.Predicates.*; +import static com.tngtech.archunit.lang.conditions.ArchConditions.*; +import static com.tngtech.archunit.lang.conditions.ArchPredicates.*; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.*; /** @@ -30,14 +31,16 @@ public final class ArchitectureRules { /** Never create exception without any context. */ public static final ArchRule NO_EXCEPTIONS_WITH_NO_ARG_CONSTRUCTOR = noClasses().that().haveSimpleNameNotContaining("Benchmark") - .should().callConstructorWhere(new ExceptionHasNoContext()); + .should().callConstructorWhere(exceptionHasNoContextAsParameter()) + .because("exceptions should include failure-capture information in detail messages (Effective Java Item 75)"); /** Junit 5 test classes should not be public. */ public static final ArchRule NO_PUBLIC_TEST_CLASSES = noClasses().that().haveSimpleNameEndingWith("Test") .and().haveSimpleNameNotContaining("_jmh") .and().doNotHaveModifier(JavaModifier.ABSTRACT) - .should().bePublic(); + .should().bePublic() + .because("test classes are not part of the API and should be hidden in a package"); /** Junit 5 test methods should not be public. */ public static final ArchRule ONLY_PACKAGE_PRIVATE_TEST_METHODS = @@ -45,45 +48,51 @@ public final class ArchitectureRules { .or().areAnnotatedWith(ParameterizedTest.class) .and().areDeclaredInClassesThat() .haveSimpleNameEndingWith("Test") - .should().bePackagePrivate(); + .should().bePackagePrivate() + .because("test methods are not part of the API and should be hidden in a package"); /** ArchUnit tests should not be public. */ - public static final ArchRule NO_PUBLIC_ARCHITECTURE_TESTS = + public static final ArchRule ONLY_PACKAGE_PRIVATE_ARCHITECTURE_TESTS = fields().that().areAnnotatedWith(ArchTest.class) - .should().notBePublic(); + .should().bePackagePrivate(); /** * Methods or constructors that are annotated with {@link VisibleForTesting} must not be called by other classes. * These methods are meant to be {@code private}. Only test classes are allowed to call these methods. */ public static final ArchRule NO_TEST_API_CALLED = - noClasses().that() - .haveSimpleNameNotEndingWith("Test").and().haveSimpleNameNotContaining("Benchmark") - .should().callCodeUnitWhere(new AccessRestrictedToTests()); + noClasses().that().haveSimpleNameNotEndingWith("Test") + .and().haveSimpleNameNotContaining("Benchmark") + .should().callCodeUnitWhere(accessIsRestrictedForTests()); /** Prevents that classes use visible but forbidden API. */ public static final ArchRule NO_FORBIDDEN_PACKAGE_ACCESSED = - noClasses().should().dependOnClassesThat(resideInAnyPackage( + noClasses().should().dependOnClassesThat().resideInAnyPackage( "org.apache.commons.lang..", "org.joda.time..", "javax.xml.bind..", "net.jcip.annotations..", - "javax.annotation..", "junit..", "org.hamcrest..", "com.google.common..", "org.junit" - )); + ); - /** Prevents that classes use visible but forbidden API. */ + /** Prevents that classes use visible but forbidden annotations. */ public static final ArchRule NO_FORBIDDEN_ANNOTATION_USED = - noClasses().should().dependOnClassesThat().haveSimpleNameEndingWith("Nullable"); + noClasses().should().dependOnClassesThat().haveNameMatching("javax.annotation.Check.*") + .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Nonnull") + .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Nullable") + .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Parameters.*") + .orShould().dependOnClassesThat().haveNameMatching("edu.umd.cs.findbugs.annotations.Nullable") // only CheckForNull and NonNull is allowed + .because("JSR 305 annotations are now part of edu.umd.cs.findbugs.annotations package"); /** Prevents that classes use visible but forbidden API. */ - public static final ArchRule NO_FORBIDDEN_CLASSES_CALLED - = noClasses() - .should().callCodeUnitWhere(new TargetIsForbiddenClass( - "org.junit.jupiter.api.Assertions", "org.junit.Assert")); + public static final ArchRule NO_FORBIDDEN_CLASSES_CALLED = + noClasses().should().callCodeUnitWhere(targetOwner(has( + fullyQualifiedName("org.junit.jupiter.api.Assertions") + .or(fullyQualifiedName("org.junit.Assert"))))) + .because("only AssertJ should be used for assertions"); /** Ensures that the {@code readResolve} methods are protected so subclasses can call the parent method. */ public static final ArchRule READ_RESOLVE_SHOULD_BE_PROTECTED = @@ -91,10 +100,18 @@ public final class ArchitectureRules { .should().beDeclaredInClassesThat().implement(Serializable.class) .andShould().beProtected().allowEmptyShould(true); + private static ExceptionHasNoContext exceptionHasNoContextAsParameter() { + return new ExceptionHasNoContext(); + } + private ArchitectureRules() { // prevents instantiation } + private static DescribedPredicate> accessIsRestrictedForTests() { + return new AccessRestrictedToTests(); + } + /** * Matches if a call from outside the defining class uses a method or constructor annotated with * {@link VisibleForTesting}. There are two exceptions: @@ -120,25 +137,6 @@ private boolean isVisibleForTesting(final CanBeAnnotated target) { } } - /** - * Matches if a code unit of one of the registered classes has been called. - */ - private static class TargetIsForbiddenClass extends DescribedPredicate> { - private final String[] classes; - - TargetIsForbiddenClass(final String... classes) { - super("forbidden class"); - - this.classes = Arrays.copyOf(classes, classes.length); - } - - @Override - public boolean apply(final JavaCall input) { - return StringUtils.containsAny(input.getTargetOwner().getFullName(), classes) - && !"assertTimeoutPreemptively".equals(input.getName()); - } - } - /** * Predicate to match exception constructor calls without contexts. */ diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java b/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java index a2106754..79b7cd8f 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java @@ -13,39 +13,98 @@ * @author Ullrich Hafner */ class ArchitectureRulesTest { + private static final String BROKEN_CLASS_NAME = ArchitectureRulesViolatedTest.class.getTypeName(); + + @Test + void shouldNotUseJsr305Annotations() { + assertThatExceptionOfType(AssertionError.class).isThrownBy( + () -> ArchitectureRules.NO_FORBIDDEN_ANNOTATION_USED.check( + importClasses(ArchitectureRulesViolatedTest.class))) + .withMessageContainingAll("was violated (4 times)", "edu.umd.cs.findbugs.annotations", + "Field is annotated with ", + "Field is annotated with ", + "Method is annotated with ", + "Parameter of method is annotated with "); + + assertThatNoException().isThrownBy( + () -> ArchitectureRules.NO_FORBIDDEN_ANNOTATION_USED.check(importPassingClass())); + } + + @Test + void shouldVerifyForbiddenAnnotations() { + assertThatExceptionOfType(AssertionError.class).isThrownBy( + () -> ArchitectureRules.NO_FORBIDDEN_CLASSES_CALLED.check(importBrokenClass())) + .withMessageContainingAll(BROKEN_CLASS_NAME, "only AssertJ should be used"); + + assertThatNoException().isThrownBy( + () -> ArchitectureRules.NO_FORBIDDEN_CLASSES_CALLED.check(importPassingClass())); + } + + @Test + void shouldVerifyExceptionWithNoArgConstructorCalled() { + assertThatExceptionOfType(AssertionError.class).isThrownBy( + () -> ArchitectureRules.NO_EXCEPTIONS_WITH_NO_ARG_CONSTRUCTOR.check(importBrokenClass())) + .withMessageContainingAll(BROKEN_CLASS_NAME, "(Effective Java Item 75)"); + + assertThatNoException().isThrownBy( + () -> ArchitectureRules.NO_EXCEPTIONS_WITH_NO_ARG_CONSTRUCTOR.check(importPassingClass())); + } + @Test void shouldVerifyNoPublicTestClassesRule() { - JavaClasses violatedClasses = importClasses(NoPublicTestElementsViolatedTest.class); assertThatExceptionOfType(AssertionError.class).isThrownBy( - () -> ArchitectureRules.NO_PUBLIC_TEST_CLASSES.check(violatedClasses)); - JavaClasses passedClasses = new ClassFileImporter().importClasses(NoPublicTestElementsPassedTest.class); + () -> ArchitectureRules.NO_PUBLIC_TEST_CLASSES.check(importBrokenClass())) + .withMessageContainingAll(BROKEN_CLASS_NAME, "test classes are not part of the API"); + assertThatNoException().isThrownBy( - () -> ArchitectureRules.NO_PUBLIC_TEST_CLASSES.check(passedClasses)); + () -> ArchitectureRules.NO_PUBLIC_TEST_CLASSES.check(importPassingClass())); } @Test void shouldVerifyNoPublicTestMethodsRule() { - JavaClasses violatedClasses = importClasses(NoPublicTestElementsViolatedTest.class); assertThatExceptionOfType(AssertionError.class).isThrownBy( - () -> ArchitectureRules.ONLY_PACKAGE_PRIVATE_TEST_METHODS.check(violatedClasses)); - JavaClasses passedClasses = new ClassFileImporter().importClasses(NoPublicTestElementsPassedTest.class); + () -> ArchitectureRules.ONLY_PACKAGE_PRIVATE_TEST_METHODS.check(importBrokenClass())) + .withMessageContainingAll(BROKEN_CLASS_NAME, "test methods are not part of the API"); + assertThatNoException().isThrownBy( - () -> ArchitectureRules.ONLY_PACKAGE_PRIVATE_TEST_METHODS.check(passedClasses)); + () -> ArchitectureRules.ONLY_PACKAGE_PRIVATE_TEST_METHODS.check(importPassingClass())); + } + + private JavaClasses importPassingClass() { + return new ClassFileImporter().importClasses(ArchitectureRulesPassedTest.class); + } + + private JavaClasses importBrokenClass() { + return importClasses(ArchitectureRulesViolatedTest.class); } private JavaClasses importClasses(final Class... classes) { return new ClassFileImporter().importClasses(classes); } - public static class NoPublicTestElementsViolatedTest { + public static class ArchitectureRulesViolatedTest { + @javax.annotation.Nonnull + private final String empty = ""; + @edu.umd.cs.findbugs.annotations.Nullable + private final String noNullable = null; + @Test public void shouldFail() { + org.junit.jupiter.api.Assertions.assertEquals(1, 1); + + throw new IllegalArgumentException(); + } + + @javax.annotation.CheckForNull + protected String method(@javax.annotation.Nonnull String param) { + return null; } } - static class NoPublicTestElementsPassedTest { + static class ArchitectureRulesPassedTest { @Test void shouldPass() { + throw new IllegalArgumentException("context"); } } } diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureTest.java b/src/test/java/edu/hm/hafner/util/ArchitectureTest.java index 3abfa2a4..fadacbaf 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureTest.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureTest.java @@ -23,7 +23,7 @@ class ArchitectureTest { static final ArchRule ONLY_PACKAGE_PRIVATE_TEST_METHODS = ArchitectureRules.ONLY_PACKAGE_PRIVATE_TEST_METHODS; @ArchTest - static final ArchRule NO_PUBLIC_ARCHITECTURE_TESTS = ArchitectureRules.NO_PUBLIC_ARCHITECTURE_TESTS; + static final ArchRule ONLY_PACKAGE_PRIVATE_ARCHITECTURE_TESTS = ArchitectureRules.ONLY_PACKAGE_PRIVATE_ARCHITECTURE_TESTS; @ArchTest static final ArchRule NO_TEST_API_CALLED = ArchitectureRules.NO_TEST_API_CALLED;