Skip to content

Commit

Permalink
Improve detection of JSR 305 annotations.
Browse files Browse the repository at this point in the history
  • Loading branch information
@uhafner
uhafner committed Jun 1, 2022
1 parent d896914 commit d221fad
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 49 deletions.
74 changes: 36 additions & 38 deletions src/test/java/edu/hm/hafner/util/ArchitectureRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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.*;

/**
Expand All @@ -30,71 +31,87 @@ 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 =
methods().that().areAnnotatedWith(Test.class)
.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 =
methods().that().haveName("readResolve").and().haveRawReturnType(Object.class)
.should().beDeclaredInClassesThat().implement(Serializable.class)
.andShould().beProtected().allowEmptyShould(true);

private static ExceptionHasNoContext exceptionHasNoContextAsParameter() {
return new ExceptionHasNoContext();
}

private ArchitectureRules() {
// prevents instantiation
}

private static DescribedPredicate<? super JavaCall<?>> 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:
Expand All @@ -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<JavaCall<?>> {
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.
*/
Expand Down
79 changes: 69 additions & 10 deletions src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.empty> is annotated with <javax.annotation.Nonnull>",
"Field <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.noNullable> is annotated with <edu.umd.cs.findbugs.annotations.Nullable>",
"Method <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.method(java.lang.String)> is annotated with <javax.annotation.CheckForNull>",
"Parameter <java.lang.String> of method <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.method(java.lang.String)> is annotated with <javax.annotation.Nonnull>");

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");
}
}
}
2 changes: 1 addition & 1 deletion src/test/java/edu/hm/hafner/util/ArchitectureTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit d221fad

Please sign in to comment.