Fix PMD violations for test code in Checkstyle #877

Closed
mkordas opened this Issue Mar 28, 2015 · 21 comments

Comments

Projects
6 participants
@mkordas
Contributor

mkordas commented Mar 28, 2015

Follow-up to #744.

Add <includeTests>true</includeTests> to PMD configuration.

Violations:

  • PMD Failure: com.puppycrawl.tools.checkstyle.DebugChecker:17 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com.puppycrawl.tools.checkstyle.DebugChecker:17 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com.puppycrawl.tools.checkstyle.DebugChecker:23 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com.puppycrawl.tools.checkstyle.DebugChecker:23 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com\puppycrawl\tools\checkstyle\UtilsTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com.puppycrawl.tools.checkstyle.api.AbstractViolationReporterTest:74 Rule:UnnecessaryFullyQualifiedName Priority:4 Unnecessary use of fully qualified name 'Assert.assertEquals' due to existing import 'org.junit.Assert'.
  • PMD Failure: com.puppycrawl.tools.checkstyle.api.AbstractViolationReporterTest:93 Rule:UnnecessaryFullyQualifiedName Priority:4 Unnecessary use of fully qualified name 'Assert.assertEquals' due to existing import 'org.junit.Assert'.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:43 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:43 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:53 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:53 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:66 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.ClassResolverTest:66 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\annotation\AnnotationUseStyleTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.coding.OneStatementPerLineCheckInput:50 Rule:UnusedPrivateField Priority:3 Avoid unused private fields such as 'one'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.coding.OneStatementPerLineCheckInput:55 Rule:UnusedPrivateField Priority:3 Avoid unused private fields such as 'two'..
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\coding\UnnecessaryParenthesesCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:97 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:97 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:197 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:197 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:212 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:212 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:228 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:228 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:243 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.header.HeaderCheckTest:243 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest:107 Rule:EmptyStatementNotInLoop Priority:3 An empty statement (semicolon) not part of a loop.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest:107 Rule:EmptyStatementNotInLoop Priority:3 An empty statement (semicolon) not part of a loop.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest:124 Rule:EmptyStatementNotInLoop Priority:3 An empty statement (semicolon) not part of a loop.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest:124 Rule:EmptyStatementNotInLoop Priority:3 An empty statement (semicolon) not part of a loop.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:11 Rule:UnusedPrivateField Priority:3 Avoid unused private fields such as 'mMissingJavadoc'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:121 Rule:UnnecessaryFullyQualifiedName Priority:4 Unnecessary use of fully qualified name 'java.io.IOException' due to existing import 'java.io.IOException'.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:130 Rule:UnusedLocalVariable Priority:3 Avoid unused local variables such as 'x'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:135 Rule:UnusedLocalVariable Priority:3 Avoid unused local variables such as 'z'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:140 Rule:UnusedPrivateField Priority:3 Avoid unused private fields such as 'ON_SECOND_LINE'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:151 Rule:UnnecessaryFullyQualifiedName Priority:4 Unnecessary use of fully qualified name 'java.io.IOException' due to existing import 'java.io.IOException'.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:156 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:156 Rule:UselessOverridingMethod Priority:3 Overriding method merely calls super.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.InputTags1:203 Rule:UnnecessaryFullyQualifiedName Priority:4 Unnecessary use of fully qualified name 'java.io.IOException' due to existing import 'java.io.IOException'.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_01:13 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_01:13 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_02:31 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_02:31 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_03:12 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.javadoc.Input_03:12 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocMethodCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocStyleCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocTypeCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocUtilsTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\javadoc\WriteTagCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.metrics.NPathComplexityCheckTest:63 Rule:UnusedLocalVariable Priority:3 Avoid unused local variables such as 'expectedComplexity'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.naming.ConstantNameCheckTest:44 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.naming.ConstantNameCheckTest:44 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.FileLengthCheckTest:80 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.FileLengthCheckTest:80 Rule:EmptyCatchBlock Priority:3 Avoid empty catch blocks.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput$PublicMethodsInnerInterface:49 Rule:UnusedModifier Priority:3 Avoid modifiers which are implied by the context.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput$PublicMethodsInnerInterface:54 Rule:UnusedModifier Priority:3 Avoid modifiers which are implied by the context.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput$PublicMethodsInnerInterface:59 Rule:UnusedModifier Priority:3 Avoid modifiers which are implied by the context.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput$PublicMethodsInnerInterface:64 Rule:UnusedModifier Priority:3 Avoid modifiers which are implied by the context.
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput:165 Rule:UnusedPrivateMethod Priority:3 Avoid unused private methods such as 'doNothing30()'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput:171 Rule:UnusedPrivateMethod Priority:3 Avoid unused private methods such as 'doNothing31()'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput:177 Rule:UnusedPrivateMethod Priority:3 Avoid unused private methods such as 'doNothing32()'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput:183 Rule:UnusedPrivateMethod Priority:3 Avoid unused private methods such as 'doNothing33()'..
  • PMD Failure: com.puppycrawl.tools.checkstyle.checks.sizes.MethodCountCheckInput:189 Rule:UnusedPrivateMethod Priority:3 Avoid unused private methods such as 'doNothing34()'..
  • PMD Failure: com\puppycrawl\tools\checkstyle\checks\sizes\MethodCountCheckTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.
  • PMD Failure: com\puppycrawl\tools\checkstyle\comments\CommentsTest.java:19 Rule:TooManyStaticImports Priority:3 Too many static imports may lead to messy code.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 30, 2015

