New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pmd: enable validation on IT java sources #4500

Closed
romani opened this Issue Jun 22, 2017 · 16 comments

Comments

Projects
3 participants
@romani
Member

romani commented Jun 22, 2017

right now all ITs are excluded
<excludeRoot>src/it</excludeRoot>

but exclude shuold be applied only to:
src/it/resourecs

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

we still need to review all tentative suppressions

Member

romani commented Jun 24, 2017

we still need to review all tentative suppressions

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 24, 2017

Contributor

@romani here is report

Contributor

Nimfadora commented Jun 24, 2017

@romani here is report

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 24, 2017

Member

Avoid unused private methods such as 'countTokens(AST,int)'.
Avoid unused private methods such as 'verifyWarns(Configuration,String,String...)'.
......

looks like false positives, recheck, if that is true - suppress certain violation.

JUnit tests should include assert() or fail()

please add at least one assertXXXX, what ever is easier to check but in context of test. As last resort recheck inner state by reflection.

CheckstyleAntTaskTest.java: Avoid instantiation through private constructors from outside of the constructors class.

it is very weird ... referenced line numbers are derected to empty lines and "}".

IndentationCheckTest.java : Avoid instantiation through private constructors from outside of the constructors class.

please make c-tor private IndentAudit(IndentComment... comments) { as public.

BlockCommentPositionTest.java: Avoid instantiation through private constructors from outside of the constructors class.
GeneratedJavaTokenTypesTest.java: .....

sources are not existing .... please fix.

ParenPadCheckTest.java: Avoid really long methods.

suppress, it is intended to keep all in one method.

Avoid using short method names

should be fixed.

Do not add empty strings

it was to make result as string type. but ok, lets use String c-tor.

ParseTreeBuilder.java: The type has an NCSS line count of 6181

suppress, we want to keep it as is.

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

suppress, "we want to avoid wide throws signature, it is just test"

JUnit 4 tests that set up tests should use the @before annotation

false-positive, please suppress.

XdocsPagesTest.java: The method validateCheckSection() has an NPath complexity of 434

please suppress, "we do not want invest extra time to make logic ideal in test method, may be later..."

ModuleReflectionUtilsTest.java

all should be fixed.

Member

romani commented Jun 24, 2017

Avoid unused private methods such as 'countTokens(AST,int)'.
Avoid unused private methods such as 'verifyWarns(Configuration,String,String...)'.
......

looks like false positives, recheck, if that is true - suppress certain violation.

JUnit tests should include assert() or fail()

please add at least one assertXXXX, what ever is easier to check but in context of test. As last resort recheck inner state by reflection.

CheckstyleAntTaskTest.java: Avoid instantiation through private constructors from outside of the constructors class.

it is very weird ... referenced line numbers are derected to empty lines and "}".

IndentationCheckTest.java : Avoid instantiation through private constructors from outside of the constructors class.

please make c-tor private IndentAudit(IndentComment... comments) { as public.

BlockCommentPositionTest.java: Avoid instantiation through private constructors from outside of the constructors class.
GeneratedJavaTokenTypesTest.java: .....

sources are not existing .... please fix.

ParenPadCheckTest.java: Avoid really long methods.

suppress, it is intended to keep all in one method.

Avoid using short method names

should be fixed.

Do not add empty strings

it was to make result as string type. but ok, lets use String c-tor.

ParseTreeBuilder.java: The type has an NCSS line count of 6181

suppress, we want to keep it as is.

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

suppress, "we want to avoid wide throws signature, it is just test"

JUnit 4 tests that set up tests should use the @before annotation

false-positive, please suppress.

XdocsPagesTest.java: The method validateCheckSection() has an NPath complexity of 434

please suppress, "we do not want invest extra time to make logic ideal in test method, may be later..."

ModuleReflectionUtilsTest.java

all should be fixed.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 26, 2017

Contributor

@romani

Avoid using short method names
should be fixed.

I cannot fix it, since this method is override of GeneratedJavaLexer
I guess I need to supress this violation for current case, am I right?

Contributor

Nimfadora commented Jun 26, 2017

@romani

Avoid using short method names
should be fixed.

I cannot fix it, since this method is override of GeneratedJavaLexer
I guess I need to supress this violation for current case, am I right?

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 26, 2017

Contributor

@romani CheckstyleAntTaskTesthas two strange test methodstestSetClasspath and testSetClasspathRef which were marked as temporary fake test
by @Vladlis. What assert should I add to these methods if they test nothing?

Contributor

Nimfadora commented Jun 26, 2017

@romani CheckstyleAntTaskTesthas two strange test methodstestSetClasspath and testSetClasspathRef which were marked as temporary fake test
by @Vladlis. What assert should I add to these methods if they test nothing?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

I cannot fix it, since this method is override of GeneratedJavaLexer
I guess I need to supress this violation for current case, am I right?

yes, suppress, nothing we can do against compiler.

What assert should I add to these methods if they test nothing?

try to remove them, if coverage is damages - you know what to check.
if nothing changed .... lets remove them completely.

Member

romani commented Jun 26, 2017

I cannot fix it, since this method is override of GeneratedJavaLexer
I guess I need to supress this violation for current case, am I right?

yes, suppress, nothing we can do against compiler.

What assert should I add to these methods if they test nothing?

try to remove them, if coverage is damages - you know what to check.
if nothing changed .... lets remove them completely.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 26, 2017

Member

I cannot fix it, since this method is override of GeneratedJavaLexer
nothing we can do

@romani We should report issue to PMD. Overridden methods should be ignored by the check.

Member

rnveach commented Jun 26, 2017

I cannot fix it, since this method is override of GeneratedJavaLexer
nothing we can do

@romani We should report issue to PMD. Overridden methods should be ignored by the check.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

yes, but please do this ones you finish upgrade of our code to finish the issue.
For now it should be suppressed no matter what.

Member

romani commented Jun 26, 2017

yes, but please do this ones you finish upgrade of our code to finish the issue.
For now it should be suppressed no matter what.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 26, 2017

Contributor

@romani one of pmd check conflicts with checkstyle rule -

In JUnit4, use the @test(expected) annotation to denote tests that should throw exceptions line 103

When I fixing it, I'm getting

Forbidden element 'expected' in annotation 'Test'.

Should I suppress it?

Contributor

Nimfadora commented Jun 26, 2017

@romani one of pmd check conflicts with checkstyle rule -

In JUnit4, use the @test(expected) annotation to denote tests that should throw exceptions line 103

When I fixing it, I'm getting

Forbidden element 'expected' in annotation 'Test'.

Should I suppress it?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

When I fixing it, I'm getting

that annotation is forbidden in our code, special Check is controlling this, if that is violation from PMD - suppress it.

         catch (IllegalStateException ex) {
             //expected
        }

please add validation for message in catch, we should expect exact exception, not an any of that time.

Member

romani commented Jun 26, 2017

When I fixing it, I'm getting

that annotation is forbidden in our code, special Check is controlling this, if that is violation from PMD - suppress it.

         catch (IllegalStateException ex) {
             //expected
        }

please add validation for message in catch, we should expect exact exception, not an any of that time.

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

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

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

Items that are left:

        <!--tentative-->
        <exclude name="AccessorClassGeneration"/>
        <!--tentative-->
        <exclude name="NonThreadSafeSingleton"/>
Member

romani commented Jun 26, 2017

Items that are left:

        <!--tentative-->
        <exclude name="AccessorClassGeneration"/>
        <!--tentative-->
        <exclude name="NonThreadSafeSingleton"/>
@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jun 26, 2017

Contributor

@romani the report on them is here
Avoid instantiation through private constructors from outside of the constructors class.
needs to be suppressed, in my opinion.
Second one is controversial.

Contributor

Nimfadora commented Jun 26, 2017

@romani the report on them is here
Avoid instantiation through private constructors from outside of the constructors class.
needs to be suppressed, in my opinion.
Second one is controversial.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 26, 2017

Member

NonThreadSafeSingleton -

please suppress for now, "will be addressed in https://github.com/checkstyle/checkstyle/projects/5 "

AccessorClassGeneration

it is on to make c-tor as public for private internal classes.

     private static final class MessageLevelPair {
         private final String msg;
         private final int level;
 
         private MessageLevelPair(String msg, int level) { //<!--- make it public
Member

romani commented Jun 26, 2017

NonThreadSafeSingleton -

please suppress for now, "will be addressed in https://github.com/checkstyle/checkstyle/projects/5 "

AccessorClassGeneration

it is on to make c-tor as public for private internal classes.

     private static final class MessageLevelPair {
         private final String msg;
         private final int level;
 
         private MessageLevelPair(String msg, int level) { //<!--- make it public
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 28, 2017

Member

from Nimfadora:
renewed report by this link

Member

romani commented Jun 28, 2017

from Nimfadora:
renewed report by this link

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 28, 2017

Member

@Nimfadora ,

TreeWalkerTest.java::testProcessNonJavaFiles

just put in config some Check and make it execute on file that have no violations and assert that Check have no events after execution, see example at testProcessWithRecognitionException .
The same fix is required for other tests.

CheckstyleAntTaskTest.java:: testDefaultFlawless

just put some check for execution and validate that Check is finished without violations.
Example: testPathsDirectoryWithNestedFile

TranslationCheckTest.java::testLogIoException

recheck that dispatcher got event - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/TranslationCheck.java#L523

DeclarationOrderCheckTest.java::testTokensNotNull

set collection to check and recheck messages/violations after visit token.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java#L117

NPathComplexityCheckTest.java::testDefaultHooks

set collection to check and recheck messages/violations after visit token.


please apply the same principles to other tests.

Member

romani commented Jun 28, 2017

@Nimfadora ,

TreeWalkerTest.java::testProcessNonJavaFiles

just put in config some Check and make it execute on file that have no violations and assert that Check have no events after execution, see example at testProcessWithRecognitionException .
The same fix is required for other tests.

CheckstyleAntTaskTest.java:: testDefaultFlawless

just put some check for execution and validate that Check is finished without violations.
Example: testPathsDirectoryWithNestedFile

TranslationCheckTest.java::testLogIoException

recheck that dispatcher got event - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/TranslationCheck.java#L523

DeclarationOrderCheckTest.java::testTokensNotNull

set collection to check and recheck messages/violations after visit token.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java#L117

NPathComplexityCheckTest.java::testDefaultHooks

set collection to check and recheck messages/violations after visit token.


please apply the same principles to other tests.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 29, 2017

Member

fix is mered , last commit reference changes in non-IT area

Member

romani commented Jun 29, 2017

fix is mered , last commit reference changes in non-IT area

@romani romani closed this Jun 29, 2017

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

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

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