-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[java] Rename JUnit rules with overly restrictive names #4965
base: master
Are you sure you want to change the base?
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 |
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.
Thanks for restoring API compatibility :)
* `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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `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
->JUnitTestShouldUseAfterAnnotation
JUnit4TestShouldUseBeforeAnnotation
->JUnitTestShouldUseBeforeAnnotation
JUnit4TestShouldUseTestAnnotation
->UnitTestShouldUseTestAnnotation
JUnitAssertionsShouldIncludeMessage
->UnitTestAssertionsShouldIncludeMessage
JUnitTestContainsTooManyAsserts
->UnitTestContainsTooManyAsserts
JUnitTestsShouldIncludeAssert
->UnitTestsShouldIncludeAssert
Related issues
Ready?
./mvnw clean verify
passes (checked automatically by github actions)