Member

@mkordas , do you really think that is good idea ?

We do not do even Checkstyle validation on test sources, I never recommend to do that even in commercial projects (or use Checks that are mostly minors).
Tests are HACK/WORKAROUND by design, so there will never be good code. Supporting two configurations for main code and test is too much to my mind.

Member

romani commented Mar 30, 2015

@mkordas , do you really think that is good idea ?

We do not do even Checkstyle validation on test sources, I never recommend to do that even in commercial projects (or use Checks that are mostly minors).
Tests are HACK/WORKAROUND by design, so there will never be good code. Supporting two configurations for main code and test is too much to my mind.

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Apr 6, 2015

Contributor

I would disagree that tests are hacks/workarounds. In my commercial product we have enabled Checkstyle and PMD on the same level as in production code.

  • Checkstyle is about style - why your tests should follow different style than production code?
  • PMD is mainly about detecting programming mistakes - and why you wouldn't like to have mistakes in tests detected?
  • Unit tests can have bugs too. And errors are more visible when the code is well written.
  • Tests require maintenance on the same level as production code.
Contributor

mkordas commented Apr 6, 2015

I would disagree that tests are hacks/workarounds. In my commercial product we have enabled Checkstyle and PMD on the same level as in production code.

  • Checkstyle is about style - why your tests should follow different style than production code?
  • PMD is mainly about detecting programming mistakes - and why you wouldn't like to have mistakes in tests detected?
  • Unit tests can have bugs too. And errors are more visible when the code is well written.
  • Tests require maintenance on the same level as production code.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 6, 2015

Member

Checkstyle is about style

it is mostly like that, but it is competitor for PMD on problem(bad practice) detection.
I am ok to enforce extra rules on UTs, but lets do not be fanatic about that , example "Too many static imports may lead to messy code.", ..... some code have to be dirty.
We could support two types of configurations for test and prod code - but I doubt we can manage that now, but could be possible after we enforce all Cheksltyle's/PMDs/FindBugs validations over prod code.

So , for now I will not mark that issue as "approved" to stress that this is not a priority. Does it make sense ?

Member

romani commented Apr 6, 2015

Checkstyle is about style

it is mostly like that, but it is competitor for PMD on problem(bad practice) detection.
I am ok to enforce extra rules on UTs, but lets do not be fanatic about that , example "Too many static imports may lead to messy code.", ..... some code have to be dirty.
We could support two types of configurations for test and prod code - but I doubt we can manage that now, but could be possible after we enforce all Cheksltyle's/PMDs/FindBugs validations over prod code.

So , for now I will not mark that issue as "approved" to stress that this is not a priority. Does it make sense ?

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Apr 6, 2015

Contributor

Yeah, I agree with you, it's the second priority right now.
... and rule about static imports in my project is disabled both for tests and prod code, because we like them :)

Contributor

mkordas commented Apr 6, 2015

Yeah, I agree with you, it's the second priority right now.
... and rule about static imports in my project is disabled both for tests and prod code, because we like them :)

mkordas added a commit to mkordas/checkstyle that referenced this issue Aug 24, 2015

