Analyse the test sources with PMD - #218
Merged
Merged
Conversation
The test tree holds the harness that decides whether a security violation was detected,
and it was the one part of the code PMD never looked at.
Enabling it reported 102 findings, which sort into three kinds rather than one:
- 46 in fixtures. subject/, astTestFiles, example/student, org/apache/xyz and testuser
are test input that happens to be written in Java. An empty catch block there is the
behaviour under test, and MaliciousExceptionB discards the result of a forbidden file
read because that read is the attack. Cleaning any of it up would change what the
suite measures while leaving it green. Excluded in the ruleset, with that reasoning
recorded next to the patterns.
- 41 false positives: strictTimeoutTarget() carries an annotation read reflectively,
and three MockedStatic or ServerSocket resources exist to scope a mock or complete a
connection rather than to be read. Suppressed individually, each with its reason.
AvoidUsingHardCodedIP fires 38 times in four classes whose subject is the parsing of
loopback addresses, suppressed per class rather than by disabling the rule, which
would have disarmed it for main sources too.
- 15 real defects, fixed: five dead constants, one dead helper, three unused imports.
The last one was not dead code. ThreadTest.assertThreadErrorMessage took an
operationText the body never read while its javadoc claimed it was checked. Wiring it
up fails 14 of 20 tests, because the callers pass the console output the subject would
have printed ("Task 2 executed") while the message names the intercepted call ("tried
to illegally create Thread ... via ExecutorService.submit"). The parameter was never
the right thing to assert on, so it is gone from the signature and all 15 call sites,
and the note explaining that stays where the next person will find it.
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 |
github-actions
Bot
added
aop
Enabling PMD on the test sources surfaced 19 UnnecessaryImport findings, all false positives: the types they bring in (@Userbased, @UserTestResults, @Testtest and the LocaleUser/structural fixtures) are used exclusively as annotations or as .class literals, and PMD 7.17.0 does not count those uses against an on-demand import, so it reports the import as unused even though removing it breaks compilation. Replace the flagged wildcard imports with explicit single-type imports of the types each test actually uses. This removes the ambiguity that trips PMD while keeping compilation identical. ThreadTest loses its UserBased import outright, since its only @Userbased use is commented out.
added 3 commits
August 27, 2026 09:54
Running CPD over the test sources surfaces 11 cross-file duplication groups that are legitimate: the astTestFiles structural fixtures are deliberately similar (they are the subjects the structural analysis is checked against), and the parallel *PathAdviceTest / Essential*Test / forbidden-vs-architecture SystemAccessTest pairs mirror each other on purpose. Add these complete file sets to the accepted baseline; the within-file duplication is fixed in code instead.
Extract the repeated Mockito arrange blocks into shared private helpers so the same setup is written once per test class: - JavaWriterTest: stubArchitectureModeDefaults/stubAopModeDefaults/stubFileToolsDefaults/stubPhobosDefaults - JavaCreatorTest: common arrange moved to nested-class fields plus stubClasspathAndArchitecture - SecurityPolicyReaderAndDirectorTest: stubReaderAndDirectorChain - JavaWriter/Network/FileSystem extractor tests: sample permission list builders - WalaRuleTest: fold four near-identical CGNode builders into applicationNode Behaviour is unchanged: every affected test class still passes, and the verify(...) assertions keep their original specific argument matchers.
Run spotless:apply on the classes touched by the import and CPD-duplication changes so they satisfy the Static Code Analysis Spotless gate (line wrapping and import layout only, no behaviour change).
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
PMD never looked at the test tree, which is where the code that decides whether a violation was detected lives. It does now. Of 102 findings, 46 are in fixtures and excluded, 41 are false positives and suppressed individually, and 15 were real and are fixed. One of them was a test helper that ignored the value it claimed to check.
Linked issues
No linked issues
1. Problem
maven-pmd-pluginran withoutincludeTests, sosrc/test/javawas never analysed. That is the half of the repository that decides whether a security violation was detected, and therefore the half whose defects fail silently: a test that asserts nothing still passes.The clearest example found by turning it on:
ThreadTest.assertThreadErrorMessagetook anoperationTextparameter, documented it as "the specific operation text to check in the exception message", and never read it. Fifteen call sites passed distinct values. All fifteen were checked against the same hardcoded string instead.2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
The test tree is now held to the same rules as the main tree, so dead helpers, unused constants and swallowed results are reported rather than accumulated.
The boundary drawn here is not "main against test" but "assertion against the thing being asserted about". Fixtures are excluded in the ruleset, with the reasoning recorded next to the patterns: an empty catch block in the harness is a swallowed failure, while in a fixture it is the behaviour under test, and
MaliciousExceptionBdiscards the result of a forbidden file read because that read is the attack. Cleaning either up would change what the suite measures while leaving it green.4. Testing manual
Prerequisites
Steps
Not reproducible from an exercise. This changes the build and the test tree.
mvn test-compile pmd:check. The compile has to come first, because PMD reads the compiled classes to tell a used wildcard import from an unused one, so a baremvn pmd:checkon a fresh checkout reports every wildcard import as unused and fails. CI compiles before the goal for the same reason.mvn test -Punit-core-tests,coverage.mvn test -Pcoverage -Dtest=de.tum.cit.ase.ares.integration.ThreadTest.assertThreadErrorMessageinThreadTest.javaand the exclusion block in.settings/pmd-rules.xml.Expected result
Step 1 reports no violations. Step 2 reports 774 tests with no failures. Step 3 reports 20 tests with no failures. Step 4 shows why the parameter was removed rather than wired up, and which paths count as fixtures.
The parameter was removed rather than used because using it fails 14 of those 20 tests: the callers pass the console output the subject would have printed, such as "Task 2 executed", while the message names the intercepted call, for example "tried to illegally create Thread ... via ExecutorService.submit". The two were never the same thing.
Negative case (what must still be rejected)
The remaining assertion in
assertThreadErrorMessagemust still require "create Thread" in the message, so a thread-creation violation that stops being reported as such still fails the test. Confirm by reading the method: six substrings are still required.Excluding the fixture paths must not exclude the harness. Confirm by checking that
.settings/pmd-rules.xmllists onlysubject/,astTestFiles,example/student,org/apache/xyz,testuserand*Probe.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