Skip to content

[java] New rule: JUnit5TestShouldBePackagePrivate#3247

Merged
oowekyala merged 9 commits into
pmd:masterfrom
ajeans:jupiter
May 25, 2021
Merged

[java] New rule: JUnit5TestShouldBePackagePrivate#3247
oowekyala merged 9 commits into
pmd:masterfrom
ajeans:jupiter

Conversation

@ajeans

@ajeans ajeans commented May 4, 2021 •
edited by adangel
Loading

Copy link
Copy Markdown
Contributor

Add the rule to check that JUnit5 tests methods and classes are package private.
Add unit tests

Describe the PR

Propose a codestyle rule for Jupiter / JUnit5 tests according to the feedback received from @adangel in the issue.

Not sure how to add the 'externalInfoURL', so simply pointed at the rule name, assuming that the site is generated from the XML rule file

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

ajeans added 3 commits May 4, 2021 12:54
@ajeans
issue 3239 Implement best practice rule 'JUnit5TestShouldBePackagePri…
bdf056e 
…vate'

Add the rule to check that JUnit5 tests methods and classes are package
private.
Add unit tests
@ajeans
Fix build
345f9bf 
AFAICT the site is generated from the XML rule file, so simply add an externalInfoUrl that points at where the rule should end up.

Also put in the correct since date.
@ajeans
Fix example in rule
6f11a61
@oowekyala oowekyala changed the title issue 3239 Implement best practice rule 'JUnit5TestShouldBePackagePri… [java] New rule: JUnit5TestShouldBePackagePrivate May 4, 2021
@oowekyala oowekyala added this to the 6.35.0 milestone May 4, 2021
@oowekyala oowekyala added the a:new-rule Proposal to add a new built-in rule label May 4, 2021
@ghost

ghost commented May 4, 2021 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset changes 0 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset changes 0 violations,
introduces 45 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset changes 0 violations,
introduces 2268 new violations, 15 new errors and 0 new configuration errors,
removes 1877 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset changes 0 violations,
introduces 2268 new violations, 15 new errors and 0 new configuration errors,
removes 1877 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset changes 0 violations,
introduces 2268 new violations, 15 new errors and 0 new configuration errors,
removes 1877 violations, 0 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

oowekyala
oowekyala reviewed May 4, 2021
View reviewed changes

@oowekyala oowekyala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome and thanks for your work on this! I have a couple of suggestions for improvements, please take a look and push new commits when you can

Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread ...es/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/JUnit5TestShouldBePackagePrivate.xml
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml
ajeans added 2 commits May 4, 2021 21:22
@ajeans
PR review
a37f458 
Tweak descriptions, add unit test for Junit4...
@ajeans
Make rule report error on the method declaration
c1b5372 
rather than the body declaration
oowekyala
oowekyala reviewed May 5, 2021
View reviewed changes
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml Outdated
adangel
adangel reviewed May 8, 2021
View reviewed changes
Comment thread pmd-java/src/main/resources/category/java/bestpractices.xml
@adangel

adangel commented May 13, 2021

Copy link
Copy Markdown
Member

@ajeans This new rule detects two violations in the spring-framework: https://chunk.io/pmd/0094edd7ef3f417898be43865c65ec41/diff/spring-framework/index.html#section-violations

Both are private static inner classes. Are these valid cases?

Since they are private, I thought, they should only be picked up by the other (to be implemented) rule JUnit5TestNoPrivateModifier...
We should avoid to report these cases twice.

Besides that, these two cases might be a false positive for JUnit5TestNoPrivateModifier anyway - but probably not so easy to detect. It seems, that they use org.junit.platform.engine.discovery.DiscoverySelectors, which seems to allow private test classes...

So for this rule, I'd suggest to exclude private classes/methods, as this rule should only flag cases, where the class/method is public/protected. This should be possible by restricting the query additionally via [@Private=false()].

@ajeans

ajeans commented May 17, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the feedbacl @adangel

@ajeans This new rule detects two violations in the spring-framework: https://chunk.io/pmd/0094edd7ef3f417898be43865c65ec41/diff/spring-framework/index.html#section-violations

* https://github.com/spring-projects/spring-framework/tree/v5.0.6.RELEASE/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/FailingBeforeAndAfterMethodsSpringExtensionTestCase.java#L246

* https://github.com/spring-projects/spring-framework/tree/v5.0.6.RELEASE/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/FailingBeforeAndAfterMethodsSpringExtensionTestCase.java#L260

Both are private static inner classes. Are these valid cases?

It appears they are, those are inner static classes annotated with @Test and fed through a @TestFactory to test a JUnit extension.

Since they are private, I thought, they should only be picked up by the other (to be implemented) rule JUnit5TestNoPrivateModifier...
We should avoid to report these cases twice.

Agreed, I added two unit tests and changed the rule to target protected and public modifiers, and ignore private modifiers.

Besides that, these two cases might be a false positive for JUnit5TestNoPrivateModifier anyway - but probably not so easy to detect. It seems, that they use org.junit.platform.engine.discovery.DiscoverySelectors, which seems to allow private test classes...

That's using the jupiter engine dependency directly, so definitely advanced usage. The rule is targeted at devs that simply use the test annotations for simple and documented use cases. If you are doing the test discovery using an hardcoded list of test classes, you're way outside the junit user guide, and this rule doesn't make a lot of sense...
We can have this conversation when we implement the second rule in a separate MR but I would argue that this is an advanced usage and people should simply annotate the class to ignore the rule.

So for this rule, I'd suggest to exclude private classes/methods, as this rule should only flag cases, where the class/method is public/protected. This should be possible by restricting the query additionally via [@Private=false()].

Done with latest commit, hope this is what you had in mind.

adangel
adangel approved these changes May 18, 2021
View reviewed changes

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me!

@oowekyala oowekyala merged commit d33eab2 into pmd:master May 25, 2021
oowekyala added a commit that referenced this pull request May 25, 2021
@oowekyala
Update JUnit5TestShouldBePackagePrivate to pmd 7
03e9e84 
refs #3247
adangel added a commit that referenced this pull request May 25, 2021
@adangel
[doc] Update release notes (#3247, #3239)
993ae82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel approved these changes

@oowekyala oowekyala oowekyala approved these changes

Assignees

@oowekyala oowekyala

Labels

a:new-rule Proposal to add a new built-in rule

Projects

None yet

Milestone

6.35.0

Development

Successfully merging this pull request may close these issues.

[java] PMD could enforce non-public methods for Junit5 / Jupiter test methods

3 participants

@ajeans @adangel @oowekyala