romani added a commit that referenced this issue Aug 25, 2015

@romani romani added the approved label Jan 3, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 7, 2016

Member

@MEZk , before doing some updates please think twice and if you are not sure please consult with me.

Too many static imports may lead to messy code

That is ok for Tests and they are locked to use Junit library, it has too much the same methods executions, so short names are implied that it is smth from Junit. We need to suppress that validation in test area.

Member

romani commented Sep 7, 2016

@MEZk , before doing some updates please think twice and if you are not sure please consult with me.

Too many static imports may lead to messy code

That is ok for Tests and they are locked to use Junit library, it has too much the same methods executions, so short names are implied that it is smth from Junit. We need to suppress that validation in test area.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 21, 2017

Contributor

After enabling pmd on tests found
SignatureDeclareThrowsException, 1510
AvoidDuplicateLiterals, 930
CommentRequired, 431
JUnitTestContainsTooManyAsserts, 200
TooManyStaticImports, 44
TooManyMethods, 22
CheckstyleCustomShortVariable, 19
ExcessiveMethodLength, 15
NcssMethodCount, 14
AccessorMethodGeneration, 12
UncommentedEmptyMethodBody, 8
ExcessiveImports, 7
AbstractNaming, 5
DefaultPackage, 4
AvoidUsingHardCodedIP, 4
CommentDefaultAccessModifier, 4
ConfusingTernary, 4
AppendCharacterWithChar, 3
UncommentedEmptyConstructor, 3
ExcessivePublicCount, 3
ExcessiveClassLength, 3
ShortClassName, 2
AccessorClassGeneration, 2
InsufficientStringBufferDeclaration, 2
AddEmptyString, 1
AvoidCatchingGenericException, 1
ShortMethodName, 1
CouplingBetweenObjects, 1
LoggerIsNotStaticFinal, 1
JUnit4TestShouldUseBeforeAnnotation, 1
JUnit4TestShouldUseTestAnnotation, 1
UselessOverridingMethod, 1
NPathComplexity, 1
NcssTypeCount, 1

I will divide them and make sequential pull requests

Contributor

Nimfadora commented Jun 21, 2017

After enabling pmd on tests found
SignatureDeclareThrowsException, 1510
AvoidDuplicateLiterals, 930
CommentRequired, 431
JUnitTestContainsTooManyAsserts, 200
TooManyStaticImports, 44
TooManyMethods, 22
CheckstyleCustomShortVariable, 19
ExcessiveMethodLength, 15
NcssMethodCount, 14
AccessorMethodGeneration, 12
UncommentedEmptyMethodBody, 8
ExcessiveImports, 7
AbstractNaming, 5
DefaultPackage, 4
AvoidUsingHardCodedIP, 4
CommentDefaultAccessModifier, 4
ConfusingTernary, 4
AppendCharacterWithChar, 3
UncommentedEmptyConstructor, 3
ExcessivePublicCount, 3
ExcessiveClassLength, 3
ShortClassName, 2
AccessorClassGeneration, 2
InsufficientStringBufferDeclaration, 2
AddEmptyString, 1
AvoidCatchingGenericException, 1
ShortMethodName, 1
CouplingBetweenObjects, 1
LoggerIsNotStaticFinal, 1
JUnit4TestShouldUseBeforeAnnotation, 1
JUnit4TestShouldUseTestAnnotation, 1
UselessOverridingMethod, 1
NPathComplexity, 1
NcssTypeCount, 1

I will divide them and make sequential pull requests

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 21, 2017

Member

@Nimfadora ,

SignatureDeclareThrowsException

http://pmd.sourceforge.net/pmd-4.3.0/rules/strictexception.html#SignatureDeclareThrowsException

should be suppressed with reason: "We do throws Exception, as any exception that in not caught in test should fail a test that is expected behavior and we like it as it follow fail-fast and helps to avoid extra catch blocks"

AvoidDuplicateLiterals

should be suppressed with reason: "it madness to move all string values to common variables, it will be hard to read test code"

CommentRequired

http://pmd.sourceforge.net/pmd-5.0.4/rules/java/comments.html#CommentRequired

suppress with comment: "we do not need javadoc on test code"

JUnitTestContainsTooManyAsserts

need to be investigated, probably need to be configured to allow more asserts.
Please provide report, and we will see.

