From bdf056e96d98f2600bee95f451167d0689e24ea1 Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Thu, 29 Apr 2021 12:12:15 +0200 Subject: [PATCH 01/12] issue 3239 Implement best practice rule 'JUnit5TestShouldBePackagePrivate' Add the rule to check that JUnit5 tests methods and classes are package private. Add unit tests --- .../resources/category/java/bestpractices.xml | 70 +++++++++++++++++++ .../JUnit5TestShouldBePackagePrivateTest.java | 11 +++ .../xml/JUnit5TestShouldBePackagePrivate.xml | 63 +++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/JUnit5TestShouldBePackagePrivateTest.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index e8c1f71d4c..880e372345 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -802,6 +802,76 @@ public class MyTest { + + + + JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, @TestFactory, @TestTemplate or @ParameterizedTest. + + 3 + + + + + + + + + + + + + + + + + + + Public modifier is not necessary on a test method nor on the class + 2 + + + + + Package private modifiers are what is required + 0 + + + + + Non package private modifiers on all JUnit5 test types should be rejected + 6 + + + From 345f9bfd931205a12127766b6c9f69301ed78f86 Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Tue, 4 May 2021 13:22:04 +0200 Subject: [PATCH 02/12] Fix build AFAICT the site is generated from the XML rule file, so simply add an externalInfoUrl that points at where the rule should end up. Also put in the correct since date. --- pmd-java/src/main/resources/category/java/bestpractices.xml | 6 +++--- pmd-java/src/main/resources/rulesets/java/quickstart.xml | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 880e372345..2d28c7e7cb 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -802,13 +802,13 @@ public class MyTest { - + typeResolution="true" + externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#junit5testshouldbepackageprivate"> JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, @TestFactory, @TestTemplate or @ParameterizedTest. diff --git a/pmd-java/src/main/resources/rulesets/java/quickstart.xml b/pmd-java/src/main/resources/rulesets/java/quickstart.xml index b1b89a532b..23b2c85034 100644 --- a/pmd-java/src/main/resources/rulesets/java/quickstart.xml +++ b/pmd-java/src/main/resources/rulesets/java/quickstart.xml @@ -27,6 +27,7 @@ + From 6f11a61b4baa97d80766bd03240a35769fb3a9cc Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Tue, 4 May 2021 18:37:16 +0200 Subject: [PATCH 03/12] Fix example in rule --- pmd-java/src/main/resources/category/java/bestpractices.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 2d28c7e7cb..35e2cbd960 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -847,7 +847,7 @@ public class MyTest { Date: Tue, 4 May 2021 20:52:29 +0200 Subject: [PATCH 04/12] PR review Tweak descriptions, add unit test for Junit4... --- .../resources/category/java/bestpractices.xml | 37 +++++++------------ .../xml/JUnit5TestShouldBePackagePrivate.xml | 12 ++++++ 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 35e2cbd960..ece9af091f 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -804,22 +804,24 @@ public class MyTest { - JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, @TestFactory, @TestTemplate or @ParameterizedTest. +JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, +@TestFactory, @TestTemplate or @ParameterizedTest. +Contrary to JUnit4 tests that required public visibility to be run by the engine, JUnit5 tests may be run +only with package private visibility. Marking them as such is a good practice to limit their visibility. 3 - - - - + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml index 93117786fc..cebcb606a5 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml @@ -57,6 +57,18 @@ protected class MyTests { @ParameterizedTest @ValueSource(strings = {"Hello", "World"}) protected void testParameterized(final String value) { } +} + ]]> + + + + Public JUnit4 tests are not flagged + 0 + From c1b537247a857ac04ed101bf4dbbb5285b428361 Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Tue, 4 May 2021 21:26:11 +0200 Subject: [PATCH 05/12] Make rule report error on the method declaration rather than the body declaration --- pmd-java/src/main/resources/category/java/bestpractices.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index ece9af091f..9c51974c92 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -834,12 +834,12 @@ only with package private visibility. Marking them as such is a good practice to | //ClassOrInterfaceDeclaration /ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration - [MethodDeclaration[@PackagePrivate=false()]] [Annotation//Name[ pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') ]] + /MethodDeclaration[@PackagePrivate=false()] ]]> From bee6e3a2bc6ce42e92745e14864dc95bc1831799 Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Wed, 5 May 2021 20:44:54 +0200 Subject: [PATCH 06/12] PR review: merge parts of the rule --- .../resources/category/java/bestpractices.xml | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 9c51974c92..5c0f172d91 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -812,8 +812,8 @@ public class MyTest { JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, @TestFactory, @TestTemplate or @ParameterizedTest. -Contrary to JUnit4 tests that required public visibility to be run by the engine, JUnit5 tests may be run -only with package private visibility. Marking them as such is a good practice to limit their visibility. +Contrary to JUnit4 tests that required public visibility to be run by the engine, JUnit5 tests can also be run +if they're package-private. Marking them as such is a good practice to limit their visibility. 3 @@ -822,27 +822,27 @@ only with package private visibility. Marking them as such is a good practice to - + Date: Fri, 7 May 2021 14:29:34 +0200 Subject: [PATCH 07/12] Fix JUnit5 interfaces and add more UTs --- .../resources/category/java/bestpractices.xml | 1 + .../xml/JUnit5TestShouldBePackagePrivate.xml | 28 ++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 5c0f172d91..19a15c960d 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -823,6 +823,7 @@ if they're package-private. Marking them as such is a good practice to limit the Package private modifiers are what is required - 0 + 0 + + Public modifier is allowed on an abstract test classes + 0 + + + + + Public modifier is allowed on test interfaces + 0 + + + Non package private modifiers on all JUnit5 test types should be rejected 6 From 76af096687f0ac43b3626f5341fefc7989cdd8c8 Mon Sep 17 00:00:00 2001 From: Arnaud Jeansen Date: Mon, 10 May 2021 09:41:45 +0200 Subject: [PATCH 08/12] PR review: add expected line numbers --- .../rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml index de563e3cfb..afc474bab0 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml @@ -7,6 +7,7 @@ Public modifier is not necessary on a test method nor on the class 2 + 3,5 Non package private modifiers on all JUnit5 test types should be rejected 6 + 8,10,13,16,19,23 Date: Mon, 17 May 2021 09:03:00 +0200 Subject: [PATCH 09/12] Ignore private modifiers for both methods and inner classes --- .../resources/category/java/bestpractices.xml | 4 +-- .../xml/JUnit5TestShouldBePackagePrivate.xml | 35 +++++++++++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 19a15c960d..41cfb00807 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -832,14 +832,14 @@ if they're package-private. Marking them as such is a good practice to limit the ]] [MethodDeclaration] ]/( - self::*[@Abstract=false() and @PackagePrivate=false()] + self::*[@Abstract=false() and (@Public=true() or @Protected=true())] | ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration [Annotation//Name[ pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') ]] - /MethodDeclaration[@PackagePrivate=false()] + /MethodDeclaration[@Public=true() or @Protected=true()] ) ]]> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml index afc474bab0..9fdea7c27d 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml @@ -31,6 +31,35 @@ class MyTests { ]]> + + Private method modifiers should be reported by rule 'JUnit5TestNoPrivateModifier' and ignored by rule 'JUnit5TestShouldBePackagePrivate' + 0 + + + + + Private modifiers for inner classes should be reported by rule 'JUnit5TestNoPrivateModifier' and ignored by rule 'JUnit5TestShouldBePackagePrivate' + 0 + + + Public modifier is allowed on an abstract test classes 0 @@ -58,9 +87,9 @@ public interface MyTests { - Non package private modifiers on all JUnit5 test types should be rejected - 6 - 8,10,13,16,19,23 + Public and protected modifiers on all JUnit5 test types should be rejected + 5 + 8,10,13,19,23 Date: Fri, 21 May 2021 10:12:43 +0200 Subject: [PATCH 10/12] [java] Deprecate rule CloneThrowsCloneNotSupportedException Fixes #3112 --- docs/pages/release_notes.md | 13 +++++++++++++ .../src/main/resources/category/java/errorprone.xml | 7 +++++++ .../src/main/resources/rulesets/java/quickstart.xml | 2 +- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2d58181c1c..3c6abfe1ad 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -23,6 +23,18 @@ This is a {{ site.pmd.release_type }} release. Additionally comparisons against constants are allowed now. This makes the rule less noisy when two constants are compared. Constants are identified by looking for an all-caps identifier. +#### Deprecated rules + +* The Java rule {% rule "java/errorprone/CloneThrowsCloneNotSupportedException" %} has been deprecated without + replacement. + + The rule has no real value as `CloneNotSupportedException` is a + checked exception and therefore you need to deal with it while implementing the `clone()` method. You either + need to declare the exception or catch it. If you catch it, then subclasses can't throw it themselves explicitly. + However, `Object.clone()` will still throw this exception if the `Cloneable` interface is not implemented. + + Note, this rule has also been removed from the Quickstart Ruleset (`rulesets/java/quickstart.xml`). + ### Fixed Issues * apex @@ -53,6 +65,7 @@ This is a {{ site.pmd.release_type }} release. * [#2780](https://github.com/pmd/pmd/issues/2780): \[java] DataClass example from documentation results in false-negative * java-errorprone * [#3110](https://github.com/pmd/pmd/issues/3110): \[java] Enhance CompareObjectsWithEquals with list of exceptions + * [#3112](https://github.com/pmd/pmd/issues/3112): \[java] Deprecate rule CloneThrowsCloneNotSupportedException * [#3205](https://github.com/pmd/pmd/issues/3205): \[java] Make CompareObjectWithEquals allow comparing against constants * [#3248](https://github.com/pmd/pmd/issues/3248): \[java] Documentation is wrong for SingletonClassReturningNewInstance rule * [#3249](https://github.com/pmd/pmd/pull/3249): \[java] AvoidFieldNameMatchingTypeName: False negative with interfaces diff --git a/pmd-java/src/main/resources/category/java/errorprone.xml b/pmd-java/src/main/resources/category/java/errorprone.xml index 2bd70267bb..5797ddb216 100644 --- a/pmd-java/src/main/resources/category/java/errorprone.xml +++ b/pmd-java/src/main/resources/category/java/errorprone.xml @@ -1006,6 +1006,7 @@ public class Foo implements Cloneable { The method clone() should throw a CloneNotSupportedException. + +This rule is deprecated since PMD 6.35.0 without replacement. The rule has no real value as +`CloneNotSupportedException` is a checked exception and therefore you need to deal with it while +implementing the `clone()` method. You either need to declare the exception or catch it. If you catch it, +then subclasses can't throw it themselves explicitly. However, `Object.clone()` will still throw this +exception if the `Cloneable` interface is not implemented. 3 diff --git a/pmd-java/src/main/resources/rulesets/java/quickstart.xml b/pmd-java/src/main/resources/rulesets/java/quickstart.xml index b1b89a532b..7635f27bc5 100644 --- a/pmd-java/src/main/resources/rulesets/java/quickstart.xml +++ b/pmd-java/src/main/resources/rulesets/java/quickstart.xml @@ -205,7 +205,7 @@ - + From c2c26cbbb7026fa4f0f4b636ab92758248e07aa1 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 21 May 2021 10:46:55 +0200 Subject: [PATCH 11/12] [java] Deprecate rule DefaultPackage Fixes #3206 --- docs/pages/release_notes.md | 16 ++++++++++++++++ .../main/resources/category/java/codestyle.xml | 11 +++++++++++ .../main/resources/rulesets/java/quickstart.xml | 2 +- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2d58181c1c..aa18f4bfc0 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -23,6 +23,21 @@ This is a {{ site.pmd.release_type }} release. Additionally comparisons against constants are allowed now. This makes the rule less noisy when two constants are compared. Constants are identified by looking for an all-caps identifier. +#### Deprecated rules + +* The java rule {% rule "java/codestyle/DefaultPackage" %} has been deprecated in favor of + {% rule "java/codestyle/CommentDefaultAccessModifier" %}. + + The rule "DefaultPackage" assumes that any usage of package-access is accidental, + and by doing so, prohibits using a really fundamental and useful feature of the language. + + To satisfy the rule, you have to make the member public even if it doesn't need to, or make it protected, + which muddies your intent even more if you don't intend the class to be extended, and may be at odds with + other rules like {% rule "java/codestyle/AvoidProtectedFieldInFinalClass" %}. + + The rule {% rule "java/codestyle/CommentDefaultAccessModifier" %} should be used instead. + It flags the same thing, but has an escape hatch. + ### Fixed Issues * apex @@ -44,6 +59,7 @@ This is a {{ site.pmd.release_type }} release. * [#3254](https://github.com/pmd/pmd/issues/3254): \[java] AvoidReassigningParameters reports violations on wrong line numbers * java-codestyle * [#2655](https://github.com/pmd/pmd/issues/2655): \[java] UnnecessaryImport false positive for on-demand imports + * [#3206](https://github.com/pmd/pmd/issues/3206): \[java] Deprecate rule DefaultPackage * [#3262](https://github.com/pmd/pmd/pull/3262): \[java] FieldDeclarationsShouldBeAtStartOfClass: false negative with anon classes * [#3265](https://github.com/pmd/pmd/pull/3265): \[java] MethodArgumentCouldBeFinal: false negatives with interfaces and inner classes * [#3266](https://github.com/pmd/pmd/pull/3266): \[java] LocalVariableCouldBeFinal: false negatives with interfaces, anon classes diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 5515d5ef04..226ea0acb2 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -550,6 +550,7 @@ while (true) { // preferred approach Use explicit scoping instead of accidental usage of default package private level. The rule allows methods and fields annotated with Guava's @VisibleForTesting and JUnit 5's annotations. + +This rule is deprecated since PMD 6.35.0. It assumes that any usage of package-access is accidental, +and by doing so, prohibits using a really fundamental and useful feature of the language. + +To satisfy the rule, you have to make the member public even if it doesn't need to, or make it protected, +which muddies your intent even more if you don't intend the class to be extended, and may be at odds with +other rules like {% rule "java/codestyle/AvoidProtectedFieldInFinalClass" %}. + +The rule {% rule "java/codestyle/CommentDefaultAccessModifier" %} should be used instead. This rule flags +the same thing, but has an escape hatch. 3 diff --git a/pmd-java/src/main/resources/rulesets/java/quickstart.xml b/pmd-java/src/main/resources/rulesets/java/quickstart.xml index b1b89a532b..8b8614c811 100644 --- a/pmd-java/src/main/resources/rulesets/java/quickstart.xml +++ b/pmd-java/src/main/resources/rulesets/java/quickstart.xml @@ -90,7 +90,7 @@ - + From 4048b67dd7cf7a37041e115a8186311b71fc7828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 25 May 2021 13:12:12 +0200 Subject: [PATCH 12/12] Update release notes --- docs/pages/release_notes.md | 9 +++++++++ .../resources/category/java/bestpractices.xml | 15 +++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index fa4be8f336..7e967c8806 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -21,6 +21,15 @@ for parsing JavaScript code, requires at least Java 8. Therefore we decided to u module to Java 8 as well. This means that from now on, a Java 8 or later runtime is required in order to analyze JavaScript code. Note that PMD core still only requires Java 7. +#### New rules + +* The new Java rule {% rule "java/bestpractices/JUnit5TestShouldBePackagePrivate" %} + enforces the convention that JUnit 5 tests should have minimal visibility. + You can try out this rule like so: +```xml + +``` + #### Modified rules * The Java rule {% rule "java/errorprone/CompareObjectsWithEquals" %} has now a new property diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 6f1f8faf33..f18ed66aea 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -824,12 +824,15 @@ public class MyTest { class="net.sourceforge.pmd.lang.rule.XPathRule" typeResolution="true" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#junit5testshouldbepackageprivate"> - -JUnit 5 tests should be package private at the class level and for each method that uses @Test, @RepeatedTest, -@TestFactory, @TestTemplate or @ParameterizedTest. -Contrary to JUnit4 tests that required public visibility to be run by the engine, JUnit5 tests can also be run -if they're package-private. Marking them as such is a good practice to limit their visibility. - + 3