[java] Rename JUnit rules with overly restrictive names #4965
Conversation
- Call it JUnitTestShouldUseBeforeAnnotation as it applies to JUnit 4 and 5. - Improve the doc to clarify it's intended use.
- Call it JUnitTestShouldUseAfterAnnotation instead as it not only applies to JUnit4 - Improve the doc to further clarify it's usages
- The rule is now called UnitTestAssertionsShouldIncludeMessageRule as it applies to JUnit and TestNG. - The doc is updated to reflect this.
- The rule is now called UnitTestContainsTooManyAssertsRule as it checks for JUnit and TestNG. - This is further cleared up in the documentation.
- It's now called UnitTestsShouldIncludeAssertRule as it applies to JUnit and TestNG - The doc is updated to reflect this
- The rule is now called UnitTestShouldUseTestAnnotation as it applies to both JUnit and TestNG. - The doc is further improved to reflect this.
This comment was marked as resolved.
This comment was marked as resolved.
- Don't break API compatibility, and set everything for removal in PMD 8
Generated by 🚫 Danger |
| * `java/bestpractices/JUnit4TestShouldUseAfterAnnotation` has been renamed to {% rule java/bestpractices/JUnitTestShouldUseAfterAnnotation %} | ||
|
|
||
| * `java/bestpractices/JUnit4TestShouldUseBeforeAnnotation` has been renamed to {% rule java/bestpractices/JUnitTestShouldUseBeforeAnnotation %} |
There was a problem hiding this comment.
Any reason why these two rules are named "JUnitTestShould..." and not just "UnitTestShould..."?
At least, we also have some testng annotation in our xpath expression (not sure, if that makes sense...)
We could argue, that an unexperienced TestNG user (coming from JUnit 3) could maybe write a method called "setUp" but maybe forget the "BeforeMethod" annotation. Which I would consider a valid violation for this rule. But we probably don't report this at the moment, because we are explicitly restricting the rule to JUnit4. Which means, it would be an enhancement (which could be a separate PR), to support Junit5 and TestNG here as well...
Overall, I'd suggest to also name these rules without the "J" to have all unit test related rules named similar.
There was a problem hiding this comment.
Any reason why these two rules are named "JUnitTestShould..." and not just "UnitTestShould..."?
At least, we also have some testng annotation in our xpath expression (not sure, if that makes sense...)
TBH, I hadn't realized the rules include the TestNG annotations… but looking at it, they will never work, both rules still add a [../MethodDeclaration[pmd-java:hasAnnotation('org.junit.Test')]] condition, that is bound to JUnit (and JUnit 4 for the matter, so looking at JUnit 5 annotations doesn't really work either even if the doc claimed otherwise).
I'll move this back to draft and rework these rules to properly cover all claimed cases + TestNG. It should be small enough of a change.
|
|
||
| Several rules for unit testing have been renamed to better reflect their actual scope. Lots of them were called after JUnit / JUnit 4, even when they applied to JUnit 5 and / or TestNG. | ||
|
|
||
| * `java/bestpractices/JUnit4TestShouldUseAfterAnnotation` has been renamed to {% rule java/bestpractices/JUnitTestShouldUseAfterAnnotation %} |
There was a problem hiding this comment.
| * `java/bestpractices/JUnit4TestShouldUseAfterAnnotation` has been renamed to {% rule java/bestpractices/JUnitTestShouldUseAfterAnnotation %} | |
| * {% deleted_rule java/bestpractices/JUnit4TestShouldUseAfterAnnotation %} has been renamed to {% rule java/bestpractices/JUnitTestShouldUseAfterAnnotation %} |
Maybe we should use deleted_rule here? Not sure, how it is being displayed....
Describe the PR
A bunch of rules were either called after JUnit even though they also apply to TestNG, or after JUnit4, even though they apply to JUnit5. We rename them to be more self-descriptive.
Documentation is improved to reflect this.
Summary of changes:
JUnit4TestShouldUseAfterAnnotation->JUnitTestShouldUseAfterAnnotationJUnit4TestShouldUseBeforeAnnotation->JUnitTestShouldUseBeforeAnnotationJUnit4TestShouldUseTestAnnotation->UnitTestShouldUseTestAnnotationJUnitAssertionsShouldIncludeMessage->UnitTestAssertionsShouldIncludeMessageJUnitTestContainsTooManyAsserts->UnitTestContainsTooManyAssertsJUnitTestsShouldIncludeAssert->UnitTestsShouldIncludeAssertRelated issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)