TooManyStaticImports, 44

suppress with comment:
"we like static imports in UTs as them minimize the code and we know that most of such methods are from UT+moks frameworks"

TooManyMethods, 22

suppress with comment:
"we try to keep all testing for class in one place/class/file to ease navigation between target class and it's test"

==================

for all others please generate report and share with us.

Member

romani commented Jun 21, 2017

@Nimfadora ,

SignatureDeclareThrowsException

http://pmd.sourceforge.net/pmd-4.3.0/rules/strictexception.html#SignatureDeclareThrowsException

should be suppressed with reason: "We do throws Exception, as any exception that in not caught in test should fail a test that is expected behavior and we like it as it follow fail-fast and helps to avoid extra catch blocks"

AvoidDuplicateLiterals

should be suppressed with reason: "it madness to move all string values to common variables, it will be hard to read test code"

CommentRequired

http://pmd.sourceforge.net/pmd-5.0.4/rules/java/comments.html#CommentRequired

suppress with comment: "we do not need javadoc on test code"

JUnitTestContainsTooManyAsserts

need to be investigated, probably need to be configured to allow more asserts.
Please provide report, and we will see.

TooManyStaticImports, 44

suppress with comment:
"we like static imports in UTs as them minimize the code and we know that most of such methods are from UT+moks frameworks"

TooManyMethods, 22

suppress with comment:
"we try to keep all testing for class in one place/class/file to ease navigation between target class and it's test"

==================

for all others please generate report and share with us.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 21, 2017

Member

@Nimfadora , we need to find way to fix PMD issues gradually and without allowance to make such problems in code that already covered.
So my proposal:

  1. create a copy of pmd configuration and create second instance of pmd execution in pom.xml. First will be pmd config will be for main code but withtout test. Second will be only for test forlder. Plugin do have include/exclude options so that should be possible - https://maven.apache.org/plugins/maven-pmd-plugin/pmd-mojo.html .
  2. in test pmd config you suppress all and one PR by one PR remove suppressions from it.
  3. as you finish we can decided to keep pmd configs separate or merge.
Member

romani commented Jun 21, 2017

@Nimfadora , we need to find way to fix PMD issues gradually and without allowance to make such problems in code that already covered.
So my proposal:

  1. create a copy of pmd configuration and create second instance of pmd execution in pom.xml. First will be pmd config will be for main code but withtout test. Second will be only for test forlder. Plugin do have include/exclude options so that should be possible - https://maven.apache.org/plugins/maven-pmd-plugin/pmd-mojo.html .
  2. in test pmd config you suppress all and one PR by one PR remove suppressions from it.
  3. as you finish we can decided to keep pmd configs separate or merge.
@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 22, 2017

Contributor

@romani here is pmd report with tests included https://nimfadora.github.io/pmd-test/pmd.html

Contributor

Nimfadora commented Jun 22, 2017

@romani here is pmd report with tests included https://nimfadora.github.io/pmd-test/pmd.html

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 22, 2017

Member

@Nimfadora ,

Document empty method body
Overriding method merely calls super

PMD is right, should be fixed in our code .

This class has a bunch of public methods and attributes

suppress: "UTests are required to be public by design"

High amount of different objects as members denotes a high coupling

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

A high number of imports can indicate a high degree of coupling within an object.

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

Avoid really long classes.

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

JUnit tests should not contain more than 1 assert(s).

please reconfigure it to be 10 or any max value we have right now in code.

Do not hard code the IP address

suppress: "Checkstyle do not use IP in configuration, so all values in tests are ok to use, they just another test string data"

The Logger variable declaration does not contain the static and final modifiers

could be fixed, by moving first initialization to field declaration from public static void init()

StringBuffer constructor is initialized with size 16, but has at least 28 characters appended.

fix it.

Abstract classes should be named AbstractXXX

take reason to suppress from checkstyle suppressions.

Avoid if (x != y) ..; else ..;

should be fixed.

Avoid variables with short names that shorter than 2 symbols

should be fixed

JUnit 4 tests that execute tests should use the @test annotation

should be fixed

To avoid mistakes add a comment at the beginning of the clearCounter method if you want a default access modifier

weird code, need to be refactored.

The method testTokenNumbering() has an NCSS line count of 199

suppress certain violation: "this method is simple but long as it recheck each token one by one"

