Skip to content

Commit 74ed237

Browse files
SirCotareclaude
andcommitted
address review: lookup cost, null mappings, and an upgrade note
Four review comments on #4. The two counterpart lookups scanned every imported class per test class. JavaClasses is map-backed by fully qualified name, so both are now contain()/get(). New fixture nestedclassname.goodgroup pins that a production class nested inside another is still found, since its name contains a '$' and nothing covered that before - without it a keying difference would have passed silently, the badgroup fixture expecting a violation either way. SortMappings gets the null-value fixture: a static field that reads back as null yields nothing to compare, so the rule reports rather than calling it exhaustive. The undeterminable-enum and non-enum-key paths remain untested. Readme gains an upgrade note naming what breaks when a consumer moves off 1.2.0 - revived rules surfacing real violations, and the new empty-import failure - and the opt-out section now says that @disabled and @ArchIgnore are matched through meta-annotations too, so a stereotype carrying either exempts its classes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 059cb4f commit 74ed237

9 files changed

Lines changed: 94 additions & 11 deletions

File tree

readme.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,25 @@ Add this library to the classpath by adding the following maven dependency. Vers
1616
</dependency>
1717
```
1818

19+
## Upgrading to 1.3.0
20+
21+
**This release will fail builds that passed on 1.2.0, on purpose.** Nine rules were silently
22+
reporting success because they matched nothing; fixing them turns real violations into build
23+
failures for the first time. Expect two kinds:
24+
25+
- **Revived rules surface real violations.** Chiefly the two that were fully dead:
26+
`test_classes_should_be_in_the_same_package_as_their_production_code` and
27+
`nested_test_classes_have_matching_production_method_name`. Triage them with the opt-out
28+
stereotype described under [Opting out](#opting-out) before annotating classes one at a time.
29+
The stricter paths (`getCodeUnits()` reaching constructors and field initializers, exact
30+
security-test matching, `SortMappings` reporting what it cannot read) surfaced nothing in a
31+
large codebase, so noise from those is unlikely.
32+
- **`analyzed_packages_must_contain_classes` is new and fails on an empty import.** If the
33+
packages given to `@AnalyzeClasses` are mistyped or have moved, that is now a failure instead
34+
of 13 rules quietly passing.
35+
36+
Nothing else needs a migration: no rule fails over code your project does not have.
37+
1938
## Usage
2039

2140
Implement one of the provided rule collections in your own architecture test.
@@ -90,6 +109,12 @@ public @interface BusinessTest {
90109
Annotating a single class directly still works — ArchUnit counts a direct annotation as
91110
meta-annotated.
92111

112+
The same applies to `@Disabled` and ArchUnit's `@ArchIgnore`, which these rules also honour: a
113+
stereotype that carries either of them exempts every class using it. That matches how JUnit and the
114+
ArchUnit engine themselves read those two annotations — a class whose tests do not run is not held to
115+
naming rules — but it does mean a stereotype can exempt more than it appears to, so keep an eye on
116+
what your own test annotations carry.
117+
93118
Architecture tests need neither: any class in a package named `_architecture` is exempt from the
94119
production-counterpart rule, alongside the existing `_support` and `_config` exclusions. Use the
95120
annotation for the one-off that lives elsewhere.

src/main/java/it/aboutbits/archunit/toolbox/rule/base/TestClassInCorrectPackageArchRule.java

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,9 @@ public void check(JavaClass testClass, ConditionEvents events) {
6161
var productionClassSimpleName = TestClassNames.productionClassSimpleName(testClass.getSimpleName());
6262
var productionClassFullName = testClass.getPackageName() + "." + productionClassSimpleName;
6363

64-
var productionClass = allClasses.stream()
65-
.filter(clazz -> clazz.getFullName().equals(productionClassFullName))
66-
.findFirst();
67-
68-
if (productionClass.isEmpty()) {
64+
// JavaClasses is map-backed by fully qualified name, so this is a lookup rather than a
65+
// scan of every imported class per test class.
66+
if (!allClasses.contain(productionClassFullName)) {
6967
var message = "Test class <%s> does not have a matching production class <%s> in the same package (%s.java:0)".formatted(
7068
testClass.getFullName(),
7169
productionClassFullName,

src/main/java/it/aboutbits/archunit/toolbox/rule/base/TestNestedClassMatchNameArchRule.java

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -122,16 +122,15 @@ public void check(JavaClass testClass, ConditionEvents events) {
122122
enclosingClassSuffix.orElse("")
123123
);
124124

125-
var productionClassOptional = allClasses.stream()
126-
.filter(clazz -> clazz.getFullName().equals(productionClassName))
127-
.findFirst();
128-
129125
/*
130126
* Reported whether or not the @Nested class is inside a @Nested group. Without a
131127
* production class there is nothing to match the name against, so staying silent
132128
* here means the @Nested class is never checked at all.
129+
*
130+
* JavaClasses is map-backed by fully qualified name, so this is a lookup rather than
131+
* a scan of every imported class per @Nested class.
133132
*/
134-
if (productionClassOptional.isEmpty()) {
133+
if (!allClasses.contain(productionClassName)) {
135134
var message = "The @Nested test class <%s> (%s.java:%s)%ndoes not have a matching production class <%s>".formatted(
136135
nestedClass.getName(),
137136
nestedClassBaseClassSimpleName,
@@ -140,7 +139,7 @@ public void check(JavaClass testClass, ConditionEvents events) {
140139
);
141140
events.add(SimpleConditionEvent.violated(nestedClass, message));
142141
} else {
143-
var productionClass = productionClassOptional.get();
142+
var productionClass = allClasses.get(productionClassName);
144143

145144
var methodExists = productionClass.getMethods()
146145
.stream()
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package it.aboutbits.archunit.fixture.nestedclassname.goodgroup;
2+
3+
public class Widget {
4+
public static class DeleteAction {
5+
public void deleteAll() {
6+
}
7+
}
8+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
package it.aboutbits.archunit.fixture.nestedclassname.goodgroup;
2+
3+
import org.junit.jupiter.api.Nested;
4+
5+
/// The @Nested group maps onto the production nested class Widget$DeleteAction, so the lookup has to
6+
/// resolve a fully qualified name containing a '$'.
7+
class WidgetTest {
8+
@Nested
9+
class DeleteAction {
10+
@Nested
11+
class DeleteAll {
12+
}
13+
}
14+
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package it.aboutbits.archunit.fixture.sortmappings.badnullvalue;
2+
3+
public enum WidgetSort {
4+
NAME,
5+
CREATED_AT
6+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
package it.aboutbits.archunit.fixture.sortmappings.badnullvalue;
2+
3+
import it.aboutbits.springboot.toolbox.persistence.SortMappings;
4+
import it.aboutbits.springboot.toolbox.stereotype.Store;
5+
6+
/// A field that reads back as null yields no mappings to compare, so the rule cannot verify it.
7+
@Store("widget")
8+
public class WidgetStore {
9+
static final SortMappings<WidgetSort> SORT_MAPPINGS = null;
10+
11+
public Object mappings() {
12+
return SORT_MAPPINGS;
13+
}
14+
}

src/test/java/it/aboutbits/archunit/toolbox/rule/base/TestNestedClassMatchNameArchRuleTest.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,13 @@ void a_nested_test_class_whose_production_class_is_missing_entirely_is_reported(
4646
.hasMessageContaining("does not have a matching production class");
4747
}
4848

49+
/// Pins that a production class nested inside another is found: its fully qualified name
50+
/// contains a '$', and the lookup is by that name.
51+
@Test
52+
void a_nested_group_matching_a_production_nested_class_is_accepted() {
53+
nested_test_classes_have_matching_production_method_name(fixture("nestedclassname.goodgroup"));
54+
}
55+
4956
@Test
5057
void a_nested_test_class_matching_a_production_method_is_accepted() {
5158
nested_test_classes_have_matching_production_method_name(fixture("nestedclassname.good"));

src/test/java/it/aboutbits/archunit/toolbox/rule/common/SortMappingsExhaustiveArchRuleTest.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,18 @@ void a_non_static_sort_mappings_field_is_reported() {
3434
assertThat(failure).hasMessageContaining("must be static");
3535
}
3636

37+
/// Reading the field succeeds but yields nothing to compare, so there is no basis on which to
38+
/// call the mappings exhaustive.
39+
@Test
40+
void a_sort_mappings_field_that_reads_back_as_null_is_reported() {
41+
var classes = fixture("sortmappings.badnullvalue");
42+
43+
var failure = violationOf(() -> sort_mappings_cover_all_sort_enum_values(classes));
44+
45+
assertThat(violationCount(failure)).isEqualTo(1);
46+
assertThat(failure).hasMessageContaining("did not yield a Map (got null)");
47+
}
48+
3749
@Test
3850
void exhaustive_sort_mappings_are_accepted() {
3951
sort_mappings_cover_all_sort_enum_values(fixture("sortmappings.good"));

0 commit comments

Comments
 (0)