SONARJAVA-5041 S5786 should raise an issue on JUnit5 annotated lifecycle methods with a public modifier#5004
Conversation
alban-auzeill
force-pushed
the
alban/SONARJAVA-5041
branch
from
4b6c339 to
b904577
Compare
alban-auzeill
force-pushed
the
alban/SONARJAVA-5041
branch
from
b904577 to
f36240a
Compare
|
dorian-burihabwa-sonarsource
left a comment
dorian-burihabwa-sonarsource
left a comment
There was a problem hiding this comment.
The change looks good overall but it looks like we don't have examples of issues raised on classes by S5786 with the new logic. It probably would help if we could get some documentation on the modifier scope enums to understand their usage
| 522 | ||
| ], | ||
| "org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/SslEngineBehaviorTest.java": [ | ||
| 39, |
There was a problem hiding this comment.
Line 39 matches the class declaration. Are changes to the rules supposed to change our behavior on classes in addition to methods?
| 634 | ||
| ], | ||
| "org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java": [ | ||
| 54, |
There was a problem hiding this comment.
There was a problem hiding this comment.
That was fun to review 😅
|
|
||
| raiseIssueOnMethods(junit5ClassMethods, ModifierScope.CLASS_METHOD); | ||
| raiseIssueOnMethods(junit5InstanceMethods, ModifierScope.INSTANCE_METHOD); | ||
| if (!junit5InstanceMethods.isEmpty()) { |
There was a problem hiding this comment.
Can we extract this for readability?
| if (!junit5InstanceMethods.isEmpty()) { | |
| boolean classHasJunit5InstanceMethods = !junit5InstanceMethods.isEmpty(); | |
| if (classHasJunit5InstanceMethods) { |
| if (!testMethods.isEmpty()) { | ||
| raiseIssueOnNotCompliantModifiers(classTree.modifiers(), false); | ||
| // Can we change the visibility of the class? | ||
| if (!hasPublicStaticMethods && !hasPublicStaticFields) { |
There was a problem hiding this comment.
I am not sure I like seeing doubling down on negatives here
| public enum ModifierScope { | ||
| CLASS, | ||
| CLASS_METHOD, | ||
| INSTANCE_METHOD | ||
| } |
There was a problem hiding this comment.
It would be very helpful to document how these enums represent and how they can be used
…cle methods with a public modifier
alban-auzeill
force-pushed
the
alban/SONARJAVA-5041
branch
from
f36240a to
f25ea9b
Compare
|
2 participants
SONARJAVA-5041