The method treeComments() has an NCSS line count of 113
Avoid really long classes.
Avoid really long methods.

suppress that violations for whole class ParseTreeBuilder, that is unfortunate but required to stay like this

Avoid appending characters as strings in StringBuffer.append.

fix it.

CheckUtil.java - Avoid catching generic exceptions such as NullPointerException, RuntimeException, Exception in try-catch block

suppress certian violation: "To avoid a lot of 'throws' signatures and catches, we do this global catch, but we rethrow RuntimeException to fail a test"


as you resolve this, please regenerate report again.
in each PRs, please share report link with violations that you fix, to let us recheck your intention one more time.
please send PR with suppressions and change values in pmd config - first.

Member

romani commented Jun 22, 2017

@Nimfadora ,

Document empty method body
Overriding method merely calls super

PMD is right, should be fixed in our code .

This class has a bunch of public methods and attributes

suppress: "UTests are required to be public by design"

High amount of different objects as members denotes a high coupling

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

A high number of imports can indicate a high degree of coupling within an object.

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

Avoid really long classes.

suppress: "We have too much UTs for each main class, it is better to keep all tests in one file to ease navigation and search for test"

JUnit tests should not contain more than 1 assert(s).

please reconfigure it to be 10 or any max value we have right now in code.

Do not hard code the IP address

suppress: "Checkstyle do not use IP in configuration, so all values in tests are ok to use, they just another test string data"

The Logger variable declaration does not contain the static and final modifiers

could be fixed, by moving first initialization to field declaration from public static void init()

StringBuffer constructor is initialized with size 16, but has at least 28 characters appended.

fix it.

Abstract classes should be named AbstractXXX

take reason to suppress from checkstyle suppressions.

Avoid if (x != y) ..; else ..;

should be fixed.

Avoid variables with short names that shorter than 2 symbols

should be fixed

JUnit 4 tests that execute tests should use the @test annotation

should be fixed

To avoid mistakes add a comment at the beginning of the clearCounter method if you want a default access modifier

weird code, need to be refactored.

The method testTokenNumbering() has an NCSS line count of 199

suppress certain violation: "this method is simple but long as it recheck each token one by one"

The method treeComments() has an NCSS line count of 113
Avoid really long classes.
Avoid really long methods.

suppress that violations for whole class ParseTreeBuilder, that is unfortunate but required to stay like this

Avoid appending characters as strings in StringBuffer.append.

fix it.

CheckUtil.java - Avoid catching generic exceptions such as NullPointerException, RuntimeException, Exception in try-catch block

suppress certian violation: "To avoid a lot of 'throws' signatures and catches, we do this global catch, but we rethrow RuntimeException to fail a test"


as you resolve this, please regenerate report again.
in each PRs, please share report link with violations that you fix, to let us recheck your intention one more time.
please send PR with suppressions and change values in pmd config - first.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 22, 2017

Member

Avoid autogenerated methods to access private fields and methods of inner / outer classes

http://pmd.sourceforge.net/snapshot/pmd-java/rules/java/design.html#AccessorMethodGeneration
should be disabled for Tests and for main code.
reason: "we do not care about this minor overhead, we are not Android application and we do not want to change visibility of methods. package-private visibility should be avoid almost always."

Member

romani commented Jun 22, 2017

Avoid autogenerated methods to access private fields and methods of inner / outer classes

http://pmd.sourceforge.net/snapshot/pmd-java/rules/java/design.html#AccessorMethodGeneration
should be disabled for Tests and for main code.
reason: "we do not care about this minor overhead, we are not Android application and we do not want to change visibility of methods. package-private visibility should be avoid almost always."

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 22, 2017

Contributor

@romani ,

So my proposal:
create a copy of pmd configuration and create second instance of pmd execution in pom.xml. First will be pmd config will be for main code but withtout test. Second will be only for test forlder. Plugin do have include/exclude options so that should be possible - https://maven.apache.org/plugins/maven-pmd-plugin/pmd-mojo.html .

What do you think if I will not create two instances of pmd executions?
Instead I will create 2 more rulesets - pmd-main.xml for main, pmd-test.xml for test. Both of them will include/inherite existing pmd.xml. In the pmd-main.xml I will exclude test files, And in the pmd-test.xml I will exclude main files and will supress all violations we want supress for test (Currently - all that fail build). In the pom.xml I will put pmd-main/test.xml instead of pmd.xml. In this way we will avoid "config duplication" and will have flexibility in configuration pmd for test and main files separatly. Cause, I think, the majority of violations you want to supress for tests, you will want to leave for main.

