Added ArchUnit for better Consistency in Test-Classes #1206
Conversation
WalkthroughAdds ArchUnit as a test-scoped dependency and property; introduces an ArchUnit test harness, a Rules utility and a custom ArchCondition; renames many test methods to Given-Then style; adds an exact Cache-Control header assertion in one test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant J as JUnit 5
participant A as ArchUnitTest
participant I as ClassFileImporter
participant R as Rules
J->>A: run parameterized tests
A->>I: import tests (OnlyIncludeTests from app package)
I-->>A: allTestClasses
loop for each ArchRule
A->>R: request ArchRule
R-->>A: ArchRule
A->>A: rule.check(allTestClasses)
end
A-->>J: report results (violations/success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Template: Backend
DanielOber
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/CacheControlFilterTest.java (1)
47-52: Make Cache-Control assertion order-insensitive.Some frameworks reorder directives or append extras (e.g., max-age=0). Assert presence instead of exact equality to avoid brittle tests.
- assertEquals(EXPECTED_CACHE_CONTROL_HEADER_VALUES, response.getHeaders().getCacheControl()); + final String cc = response.getHeaders().getCacheControl(); + assertTrue(cc.contains("no-cache")); + assertTrue(cc.contains("no-store")); + assertTrue(cc.contains("must-revalidate"));refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/UnicodeFilterConfigurationTest.java (1)
61-81: Fix typo in test name and guard against NPEs from response/body lookup.Current flow dereferences response before any null-check; repository lookup could also be null.
- void givenDecomposedString_thenCovertToNfcNormalized() { + void givenDecomposedString_thenConvertToNfcNormalized() { @@ - final TheEntityResponseDTO response = testRestTemplate.postForEntity(URI.create(ENTITY_ENDPOINT_URL), theEntityRequestDto, TheEntityResponseDTO.class) - .getBody(); - final TheEntity theEntity = theEntityRepository.findById(response.id()).orElse(null); + final TheEntityResponseDTO response = testRestTemplate + .postForEntity(URI.create(ENTITY_ENDPOINT_URL), theEntityRequestDto, TheEntityResponseDTO.class) + .getBody(); + assertNotNull(response); + final TheEntity theEntity = theEntityRepository.findById(response.id()).orElse(null); @@ - assertNotNull(theEntity.getTextAttribute()); + assertNotNull(theEntity); + assertNotNull(theEntity.getTextAttribute());refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/nfcconverter/NfcHelperTest.java (2)
75-96: Avoid cross-test coupling to NfcConverterTest.TOKEN.Tests should not depend on constants from other test classes. Inline the value or extract a shared TestConstants.
- assertEquals(NfcConverterTest.TOKEN, nfcCookie.getName()); + assertEquals("token", nfcCookie.getName()); @@ - assertEquals(NfcConverterTest.TOKEN, nfcCookie.getName()); + assertEquals("token", nfcCookie.getName());If desired, introduce a shared constant:
// src/test/java/de/muenchen/refarch/TestConstants.java package de.muenchen.refarch; public final class TestConstants { private TestConstants() {} public static final String TOKEN = "token"; }
31-59: Reduce duplication with parameterized tests.The three near-identical assertions per input can be expressed via @ParameterizedTest with a source of NFD/NFC pairs for better maintainability.
refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/nfcconverter/NfcConverterTest.java (2)
72-89: Nit: Prefer “ContentType” casing in method name.Keeps names idiomatic and consistent with header term.
- void givenContenttypeInWhitelist_thenFilter() throws ServletException, IOException { + void givenContentTypeInWhitelist_thenFilter() throws ServletException, IOException {
92-109: Nit: Prefer “ContentType” casing in method name.- void givenContenttypeNotInWhitelist_thenSkipFilter() throws ServletException, IOException { + void givenContentTypeNotInWhitelist_thenSkipFilter() throws ServletException, IOException {refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/UserInfoAuthoritiesConverterTest.java (1)
50-70: Test name contradicts behavior: it loads roles.The method sets two authorities; rename to reflect “roles present”.
- void givenNoRoles_thenConvert() { + void givenRoles_thenConvert() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
refarch-backend/pom.xml(2 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/CacheControlFilterTest.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/UnicodeFilterConfigurationTest.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/nfcconverter/NfcConverterTest.java(2 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/filter/nfcconverter/NfcHelperTest.java(5 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakRolesAuthoritiesConverterTest.java(4 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/UserInfoAuthoritiesConverterTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(10-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: Run docker compose healthcheck
- GitHub Check: build (refarch-backend)
🔇 Additional comments (2)
refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakRolesAuthoritiesConverterTest.java (1)
39-39: Renames align with the new Given-Then convention.Method names match the ArchUnit regex and keep package‑private visibility. No further changes needed here.
Also applies to: 57-57, 74-74, 90-90
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
24-31: Anchor lifecycle method names.Without
^...$, names likesetUpDataortearDownNowwould pass.Apply this diff:
- public static final ArchRule RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED = methods() - .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("setUp") + public static final ArchRule RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED = methods() + .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("^setUp$") .allowEmptyShould(true); - public static final ArchRule RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED = methods() - .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("tearDown") + public static final ArchRule RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED = methods() + .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("^tearDown$") .allowEmptyShould(true);Likely an incorrect or invalid review comment.
refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Outdated
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Outdated
Show resolved
Hide resolved
...ackend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java
Outdated
Show resolved
Hide resolved
...ackend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (2)
19-22: Tighten the test-name regex: use a single end anchor.Multiple
$are redundant.- .should().haveNameMatching("^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$$$$$"); + .should().haveNameMatching("^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$");
16-17: Make this utility holder class final.Prevents subclassing; you already block instantiation with the private no‑args ctor.
-@NoArgsConstructor(access = AccessLevel.PRIVATE) -public class MethodRules { +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public final class MethodRules {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(10-37)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
refarch-backend/pom.xml (2)
61-61: Pin looks fine; confirm compatibility with JDK 21/JUnit 5.1.4.1 should be OK, but please confirm locally to avoid CI surprises.
#!/bin/bash # Verify we don't rely on JUnit 5 integration classes (which would need archunit-junit5*) rg -nP 'com\.tngtech\.archunit\.junit5' -C2 --type=java || true
263-268: Consider junit5 integration artifact; keep test scope.Scope is correct. If you ever use ArchUnit’s JUnit 5 annotations/extensions, switch to the junit5 engine artifact.
- <dependency> - <groupId>com.tngtech.archunit</groupId> - <artifactId>archunit</artifactId> - <version>${archunit.version}</version> - <scope>test</scope> - </dependency> + <dependency> + <groupId>com.tngtech.archunit</groupId> + <artifactId>archunit-junit5-engine</artifactId> + <version>${archunit.version}</version> + <scope>test</scope> + </dependency>refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
21-24: Optional: allow common suffixes “Tests”/“IT”.Broaden the rule to typical test class suffixes.
- if (!topEnclosingClass.getSimpleName().endsWith("Test")) { + if (!allowedTestSuffix(topEnclosingClass.getSimpleName())) { events.add(SimpleConditionEvent.violated(method, "Method %s must be declared in a class whose simple name ends with 'Test' (found: %s)" .formatted(method.getName(), topEnclosingClass.getSimpleName()))); } } private JavaClass getTopEnclosingClass(JavaClass item) { while (item.getEnclosingClass().isPresent()) { item = item.getEnclosingClass().orElseThrow(); } return item; } + + private static boolean allowedTestSuffix(String simpleName) { + return simpleName.endsWith("Test") || simpleName.endsWith("Tests") || simpleName.endsWith("IT"); + }Also applies to: 27-32
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (3)
23-29: Anchor exact method names for BeforeEach/AfterEach.Avoid substring matches; use exact name.
- public static final ArchRule RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED = methods() - .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("setUp") + public static final ArchRule RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED = methods() + .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("^setUp$") .allowEmptyShould(true); - public static final ArchRule RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED = methods() - .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("tearDown") + public static final ArchRule RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED = methods() + .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("^tearDown$") .allowEmptyShould(true);
31-35: Allow empty result to avoid false negatives in modules without tests.Align with the other rules.
public static final ArchRule RULE_TEST_METHODS_ARE_PACKAGE_PRIVATE_CONVENTION_MATCHED = methods() .that().areAnnotatedWith(Test.class).or().areAnnotatedWith(ParameterizedTest.class).should() .notHaveModifier(JavaModifier.PROTECTED) .andShould().notHaveModifier(JavaModifier.PRIVATE) - .andShould().notHaveModifier(JavaModifier.PUBLIC); + .andShould().notHaveModifier(JavaModifier.PUBLIC) + .allowEmptyShould(true);
37-39: Consider allowEmptyShould(true) for consistency.Keeps behavior consistent across rules.
public static final ArchRule RULE_TESTCLASSES_END_WITH_TEST_CONVENTION_MATCHED = methods() .that().areAnnotatedWith(Test.class).or().areAnnotatedWith(ParameterizedTest.class) - .should(haveTopEnclosingClassEndingWithTest); + .should(haveTopEnclosingClassEndingWithTest) + .allowEmptyShould(true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
refarch-backend/pom.xml(2 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(9-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: build (refarch-backend)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java
Outdated
Show resolved
Hide resolved
...ackend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (2)
18-25: Allow “Tests”/“IT” suffixes and reflect them in the violation message.Current rule is too strict for common integration-test conventions.
- if (!topEnclosingClass.getSimpleName().endsWith("Test")) { - events.add(SimpleConditionEvent.violated(method, "Method %s must be declared in a class whose simple name ends with 'Test' (found: %s)" - .formatted(method.getName(), topEnclosingClass.getSimpleName()))); + if (!allowedTestSuffix(topEnclosingClass.getSimpleName())) { + events.add(SimpleConditionEvent.violated( + method, + "Method %s must be declared in a class whose simple name ends with one of [Test, Tests, IT] (found: %s)" + .formatted(method.getName(), topEnclosingClass.getSimpleName()) + )); }Add alongside this class (outside the diff range):
// imports: // import java.util.Set; private static boolean allowedTestSuffix(String simpleName) { return simpleName.endsWith("Test") || simpleName.endsWith("Tests") || simpleName.endsWith("IT"); }
11-13: Align condition description with allowed suffixes.If you adopt multiple suffixes, update the label for clarity.
- super("have top enclosing class name ending with `Test`"); + super("have top enclosing class name ending with one of [`Test`, `Tests`, `IT`]");refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (5)
18-21: Allow empty result to avoid false negatives in modules without tests.public static final ArchRule RULE_TEST_NAMING_CONVENTION_SHOULD_WHEN_MATCHED = methods() .that().areAnnotatedWith(Test.class).or().areAnnotatedWith(ParameterizedTest.class) - .should().haveNameMatching("^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$"); + .should().haveNameMatching("^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$") + .allowEmptyShould(true);
22-25: Anchor exact BeforeEach method name.- .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("setUp") + .that().areAnnotatedWith(BeforeEach.class).should().haveNameMatching("^setUp$")
26-29: Anchor exact AfterEach method name.- .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("tearDown") + .that().areAnnotatedWith(AfterEach.class).should().haveNameMatching("^tearDown$")
30-35: Permit empty result and keep rules consistent.public static final ArchRule RULE_TEST_METHODS_ARE_PACKAGE_PRIVATE_CONVENTION_MATCHED = methods() .that().areAnnotatedWith(Test.class).or().areAnnotatedWith(ParameterizedTest.class).should() .notHaveModifier(JavaModifier.PROTECTED) .andShould().notHaveModifier(JavaModifier.PRIVATE) - .andShould().notHaveModifier(JavaModifier.PUBLIC); + .andShould().notHaveModifier(JavaModifier.PUBLIC) + .allowEmptyShould(true);
36-39: Also allow empty result for the “classes end with Test” rule.public static final ArchRule RULE_TESTCLASSES_END_WITH_TEST_CONVENTION_MATCHED = methods() .that().areAnnotatedWith(Test.class).or().areAnnotatedWith(ParameterizedTest.class) - .should(haveTopEnclosingClassEndingWithTest); + .should(haveTopEnclosingClassEndingWithTest) + .allowEmptyShould(true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(9-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: build (refarch-backend)
🔇 Additional comments (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/MethodRules.java (1)
15-16: Utility holder hardened correctly.final + private ctor via Lombok is appropriate.
...ackend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java
Show resolved
Hide resolved
…convention---backend
…convention---backend
…test-naming-convention---backend' into 758-feature-archunit---validate-test-naming-convention---backend
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java (1)
26-30: Prefer strongly typed parameters over ArgumentsAccessor (repeat).Keeps the display name behavior and adds compile-time safety.
Apply:
- @ParameterizedTest(name = "{0}") - @MethodSource("allTestClassesRulesToVerify") - void givenAllArchUnitRulesForAllTestClasses_thenRunArchUnitTests(final ArgumentsAccessor arguments) { - arguments.get(1, ArchRule.class).check(allTestClasses); - } + @ParameterizedTest(name = "{0}") + @MethodSource("allTestClassesRulesToVerify") + void givenAllArchUnitRulesForAllTestClasses_thenRunArchUnitTests(final String ruleName, final ArchRule rule) { + rule.check(allTestClasses); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(9-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: build (refarch-backend)
🔇 Additional comments (2)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java (1)
1-24: Overall ArchUnit setup looks good.Scoped import of test classes and one-time initialization are sensible. No blocking issues here.
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java (1)
15-39: Rules set is clear and cohesive.Good use of ArchUnit DSL; sensible allowEmptyShould for lifecycle hooks.
refarch-backend/src/test/java/de/muenchen/refarch/archunit/ArchUnitTest.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
devtobi
left a comment
devtobi
left a comment
There was a problem hiding this comment.
I am starting to wonder if the configured rules (e.g. modifiers for test methods, naming of test methods, etc.) could also be implemented via different mechanisms like linting tools.
Enforcing best practices and uniform implementations increases readability and maintainability, on the other hand its more nitpicky and could annoy users.
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
...ackend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java (1)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/TestClassesEndWithTestCondition.java (1)
TestClassesEndWithTestCondition(9-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: build (refarch-backend)
🔇 Additional comments (5)
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java (5)
17-18: LGTM: Proper utility class structure.The combination of
finalclass and@NoArgsConstructor(access = PRIVATE)correctly prevents instantiation and subclassing.
20-26: Verify the regex pattern strictness.The current pattern
^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$requires at least two characters after "given" and "then" (e.g.,givenXx_thenYy). This means method names likegivenNull_thenOkwould pass, butgivenX_thenYwould fail.Is this intentional? If single-letter suffixes should be allowed, consider:
- .should().haveNameMatching("^given[A-Z][a-zA-Z]+_then[A-Z][a-zA-Z]+$"); + .should().haveNameMatching("^given[A-Z][a-zA-Z]*_then[A-Z][a-zA-Z]*$");This changes
+(one or more) to*(zero or more), allowinggivenX_thenYwhile still requiring the initial uppercase letter.
28-34: LGTM: Lifecycle method rules properly anchored.The regex patterns for
setUpandtearDownare correctly anchored (^setUp$and^tearDown$), addressing the previous review feedback. The use ofallowEmptyShould(true)appropriately handles cases where no such methods exist.Based on learnings
36-43: LGTM: Package-private enforcement logic is correct.The rule correctly enforces package-private (default) access by prohibiting
PROTECTED,PRIVATE, andPUBLICmodifiers on test methods.
45-50: LGTM: Test class naming rule uses custom condition correctly.The rule properly leverages the
haveTopEnclosingClassEndingWithTestcustom condition to enforce that test methods reside in classes ending with "Test", handling nested classes correctly.
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
refarch-backend/src/test/java/de/muenchen/refarch/archunit/rules/Rules.java
Show resolved
Hide resolved
…convention---backend
…convention---backend
| Arguments.of("RULE_TESTCLASSES_END_WITH_TEST_CONVENTION_MATCHED", | ||
| Rules.RULE_TESTCLASSES_END_WITH_TEST_CONVENTION_MATCHED), | ||
| Arguments.of("TEST_NAMING_CONVENTION_RULE", Rules.RULE_TEST_NAMING_CONVENTION_GIVEN_THEN_MATCHED), | ||
| Arguments.of("RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED", Rules.RULE_BEFORE_EACH_NAMING_CONVENTION_MATCHED), | ||
| Arguments.of("RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED", Rules.RULE_AFTER_EACH_NAMING_CONVENTION_MATCHED), | ||
| Arguments.of("TEST_METHODS_ARE_PACKAGE_PRIVATE_CONVENTION_MATCHED", Rules.RULE_TEST_METHODS_ARE_PACKAGE_PRIVATE_CONVENTION_MATCHED)); |
There was a problem hiding this comment.
Instead of duplicating the rule name I'd add a more descriptive name that describes the ArchUnit check
| arguments.get(1, ArchRule.class).check(allTestClasses); | ||
| } | ||
|
|
||
| public static Stream<Arguments> allTestClassesRulesToVerify() { |
There was a problem hiding this comment.
I'd move this method all the way to the top because when we add a new rule this stream needs to be extended but the parameterized itself not so its less important.
| .that().areAnnotatedWith(Test.class) | ||
| .or().areAnnotatedWith(ParameterizedTest.class) | ||
| .or().areAnnotatedWith(RepeatedTest.class) | ||
| .or() | ||
| .areAnnotatedWith(TestTemplate.class) |
There was a problem hiding this comment.
Most tests have these 4 lines in common, maybe you can move them into a reusable snippet? e.g. areTests()
| .that().areAnnotatedWith(Test.class) | ||
| .or().areAnnotatedWith(ParameterizedTest.class) | ||
| .or().areAnnotatedWith(RepeatedTest.class).or() | ||
| .areAnnotatedWith(TestTemplate.class).should() |
| .that().areAnnotatedWith(Test.class) | ||
| .or().areAnnotatedWith(ParameterizedTest.class) | ||
| .or().areAnnotatedWith(RepeatedTest.class).or() | ||
| .areAnnotatedWith(TestTemplate.class) |
Pull Request
Changes
Reference
Issue: #758
Checklist
Note: If some checklist items are not relevant for your PR, just remove them.
General
I have read the Contribution Guidelines (TBD)Code
[ ] Wrote code and comments in English[ ] Added unit testsconsole.log), see code quality toolingBackend / EAI
[ ] Added integration tests[ ] Updated database migration scripts (if changes to model were made)[ ] Added Swagger API annotations (if changes to API was made)[ ] Checked Spring Boot version matching Camel version inpom.xml(if Camel version was bumped)Screenshots (if UI was changed)
Summary by CodeRabbit
Tests
Chores