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
dependency: bump pmd.version to 7.1.0 and maven-pmd-plugin to 3.22.0 #14856
Conversation
d6a8905
to
3ee17aa
Compare
Google is your friend. According to https://stackoverflow.com/a/78330292/1016482 , 6 and 7 are not 1 to 1 compatible and there is a migration guide. I don't know any other specifics or how to help. You could try setting a breakpoint where the exception occurs and see if you can identify anything in the code. |
3ee17aa
to
568aba9
Compare
config/pmd.xml
Outdated
<rule ref="category/java/design.xml/NcssCount"> | ||
<properties> | ||
<property name="classReportLevel" value="1000" /> | ||
<!-- *TokenTypes are special classes that are big due to a lot of description comments. | ||
RequireThisCheck has to work with and identify many frames. | ||
JavadocMethodCheck is in the process of being rewritten. | ||
SiteUtil provides a lot of functionality to generate documentation. --> | ||
<property name="violationSuppressXPath" | ||
value="//ClassOrInterfaceDeclaration[@SimpleName='JavadocTokenTypes' | ||
or @SimpleName='TokenTypes' or @SimpleName='RequireThisCheck' | ||
or @SimpleName='JavadocMethodCheck' or @SimpleName='JavaAstVisitor' | ||
or @SimpleName='SiteUtil']"/> | ||
</properties> | ||
</rule> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added instead of ExcessiveClassLength rule that was removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All properties that accept multiple values now uses ( , ) as a delimiter not ( | )
the build started successfully but somehow the violationSuppressXPath is not working as before So, we have 604 pmd failures 💀 . I will look more into that pmd docs: https://pmd.github.io/pmd/pmd_userdocs_suppressing_warnings.html#the-property-violationsuppressxpath
|
@rnveach are we ok to update all violationSuppressXPath queries to match the new version used by pmd? I have no idea about this new version or the amount of modification that should be done yet. waiting for your opinion first before proceeding |
We won't be able to merge a red CI, so how would you get this to a point where it can be merged without updating everything related to the PMD upgrade? |
568aba9
to
4822bac
Compare
Maybe I'm missing something, but this is based on my findings so far. |
@mahfouz72 Let's do a little more investigation, then open an issue with your findings. We can call it something like "Migrate to PMD 7". Please explain new checks, new AST, etc. |
256f277
to
c439dc0
Compare
what I have done so far:
|
is it likely that all of these are false positives?
|
You would have to look at a few and determine if they make no sense, or if they are valid. I would also check before your PR if there was a suppression for it before.
I just picked one and this looks valid to me.
Same with this. |
all are false positives they have no more than 11 assert |
I think JUnitTestContainsTooManyAsserts has always been questionable if they are true violations or false. Report these cases to PMD and I would suppress them. |
Keep in mind, we do not have to update to bleeding edge, we can wait for 1 year until everything is calmed down in new version of pmd, to have less problems. |
@mahfouz72 This seems like an all or nothing PR. If we delay this for year(s), it could become even harder to work with (or come back to ) when GSoC is over. I think we should consider this PR carefully if we can finish it before we start working on the project, and if we can't, then we should probably close it or mark it as abandoned. You still have other PRs on your plate that need to be wrapped up before starting the project. |
I think it will be difficult to look at all 66 violations before GSoC. and also I am not sure what exactly needs to be done to finish this PR. |
I would move any valid ones to a new issue where the list can be broken up easier so its not all on this single PR. But otherwise yes, false ones should be reported. I am also ok to suppress everything and have everything be in the new issue to confirm if it is valid or not and how to fix them. |
@romani @nrmancuso what do you think? are we ok to suppress all violations from #14856 (comment) to make PMD happy then merge this PR and open a new issue to take our time to investigate the list and take the appropriate action for each violation |
Yes, we always migrate tools like this, all new stuff goes to suppression until some ticket. At least new violations will not leak to our codebase. |
c439dc0
to
e8d9879
Compare
suppressions for all violations are added in a separate commit we should put https://gist.github.com/mahfouz72/d199fca33ce2c9e8d84be3882403093c and 8da7fc0 in the issue |
e8d9879
to
8da7fc0
Compare
please open issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items:
* @noinspection PackageVisibleInnerClass | ||
* @noinspectionreason PackageVisibleInnerClass - we keep this enum package visible for tests | ||
*/ | ||
enum OutputFormat { | ||
/* package */ enum OutputFormat { | ||
/** XML output format. */ | ||
XML, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets put this to new issue to review, kind of strange that not making it public, we usually do not use package-private just for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
656bada
to
5a1f41e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last:
5a1f41e
to
c582b7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge
@romani @nrmancuso we can close #14832 and #14835 |
Done, thanks for the reminder |
@@ -818,7 +818,7 @@ public void testClearState() throws Exception { | |||
assertWithMessage("State is not cleared on beginTree") | |||
.that(TestUtil.isStatefulFieldClearedDuringBeginTree(check, | |||
staticImport.orElseThrow(), "lastImportStatic", | |||
lastImportStatic -> !((boolean) lastImportStatic))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahfouz72 @nrmancuso @romani If this parenthesis is unnecessary, then why didn't our check ping it?
https://checkstyle.org/checks/coding/unnecessaryparentheses.html#UnnecessaryParentheses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to create issue on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that this is because we using rules that were removed in the latest pmd releases but even after the removal from pmd.xml, the issue is not fixed.
any ideas for why we got this XML validation error?