Add Error Prone code style verification #632

vlsi · 2020-10-18T10:27:34Z

It allows to capture errors like mising switch case branches,
missing override, non-static inner class, etc

The verification can be run locally via ./gradlew -PenableErrorprone classes
Note: Java 11+ is required

The changes do not seem to be very big for me, however, it seems to flag important issues: it forbids the use of deprecated methods, it flags the use of the default encoding, it suggests to avoid LinkedList, and so on.

I fixed the warnings by running classes -PenableErrorprone -Pwerror=false --rerun-tasks in my IDE, so it printed all the warnings, then I could click on each error to navigate and resolve it.

The biggest commits are

91851b1 Replace LinkedList with ArrayList
89 files changed, 249 insertions(+), 241 deletions(-)

d668dd3 Remove uses of deprecated APIs
52 files changed, 154 insertions(+), 90 deletions(-)

be8438d Suppress DefaultCharset usage: either suppress the warning or use a charset
27 files changed, 85 insertions(+), 94 deletions(-)

pmouawad

Hello @vlsi ,
Thanks for this PR.
Just few notes.
Regards

pmouawad · 2020-10-18T13:13:39Z

src/bshclient/src/main/java/org/apache/jmeter/util/BeanShellClient.java

@@ -80,7 +82,7 @@ public static void main(String [] args) throws Exception{
    private static void sendLine( String line, OutputStream outPipe )
    throws IOException
    {
-        outPipe.write( line.getBytes() ); // TODO - charset?
+        outPipe.write(line.getBytes(StandardCharsets.UTF_8));


Are you sure about the potential breaks this might introduce ?

I believe there's a movement to use UTF-8 by default.

https://openjdk.java.net/jeps/8187041

Kotlin uses UTF-8 by default: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.io/java.io.-file/read-text.html

Of course, we can make it configurable, however, I believe, most of the times UTF-8 by default would be better (less chances to corrupt data) than use platform-default encoding.

src/jorphan/src/main/java/org/apache/log/ContextMap.java

codecov-io · 2020-10-18T21:37:48Z

Codecov Report

Merging #632 into master will increase coverage by 0.04%.
The diff coverage is 68.27%.

@@             Coverage Diff              @@
##             master     #632      +/-   ##
============================================
+ Coverage     55.30%   55.35%   +0.04%     
+ Complexity    10091    10088       -3     
============================================
  Files          1038     1036       -2     
  Lines         63858    63779      -79     
  Branches       7229     7212      -17     
============================================
- Hits          35314    35302      -12     
+ Misses        26049    25983      -66     
+ Partials       2495     2494       -1

Impacted Files	Coverage Δ	Complexity Δ
...n/java/org/apache/jmeter/util/BeanShellClient.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...va/org/apache/jmeter/assertions/HTMLAssertion.java	`20.56% <0.00%> (ø)`	`15.00 <0.00> (ø)`
...org/apache/jmeter/config/RandomVariableConfig.java	`71.42% <ø> (ø)`	`23.00 <0.00> (ø)`
...va/org/apache/jmeter/control/ModuleController.java	`70.12% <ø> (ø)`	`21.00 <0.00> (ø)`
...ctor/json/render/AbstractRenderAsJsonRenderer.java	`84.21% <0.00%> (ø)`	`18.00 <0.00> (ø)`
...er/extractor/json/render/RenderAsJsonRenderer.java	`91.30% <ø> (ø)`	`9.00 <0.00> (ø)`
...ava/org/apache/jmeter/modifiers/CounterConfig.java	`91.02% <ø> (ø)`	`35.00 <0.00> (ø)`
...va/org/apache/jmeter/modifiers/UserParameters.java	`64.00% <0.00%> (ø)`	`15.00 <0.00> (ø)`
...a/org/apache/jmeter/timers/PoissonRandomTimer.java	`72.97% <ø> (-5.41%)`	`9.00 <0.00> (-1.00)`
...issonarrivals/ConstantPoissonProcessGenerator.java	`58.57% <ø> (ø)`	`14.00 <0.00> (ø)`
... and 284 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d34a52...967ac65. Read the comment docs.

vlsi · 2020-10-18T21:40:00Z

@pmouawad , the PR passes tests, and it looks like it is ready for review and merge.

extras/startup.bsh

src/components/src/main/java/org/apache/jmeter/assertions/XMLAssertion.java

FSchumacher · 2020-10-21T09:19:58Z

My biggest concern with this PR at the moment is its size. Could we add the feature (errorprone) without changing every nag it shows? I tend to get bored when looking at big PRs and either skip parts, or abort looking at it altogether.
Apart from that, I am +1 on adding the feature (if it doesn't break the build when running with Java 8). (Haven't tried it)

vlsi · 2020-10-21T10:21:57Z

if it doesn't break the build when running with Java 8

All the previous CI jobs pass, so it works.
In practice, it is activated only in case -PenableErrorprone is added as a command-line option or gradle.properties setting.

Could we add the feature (errorprone) without changing every nag it shows?

Well, most of the warnings are true issues. There is an unfortunate case when the tool discourages the use of Hashtable, Enumeration, and LinkedList in the same check.
Nobody uses Hashtable nowadays unless that is required for an ancient API, however, people can easily use LinkedList without realizing it is way slower and less efficient than ArrayList and ArrayDeque

See https://errorprone.info/ It allows to capture errors like mising switch case branches, missing override, non-static inner class, etc The verification can be run locally via ./gradlew -PenableErrorprone classes Note: Java 11+ is required

…plerContext, DynamicClassLoader

JUnitAmbiguousTestClass is intentional there

Importing nested classes/static methods/static fields with commonly-used names can make code harder to read, because it may not be clear from the context exactly which type is being referred to See https://errorprone.info/bugpattern/BadImport

The code contains too many JavaDoc warnings

…harset

In all the cases Enumeration comes from Java interfaces, so we can't avoid it. Unfortunately error-prone does not recognize that yet. See google/error-prone#1646

It would be nice to migrate to java.time API, so the idea is to suppress the current usages, and use newer APIs for the new code.

error-prone requires the comment

Most of the times, the returned Future objects must not be ignored. DiskStoreSampleSender should probably be reworked to propagate exceptions.

See https://errorprone.info/bugpattern/InlineFormatString

See https://errorprone.info/bugpattern/MixedMutabilityReturnType

…e false positives See https://errorprone.info/bugpattern/UnnecessaryAnonymousClass

…Java API

… fails See https://errorprone.info/bugpattern/LockNotBeforeTry

Unfortunately, error-prone can't detect that false positive yet

int += long pattern hides a lossy cast, so it is better to write that explicitly

See https://errorprone.info/bugpattern/ProtectedMembersInFinalClass

vlsi force-pushed the errorprone branch 2 times, most recently from 3a512c6 to adbabaf Compare October 18, 2020 13:15

pmouawad requested changes Oct 18, 2020

View reviewed changes

vlsi force-pushed the errorprone branch 9 times, most recently from ec9db99 to 967ac65 Compare October 18, 2020 21:28

ham1 reviewed Oct 19, 2020

View reviewed changes

extras/startup.bsh Outdated Show resolved Hide resolved

ham1 reviewed Oct 19, 2020

View reviewed changes

src/components/src/main/java/org/apache/jmeter/assertions/XMLAssertion.java Outdated Show resolved Hide resolved

Add Error Prone code style verification

5ae71f6

See https://errorprone.info/ It allows to capture errors like mising switch case branches, missing override, non-static inner class, etc The verification can be run locally via ./gradlew -PenableErrorprone classes Note: Java 11+ is required

vlsi force-pushed the errorprone branch from 967ac65 to 8acd73c Compare October 31, 2020 11:09

vlsi added 6 commits October 31, 2020 14:14

Remove LoggingManager class (it has been deprecated since JMeter 3.2)

43217ec

Use UTF-8 encoding in BeanShellClient instead of platform-specific one

797f9e4

Add JavaDoc summaries to RandomVariableConfig, CounterConfig, JavaSam…

f6b1bfa

…plerContext, DynamicClassLoader

Suppress error-prone errors for junit-sample

f5e3d7a

JUnitAmbiguousTestClass is intentional there

Replace obsolete Vector and Hashtable with ArrayList and HashMap

ba94b52

Replace LinkedList with ArrayList

b3b2eec

vlsi force-pushed the errorprone branch 2 times, most recently from 8f65ce9 to 2421808 Compare October 31, 2020 11:28

vlsi added 2 commits October 31, 2020 15:58

Remove uses of deprecated APIs

db768c0

Remove excessive parenthesis

77d5d1b

vlsi added 28 commits October 31, 2020 15:58

Make constructors of abstract classes protected rather than public

9c92e99

Avoid import Map.Entry to avoid amboguity

7e42131

Importing nested classes/static methods/static fields with commonly-used names can make code harder to read, because it may not be clear from the context exactly which type is being referred to See https://errorprone.info/bugpattern/BadImport

Use lower-case field names for non-static fields in StatCalculator

93e27d7

Make enums immutable

dc4bdc0

Suppresss MissingSummary and EmptyBlockTag for now

63d791c

The code contains too many JavaDoc warnings

Suppress deprecation warnings for TestResultAction

599fcc9

Suppress deprecation warnings in SamplerMetricFixedModeTest

2fde32b

Suppress DefaultCharset usage: either suppress the warning or use a c…

80c3914

…harset

Fix InconsistentCapitalization warnings

e78352a

Suppress JdkObsolete warnings for Enumeration

369f651

In all the cases Enumeration comes from Java interfaces, so we can't avoid it. Unfortunately error-prone does not recognize that yet. See google/error-prone#1646

Remove unused variables

deb89ba

Suppress JdkObsolete warnings for usages of Date

3d133a8

It would be nice to migrate to java.time API, so the idea is to suppress the current usages, and use newer APIs for the new code.

Add missing @OverRide annotations

5f84f1e

Mark empty catch blocks with a comment

b1255aa

error-prone requires the comment

Suppress FutureReturnValueIgnored

d756670

Most of the times, the returned Future objects must not be ignored. DiskStoreSampleSender should probably be reworked to propagate exceptions.

Suppress Thread.yield usage

bb34632

Suppress JavaTimeDefaultTimeZone

c441e5b

Inline format specifiers when they are used only once

ac19a29

See https://errorprone.info/bugpattern/InlineFormatString

Resolve MixedMutabilityReturnType: make the returned lists unmodifiable

ecf72b7

See https://errorprone.info/bugpattern/MixedMutabilityReturnType

Suppress UnnecessaryAnonymousClass as the only two warnings seem to b…

8f5ca8d

…e false positives See https://errorprone.info/bugpattern/UnnecessaryAnonymousClass

Resolve SynchronizeOnNonFinalField

8ae3458

Suppress warnings for Hashtable usage when the class is required for …

9d7b694

…Java API

Move .lock() statements out of try to avoid accidetal release if lock…

a4c1ab8

… fails See https://errorprone.info/bugpattern/LockNotBeforeTry

Suppress StaticAssignmentInConstructor in Summarizer

ee0aead

Suppress JdkObsolete when using appendReplacement(StringBuffer,...)

7e2a47d

Unfortunately, error-prone can't detect that false positive yet

Suppress JavaLangClash for ThreadGroup

5fad27c

Avoid NarrowingCompoundAssignment warning

ebe65b8

int += long pattern hides a lossy cast, so it is better to write that explicitly

Make members of final classes package-private

150a275

See https://errorprone.info/bugpattern/ProtectedMembersInFinalClass

vlsi force-pushed the errorprone branch from 2421808 to 150a275 Compare October 31, 2020 12:58

vlsi merged commit 150a275 into apache:master Oct 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Error Prone code style verification #632

Add Error Prone code style verification #632

vlsi commented Oct 18, 2020 •

edited

Loading

pmouawad left a comment

pmouawad Oct 18, 2020

vlsi Oct 18, 2020 •

edited

Loading

codecov-io commented Oct 18, 2020

vlsi commented Oct 18, 2020

FSchumacher commented Oct 21, 2020

vlsi commented Oct 21, 2020

Add Error Prone code style verification #632

Add Error Prone code style verification #632

Conversation

vlsi commented Oct 18, 2020 • edited Loading

pmouawad left a comment

Choose a reason for hiding this comment

pmouawad Oct 18, 2020

Choose a reason for hiding this comment

vlsi Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 18, 2020

Codecov Report

vlsi commented Oct 18, 2020

FSchumacher commented Oct 21, 2020

vlsi commented Oct 21, 2020

vlsi commented Oct 18, 2020 •

edited

Loading

vlsi Oct 18, 2020 •

edited

Loading