Contributor

Nimfadora commented Jun 22, 2017

@romani ,

So my proposal:
create a copy of pmd configuration and create second instance of pmd execution in pom.xml. First will be pmd config will be for main code but withtout test. Second will be only for test forlder. Plugin do have include/exclude options so that should be possible - https://maven.apache.org/plugins/maven-pmd-plugin/pmd-mojo.html .

What do you think if I will not create two instances of pmd executions?
Instead I will create 2 more rulesets - pmd-main.xml for main, pmd-test.xml for test. Both of them will include/inherite existing pmd.xml. In the pmd-main.xml I will exclude test files, And in the pmd-test.xml I will exclude main files and will supress all violations we want supress for test (Currently - all that fail build). In the pom.xml I will put pmd-main/test.xml instead of pmd.xml. In this way we will avoid "config duplication" and will have flexibility in configuration pmd for test and main files separatly. Cause, I think, the majority of violations you want to supress for tests, you will want to leave for main.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 22, 2017

Member

@Nimfadora ,

I am ok with this, if that works, my main point is to now mix configurations as tests config might be too relaxed for main code (test code should not lower quality level of main code.)

Member

romani commented Jun 22, 2017

@Nimfadora ,

I am ok with this, if that works, my main point is to now mix configurations as tests config might be too relaxed for main code (test code should not lower quality level of main code.)

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 22, 2017

Contributor

Abstract classes should be named AbstractXXX

take reason to suppress from checkstyle suppressions.

I didn't find that reason...

Contributor

Nimfadora commented Jun 22, 2017

Abstract classes should be named AbstractXXX

take reason to suppress from checkstyle suppressions.

I didn't find that reason...

@rnveach

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 22, 2017

Contributor

@rnveach ok, understood, thanks.

Contributor

Nimfadora commented Jun 22, 2017

@rnveach ok, understood, thanks.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2017

Member

from @Nimfadora :

There are some violations marked by me as tentative. You haven't mention them in your comments. I shared the report on them here.

Member

romani commented Jun 23, 2017

from @Nimfadora :

There are some violations marked by me as tentative. You haven't mention them in your comments. I shared the report on them here.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2017

Member

@Nimfadora ,

JUnit assertions should include a message

I know ,there are 351 cases ..... but this is very bad practice and it is always show laziness of engineers ..... . Please fix them all.
We will not be very demanding on quality of comments during your update, but try to keep message close to context.
Please do this in few PRs by 50-80 fixes per one PR.

Member

romani commented Jun 23, 2017

@Nimfadora ,

JUnit assertions should include a message

I know ,there are 351 cases ..... but this is very bad practice and it is always show laziness of engineers ..... . Please fix them all.
We will not be very demanding on quality of comments during your update, but try to keep message close to context.
Please do this in few PRs by 50-80 fixes per one PR.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

@Nimfadora , please share report of pmd violations that are disabled as "tentative" , I will review it and give you recommendations.

Member

romani commented Jun 24, 2017

@Nimfadora , please share report of pmd violations that are disabled as "tentative" , I will review it and give you recommendations.

@romani romani moved this from To Do to In Progress in Practice What You Preach Jun 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 29, 2017

Member

current model of PMD configs is ok.

Member

romani commented Jun 29, 2017

current model of PMD configs is ok.

@romani romani closed this Jun 29, 2017

@romani romani moved this from In Progress to Done in Practice What You Preach Jun 29, 2017

@romani romani moved this from Done to In Progress in Practice What You Preach Jun 29, 2017

@romani romani reopened this Jun 29, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jun 30, 2017

romani added a commit that referenced this issue Jun 30, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jun 30, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 3, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 3, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 3, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 4, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 4, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 4, 2017

romani added a commit that referenced this issue Jul 4, 2017

@romani romani added this to the 8.1 milestone Jul 4, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 4, 2017

Member

all fixes are merged.

Member

romani commented Jul 4, 2017

all fixes are merged.

@romani romani closed this Jul 4, 2017

@romani romani moved this from In Progress to Done in Practice What You Preach Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment