From ccec9cb88bc609d97be60add36bf954f4cb09114 Mon Sep 17 00:00:00 2001 From: solonovamax Date: Fri, 28 Mar 2025 19:27:37 -0400 Subject: [PATCH 1/8] Add archunit tests for naming conventions Signed-off-by: solonovamax --- .../java/org/kohsuke/github/ArchTests.java | 101 +++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index e21a7ab108..ccc2739bb9 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -1,13 +1,16 @@ package org.kohsuke.github; import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.base.HasDescription; import com.tngtech.archunit.core.domain.*; import com.tngtech.archunit.core.domain.properties.HasName; import com.tngtech.archunit.core.domain.properties.HasOwner; +import com.tngtech.archunit.core.domain.properties.HasSourceCodeLocation; import com.tngtech.archunit.core.importer.ClassFileImporter; import com.tngtech.archunit.core.importer.ImportOption; import com.tngtech.archunit.lang.ArchCondition; import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.lang.conditions.ArchConditions; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.builder.ReflectionToStringBuilder; @@ -15,26 +18,42 @@ import org.apache.commons.lang3.builder.ToStringStyle; import org.junit.BeforeClass; import org.junit.Test; +import org.kohsuke.github.GHDiscussion.Creator; +import org.kohsuke.github.GHPullRequestCommitDetail.Commit; +import org.kohsuke.github.GHPullRequestCommitDetail.CommitPointer; import java.io.Closeable; import java.io.InputStream; import java.io.OutputStream; import java.io.Reader; import java.lang.reflect.Field; +import java.net.URL; import java.nio.charset.Charset; +import java.time.Instant; import java.util.Arrays; import java.util.stream.Collectors; import static com.google.common.base.Preconditions.checkNotNull; +import static com.tngtech.archunit.base.DescribedPredicate.not; +import static com.tngtech.archunit.base.DescribedPredicate.or; import static com.tngtech.archunit.core.domain.JavaCall.Predicates.target; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAPackage; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.type; +import static com.tngtech.archunit.core.domain.JavaMember.Predicates.declaredIn; +import static com.tngtech.archunit.core.domain.JavaModifier.FINAL; +import static com.tngtech.archunit.core.domain.JavaModifier.STATIC; +import static com.tngtech.archunit.core.domain.properties.HasModifiers.Predicates.modifier; import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name; import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameContaining; +import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameMatching; import static com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With.owner; import static com.tngtech.archunit.core.domain.properties.HasParameterTypes.Predicates.rawParameterTypes; +import static com.tngtech.archunit.core.domain.properties.HasReturnType.Predicates.rawReturnType; import static com.tngtech.archunit.lang.conditions.ArchConditions.*; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noFields; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noMethods; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThan; @@ -42,6 +61,8 @@ /** * The Class ArchTests. */ +@SuppressWarnings({ "LocalVariableNamingConvention", "TestMethodWithoutAssertion", "UnqualifiedStaticUsage", + "unchecked", "MethodMayBeStatic", "FieldNamingConvention", "StaticCollection" }) public class ArchTests { private static final JavaClasses classFiles = new ClassFileImporter() @@ -69,6 +90,43 @@ public static void beforeClass() { assertThat(classFiles.size(), greaterThan(0)); } + @Test + public void testRequireFollowingNamingConvention() { + final String reason = "This project follows standard java naming conventions and does not allow the use of underscores in names."; + + final ArchRule fieldsNotFollowingConvention = noFields().that() + .arePublic() + .and(not(enumConstants())) + .and(not(modifier(STATIC).and(modifier(FINAL)).as("static final"))) + .should(haveNamesContainingUnless("_")) + .because(reason); + + @SuppressWarnings("AccessStaticViaInstance") + final ArchRule methodsNotFollowingConvention = noMethods().that() + .arePublic() + .should(haveNamesContainingUnless("_", + nameMatching("[A-Z0-9\\_]+"), + // currently failing cases + // TODO: 2025-03-28 Fix & remove these + declaredIn(PagedIterable.class).and(name("_iterator")).and(rawReturnType(PagedIterator.class)), + declaredIn(GHRepositoryBuilder.class).and(name("private_")).and(rawReturnType(Object.class)), + declaredIn(GHDeployKey.class).and(name("getAdded_by")).and(rawReturnType(String.class)), + declaredIn(GHDeployKey.class).and(name("isRead_only")).and(rawReturnType(boolean.class)), + declaredIn(Creator.class).and(name("private_")).and(rawReturnType(Creator.class)), + declaredIn(GHGistBuilder.class).and(name("public_")).and(rawReturnType(GHGistBuilder.class)), + declaredIn(Commit.class).and(name("getComment_count")).and(rawReturnType(int.class)), + declaredIn(CommitPointer.class).and(name("getHtml_url")).and(rawReturnType(URL.class)), + declaredIn(GHRelease.class).and(name("getPublished_at")).and(rawReturnType(Instant.class)))) + .because(reason); + + final ArchRule classesNotFollowingConvention = noClasses().should(haveNamesContainingUnless("_")) + .because(reason); + + fieldsNotFollowingConvention.check(classFiles); + methodsNotFollowingConvention.check(classFiles); + classesNotFollowingConvention.check(classFiles); + } + /** * Test require use of assert that. */ @@ -78,10 +136,10 @@ public void testRequireUseOfAssertThat() { final String reason = "This project uses `assertThat(...)` or `assertThrows(...)` instead of other `assert*()` methods."; final DescribedPredicate assertMethodOtherThanAssertThat = nameContaining("assert") - .and(DescribedPredicate.not(name("assertThat")).and(DescribedPredicate.not(name("assertThrows")))); + .and(not(name("assertThat")).and(not(name("assertThrows")))); final ArchRule onlyAssertThatRule = classes() - .should(not(callMethodWhere(target(assertMethodOtherThanAssertThat)))) + .should(ArchConditions.not(callMethodWhere(target(assertMethodOtherThanAssertThat)))) .because(reason); onlyAssertThatRule.check(testClassFiles); @@ -135,6 +193,27 @@ public void testRequireUseOfOnlySpecificApacheCommons() { onlyApprovedApacheCommonsMethods.check(classFiles); } + /** + * Have names containing unless. + * + * @param infix + * the infix + * @param unlessPredicates + * the unless predicates + * @return the arch condition + */ + public static ArchCondition haveNamesContainingUnless( + final String infix, + final DescribedPredicate... unlessPredicates) { + DescribedPredicate restrictedNameContaining = nameContaining(infix); + + if (unlessPredicates.length > 0) { + final DescribedPredicate allowed = or(unlessPredicates); + restrictedNameContaining = unless(nameContaining(infix), allowed); + } + return have(restrictedNameContaining); + } + /** * Not call methods in package unless. * @@ -156,7 +235,7 @@ public static ArchCondition notCallMethodsInPackageUnless(final Strin } restrictedPackageCalls = unless(restrictedPackageCalls, allowed); } - return not(callMethodWhere(restrictedPackageCalls)); + return ArchConditions.not(callMethodWhere(restrictedPackageCalls)); } /** @@ -200,6 +279,10 @@ public static DescribedPredicate unless(DescribedPredicate fir return new UnlessPredicate(first, second); } + private DescribedPredicate enumConstants() { + return new EnumConstantFieldPredicate(); + } + private static class UnlessPredicate extends DescribedPredicate { private final DescribedPredicate current; private final DescribedPredicate other; @@ -215,4 +298,16 @@ public boolean test(T input) { return current.test(input) && !other.test(input); } } + + private static final class EnumConstantFieldPredicate extends DescribedPredicate { + private EnumConstantFieldPredicate() { + super("are not enum constants"); + } + + @Override + public boolean test(JavaField javaField) { + JavaClass owner = javaField.getOwner(); + return owner.isEnum() && javaField.getRawType().isAssignableTo(owner.reflect()); + } + } } From e74755e84210cf83b100bd863c9c7e701584e9a7 Mon Sep 17 00:00:00 2001 From: solonovamax Date: Fri, 28 Mar 2025 20:01:14 -0400 Subject: [PATCH 2/8] Fix javadoc warnings Signed-off-by: solonovamax --- src/test/java/org/kohsuke/github/ArchTests.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index ccc2739bb9..631d6d80c4 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -90,6 +90,9 @@ public static void beforeClass() { assertThat(classFiles.size(), greaterThan(0)); } + /** + * Test naming conventions + */ @Test public void testRequireFollowingNamingConvention() { final String reason = "This project follows standard java naming conventions and does not allow the use of underscores in names."; @@ -196,6 +199,8 @@ public void testRequireUseOfOnlySpecificApacheCommons() { /** * Have names containing unless. * + * @param + * the generic type * @param infix * the infix * @param unlessPredicates @@ -279,6 +284,11 @@ public static DescribedPredicate unless(DescribedPredicate fir return new UnlessPredicate(first, second); } + /** + * Enum constants. + * + * @return the described predicate + */ private DescribedPredicate enumConstants() { return new EnumConstantFieldPredicate(); } From 37be904d67059ab4eea68019cae8f09726374e99 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 14:20:18 -0700 Subject: [PATCH 3/8] Update src/test/java/org/kohsuke/github/ArchTests.java --- src/test/java/org/kohsuke/github/ArchTests.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index 631d6d80c4..9f2c7095a3 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -111,15 +111,14 @@ public void testRequireFollowingNamingConvention() { nameMatching("[A-Z0-9\\_]+"), // currently failing cases // TODO: 2025-03-28 Fix & remove these - declaredIn(PagedIterable.class).and(name("_iterator")).and(rawReturnType(PagedIterator.class)), - declaredIn(GHRepositoryBuilder.class).and(name("private_")).and(rawReturnType(Object.class)), - declaredIn(GHDeployKey.class).and(name("getAdded_by")).and(rawReturnType(String.class)), - declaredIn(GHDeployKey.class).and(name("isRead_only")).and(rawReturnType(boolean.class)), - declaredIn(Creator.class).and(name("private_")).and(rawReturnType(Creator.class)), - declaredIn(GHGistBuilder.class).and(name("public_")).and(rawReturnType(GHGistBuilder.class)), - declaredIn(Commit.class).and(name("getComment_count")).and(rawReturnType(int.class)), - declaredIn(CommitPointer.class).and(name("getHtml_url")).and(rawReturnType(URL.class)), - declaredIn(GHRelease.class).and(name("getPublished_at")).and(rawReturnType(Instant.class)))) + declaredIn(PagedIterable.class).and(name("_iterator")), + declaredIn(GHDeployKey.class).and(name("getAdded_by")), + declaredIn(GHDeployKey.class).and(name("isRead_only")), + declaredIn(Creator.class).and(name("private_")), + declaredIn(GHGistBuilder.class).and(name("public_")), + declaredIn(Commit.class).and(name("getComment_count")), + declaredIn(CommitPointer.class).and(name("getHtml_url")), + declaredIn(GHRelease.class).and(name("getPublished_at")) .because(reason); final ArchRule classesNotFollowingConvention = noClasses().should(haveNamesContainingUnless("_")) From 875b913e02ae1e2931eb03e257b1976de63c4845 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 14:28:52 -0700 Subject: [PATCH 4/8] Update src/test/java/org/kohsuke/github/ArchTests.java --- src/test/java/org/kohsuke/github/ArchTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index 9f2c7095a3..c200d0d68e 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -118,7 +118,7 @@ public void testRequireFollowingNamingConvention() { declaredIn(GHGistBuilder.class).and(name("public_")), declaredIn(Commit.class).and(name("getComment_count")), declaredIn(CommitPointer.class).and(name("getHtml_url")), - declaredIn(GHRelease.class).and(name("getPublished_at")) + declaredIn(GHRelease.class).and(name("getPublished_at")))) .because(reason); final ArchRule classesNotFollowingConvention = noClasses().should(haveNamesContainingUnless("_")) From 7247cc60b7e46c0993c3471607a76e969cd4ba70 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 14:34:09 -0700 Subject: [PATCH 5/8] Update src/test/java/org/kohsuke/github/ArchTests.java --- src/test/java/org/kohsuke/github/ArchTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index c200d0d68e..f446873bf6 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -27,7 +27,6 @@ import java.io.OutputStream; import java.io.Reader; import java.lang.reflect.Field; -import java.net.URL; import java.nio.charset.Charset; import java.time.Instant; import java.util.Arrays; From 6f01f245bce8d23ec288a12c9ad7c16acf01e7ca Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 14:35:31 -0700 Subject: [PATCH 6/8] Update src/test/java/org/kohsuke/github/ArchTests.java --- src/test/java/org/kohsuke/github/ArchTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index f446873bf6..0898ef8e04 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -47,7 +47,6 @@ import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameMatching; import static com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With.owner; import static com.tngtech.archunit.core.domain.properties.HasParameterTypes.Predicates.rawParameterTypes; -import static com.tngtech.archunit.core.domain.properties.HasReturnType.Predicates.rawReturnType; import static com.tngtech.archunit.lang.conditions.ArchConditions.*; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; From 55c9085b6f94ce9bd198f5fb9ebbd8a13ac2dfb2 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 14:36:25 -0700 Subject: [PATCH 7/8] Update src/test/java/org/kohsuke/github/ArchTests.java --- src/test/java/org/kohsuke/github/ArchTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index 0898ef8e04..c7fe03d992 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -28,7 +28,6 @@ import java.io.Reader; import java.lang.reflect.Field; import java.nio.charset.Charset; -import java.time.Instant; import java.util.Arrays; import java.util.stream.Collectors; From b4687f9bb09f34436e7b4a55dc332ebd165e383c Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 29 Mar 2025 22:06:58 -0700 Subject: [PATCH 8/8] Fix inherited methods --- src/test/java/org/kohsuke/github/ArchTests.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/kohsuke/github/ArchTests.java b/src/test/java/org/kohsuke/github/ArchTests.java index c7fe03d992..fc4891f868 100644 --- a/src/test/java/org/kohsuke/github/ArchTests.java +++ b/src/test/java/org/kohsuke/github/ArchTests.java @@ -35,6 +35,7 @@ import static com.tngtech.archunit.base.DescribedPredicate.not; import static com.tngtech.archunit.base.DescribedPredicate.or; import static com.tngtech.archunit.core.domain.JavaCall.Predicates.target; +import static com.tngtech.archunit.core.domain.JavaClass.Predicates.assignableTo; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAPackage; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.type; import static com.tngtech.archunit.core.domain.JavaMember.Predicates.declaredIn; @@ -43,7 +44,6 @@ import static com.tngtech.archunit.core.domain.properties.HasModifiers.Predicates.modifier; import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name; import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameContaining; -import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.nameMatching; import static com.tngtech.archunit.core.domain.properties.HasOwner.Predicates.With.owner; import static com.tngtech.archunit.core.domain.properties.HasParameterTypes.Predicates.rawParameterTypes; import static com.tngtech.archunit.lang.conditions.ArchConditions.*; @@ -105,12 +105,13 @@ public void testRequireFollowingNamingConvention() { final ArchRule methodsNotFollowingConvention = noMethods().that() .arePublic() .should(haveNamesContainingUnless("_", - nameMatching("[A-Z0-9\\_]+"), - // currently failing cases + // currently failing method names // TODO: 2025-03-28 Fix & remove these - declaredIn(PagedIterable.class).and(name("_iterator")), + declaredIn(assignableTo(PagedIterable.class)).and(name("_iterator")), + declaredIn(GHCompare.class).and(name("getAdded_by")), declaredIn(GHDeployKey.class).and(name("getAdded_by")), declaredIn(GHDeployKey.class).and(name("isRead_only")), + declaredIn(assignableTo(GHRepositoryBuilder.class)).and(name("private_")), declaredIn(Creator.class).and(name("private_")), declaredIn(GHGistBuilder.class).and(name("public_")), declaredIn(Commit.class).and(name("getComment_count")),