Make every rule able to fail, and prove it - #4
Open
SirCotare wants to merge 7 commits into
Open
Conversation
Each of the 13 rules gets a fixture built to violate it and one built to satisfy it, asserted on the violation count and message rather than on the rule having run. The library had no such test, which is why rules that match nothing have been reporting success. This commit is deliberately red: nine tests fail. Eight fixtures that must produce a violation do not. - test classes with no production counterpart (rule is fully dead) - a blacklisted annotation on a constructor parameter - a blacklisted method call from a constructor - a blacklisted method call from an instance field initializer - System.out from a constructor - a @nested test class whose production class is missing entirely - a controller method covered only by a longer-named sibling's @nested class - a non-static SortMappings field The ninth is not a fixture: the blacklist names three AssertJ methods that do not exist, so those entries can never match. Fixtures live outside it.aboutbits.archunit.toolbox so the project's own ArchitectureTest does not analyse them, and are excluded from surefire because some are named *Test. Third-party types the blacklists name are stubbed rather than depended on: spring-boot-toolbox depends on archunit-toolbox, so SortMappings, @Store and @ArchAllowDirectAccess cannot come from there. The stub contracts were verified against the real 2.5.2 artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two root causes, each shared by several rules.
The suffix regex was hand-built in five places, in two shapes, against two
different inputs. The counterpart rule's condition rebuilt it without the
leading ".+" and matched it against getSimpleName() with String.matches, which
anchors both ends - so only a class named exactly "Test" got past the guard and
every real test class returned before the counterpart lookup ever ran. The rule
has therefore never reported anything.
TestClassNames now owns the pattern, the stripping and the selection predicate.
The redundant guard in the condition is gone rather than corrected: the selection
already guarantees the suffix, and re-deriving it in the condition is what let
the two drift apart. Selection also matches the simple name and requires a
non-empty production name to be left over, so it can no longer accept a name the
stripping turns into nothing ("CacheTest" matched with "Cache" as the ".+").
Three rules walked getMethods(), which excludes constructors - and with them
every instance field initializer, since that is compiled into the constructor.
So a blacklisted annotation on a constructor parameter, the canonical Lombok
position, was invisible. All three now walk getCodeUnits(), which also subsumes
the separate static-initializer branches two of them carried.
Also drops three blacklist entries naming AssertJ methods that do not exist
(assertThrows, assertThrowsExactly, assertDoesNotThrow are JUnit's). An entry
that can never match reads as coverage without providing any.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes the remaining ways a rule stayed quiet. allowEmptyShould(true) is gone everywhere. A rule that selects nothing reported success, indistinguishable from a rule that is satisfied, which is what let a dead rule survive. EmptySelectionTest pins this for all eight rules that narrow their input. Consequence to be aware of: CommonArchRuleCollection now fails in a project with no controllers or no @Store classes, so implement it only where those exist. The @nested name rule skipped a nested class silently whenever the production class was missing and the nested class was not inside a group - so the case with nothing to compare against was the one case never reported. It now reports, and in exchange honours @ArchIgnoreNoProductionCounterpart: a test class that declares it has no production counterpart has no production methods either. The security-test rule accepted any @nested class *starting with* the method name, so getAll() counted as covered by GetAllArchived. It now requires the name to match exactly or to continue with "$", which keeps grouped @nested classes working. The SortMappings rule swallowed every failure to read a field into a log.warn - and the build has no SLF4J provider, so the warning was discarded outright. A non-static field, an unreadable field, an unresolvable enum type and a value that is not a Map are all violations now. Non-static in particular is reported as such, since it can never be validated. Finally, @ArchIgnoreNoProductionCounterpart and @ArchIgnoreGroupName no longer meta-annotate ArchUnit's @ArchIgnore. The ArchUnit JUnit engine resolves meta-annotations, so annotating a test class skipped *every* @archtest on it and still reported BUILD SUCCESS - verified: putting it on this project's own ArchitectureTest turned 11 rules into 11 skips. Both annotations are read by their own type, so the meta-annotation bought nothing. ArchitectureTest now carries it in place of the name that was hardcoded into the counterpart rule, which also keeps the annotation exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The usage section still described the 1.1.0 API (extends ArchitectureTestBase, a BLACKLISTED_CLASSES static block), which no longer exists. Replaces it with the rule collections, records why no rule allows an empty selection, and documents both opt-out annotations - neither of which any consumer appears to use yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Failing on an empty selection was the wrong call. Seven of the eight rules that narrow their input have a legitimate empty case - no records, no controllers, no @Store classes, nobody using @nested - and one of them described this repository before this branch: a project whose only test is the ArchitectureTest has no @test methods at all. Whether that code exists is the project's business. allowEmptyShould(true) was never what protected against a dead rule either. It fires on what a consumer's code happens to contain and says nothing about whether a rule's logic works: the counterpart rule that started this had a perfectly non-empty selection and a broken condition. The guarantee is the red test per rule in this project, so all 13 rules now tolerate an empty selection. That leaves one case worth failing on. A mistyped or moved package in @AnalyzeClasses imports nothing, and every rule then passes without looking at a single class. AnalyzedPackagesMustContainClassesArchRule turns that into one failure naming the cause, instead of five rules passing silently and eight reporting that they checked nothing. It is in both collections, so it applies wherever the toolbox is used. EmptySelectionTest is inverted to match: it now pins that every rule accepts a project with nothing for it to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Converts every documentation comment to the /// form of JEP 467: 54 javadoc
comments plus two block comments that already sat in documentation positions.
Markdown idiom throughout - `code` for {@code}, [Type#member] for {@link}, blank
/// lines for <p>, and backticked annotation names for the @ entities.
The five comments inside method bodies stay /* */, since /// is a documentation
comment form and those document statements rather than declarations.
No behaviour change. Verified with javadoc -Xdoclint:all: no warnings, the
[#member] references resolve, and the generated HTML shows code spans, paragraphs
and links rather than literal markdown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Addresses the review on #4, all three points verified against reverts. The opt-outs are read with areNotMetaAnnotatedWith / isMetaAnnotatedWith instead of the direct-only variants, so a project carries @ArchIgnoreNoProductionCounterpart on one stereotype of its own rather than repeating it on 74 classes. ArchUnit counts a direct annotation as meta-annotated, so annotating a single class still works. @ArchIgnoreGroupName gets the same treatment in all three places it is read: it is the sibling opt-out with the identical ergonomics problem, and leaving it direct-only would be half a fix. Architecture tests are exempted from the production-counterpart rule by package, via .._architecture.. alongside .._support.. / .._config.., so dropping the hardcoded "ArchitectureTest" name does not push boilerplate onto every consumer. Deliberately not added to TestClassVisibilityArchRule: being package private is as achievable for an architecture test as for any other test, so the two exclusion lists encode different facts and are meant to differ. org.junit.jupiter.api.Assertions.assertThrowsExactly is blacklisted again. It only ever existed under the AssertJ namespace that the previous commit removed, so the cleanup dropped the house rule along with the bogus entry. Now covered by a fixture that actually calls it, not only by a list assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Target Version: 1.3.0
Why
test_classes_should_be_in_the_same_package_as_their_production_codehas never reported anything, in any project: the condition rebuilt the suffix regex without the leading.+, andString.matchesanchors both ends, so every real test class returned before the counterpart lookup ran. Auditing the other 12 rules against purpose-built fixtures found eight more silent passes — a rule that matches nothing reports success.What changed
Every rule now has a fixture built to violate it and one built to satisfy it, asserted on violation count and message. Nine defects fixed, and the suffix regex and code-unit iteration each moved into one shared, tested place.
Proving a rule can fail is the job of these tests, not of consumers' builds, so every rule tolerates a selection that does not apply to the project. The one case that genuinely must fail is a mistyped
@AnalyzeClassespackage importing nothing, which previously let five rules pass silently while eight reported that they checked nothing.AnalyzedPackagesMustContainClassesArchRulenow turns that into one failure naming the cause.Before / after
Review notes
@ArchIgnoreNoProductionCounterpartwas meta-annotated@ArchIgnore, which ArchUnit's JUnit engine resolves — so putting it on a test class skipped every@ArchTeston that class and still reported BUILD SUCCESS. Verified on this repo's ownArchitectureTest: 12 rules became 12 skips. That fix needs its own guard test, because skipped tests do not fail a build.@ArchIgnoreNoProductionCounterpart. I rejected adding.._business..to the library's excluded packages, as that bakes one project's convention into shared tooling.TestClassNamesrebuilds its regex per call on purpose: caching it would silently break consumers that mutateTEST_CLASS_SUFFIXES.///markdown form in their own commit, no behaviour change..._architecture..exempts arch tests from the counterpart rule (deliberately not from the visibility rule — the two lists encode different facts);assertThrowsExactlyre-blacklisted under its real JUnit owner.LineNumberUtilAPI, two limits of the record-accessor rule) are noted in the session, not addressed here.Tests
mvn clean test: 81 pass, 0 skipped, checkstyle and NullAway clean. Each of the eleven fixes was additionally validated by reverting it and confirming a named test fails. Not run: any consumer project build.🤖 Generated with Claude Code