Analyse the test sources with Checkstyle - #219
Merged
Merged
Conversation
Same gap as the PMD change, same reasoning: the harness deciding whether a violation was detected was the code nobody checked. 2314 findings, of which 2045 were MethodName firing on names like test_invokeMethod_success. That convention is not cosmetic, it is the test-selection mechanism: maven.yml picks matrix cells with -Dtest='...#*_wala_aspectj'. Suppressed for the test tree rather than loosened in the rule, which would have relaxed main sources as well. The fixture trees are suppressed for the same reason as in the PMD change. What is left is 83 real findings, all fixed: 34 single-statement if/for/else bodies now carry braces, one redundant public modifier is gone. The brace change was verified to be exactly that and nothing else: with braces removed from both token streams, every touched file is token-identical to origin/main. AstAssertionTest keeps two suppressions. Its locals are named after the supervised test method they assert about, so LocalVariableName was fighting the same convention as MethodName, and HideUtilityClassConstructor fires because JUnit sees only static members on a class whose tests live in @nested inner classes.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
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.
Summary
Checkstyle never looked at the test tree either. Turning it on reported 2314 findings, of which 2045 were the naming convention that the CI itself relies on to select matrix cells. That is suppressed for the test tree rather than loosened in the rule; the remaining 83 are fixed.
Linked issues
No linked issues
1. Problem
maven-checkstyle-pluginran withoutincludeTestSourceDirectory, so the test tree was unchecked. The same gap as the PMD change and the same reason it matters: the harness that decides whether a violation was detected is the code most able to fail quietly.Turning it on is not a one-line change, because 2045 of the 2314 findings are
MethodNamefiring on names such astest_invokeMethod_success. Those underscores are not cosmetic. The Maven workflow selects matrix cells with-Dtest='...#*MavenWalaAspectJ+*_wala_aspectj', so the convention is what makes the mode matrix work at all.2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
Test sources are checked from now on, without the rule fighting the repository's own test-selection mechanism.
MethodNameis suppressed for the test tree in a suppressions file rather than relaxed incheckstyle-rules.xml, so main sources keep the strict pattern.What was left is fixed rather than suppressed: 34 single-statement
if,forandelsebodies now carry braces, and one redundantpublicmodifier is gone. Braces matter more here than style: the next person to add a second statement under an unbracedifin the harness would silently move it outside the condition.4. Testing manual
Prerequisites
Steps
Not reproducible from an exercise. This changes the build and the test tree.
mvn checkstyle:check.mvn test -Punit-core-tests,coverage.mvn test -Pintegration-core-tests,coverage..settings/checkstyle-suppressions.xml.Expected result
Step 1 reports no violations. Step 2 reports 774 tests with no failures. Step 3 reports 319 tests with no failures. Step 4 shows two kinds of entry, the fixture paths and the
MethodNamesuppression, each with its reason.The brace change was verified to be exactly that and nothing more: with braces stripped from both token streams, every touched file is token-identical to
main. A reviewer can repeat that check on any of the five files, or read the diff and confirm that only{and}were added.Negative case (what must still be rejected)
MethodNamemust still apply tosrc/main/java. Confirm by checking that the suppression in.settings/checkstyle-suppressions.xmlis scoped tosrc/test/java, and thatcheckstyle-rules.xmlstill contains an unmodifiedMethodNamemodule.No conditional may have changed which statements it governs. Confirm by reading the diff of
ThrowableUtilsTest.java,CustomConditions.javaandTestUserExtension.java.Modes exercised
No mode-specific behaviour changed.
5. Test case coverage regarding this PR
No production Java code changed
Breaking changes and migration
No breaking changes or migration.
Checklist
Review progress