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] #940 Avoid JUnit 4 false positives for JUnit 5 tests #1256

Merged
merged 6 commits into from Aug 17, 2018

Conversation

Projects
None yet
4 participants
@vovkss
Contributor

vovkss commented Jul 25, 2018

Added JUnit5 annotations to XPath which identify JUnit methods (tests, setup, teardown), so that PMD doesn't report false positives for JUnit5 tests.

Fixes #940.

A minimal example of a false positive is the following JUnit5 test:

import org.junit.jupiter.api.Test;
public class MyTest {
    @Test
    public void testSomething() {
    }
}

PMD 6.5 reports: "JUnit 4 tests that execute tests should use the @test annotation".

@adangel adangel added this to the 6.7.0 milestone Jul 28, 2018

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Aug 4, 2018

@vovkss thanks for the PR! This looks great, but it's missing test cases. Each rule has a test data xml file (named after the rule) on which scenarios to be tested are described. For instance, the JUnit4TestShouldUseAfterAnnotation rule's test cases.

I'd also consider updating the descriptions for these rules. The rules are intended for JUnit 4. Having them work on both Junit 4 and 5 is sweet, but should be properly stated to users.

Add tests and enhance descriptions for JUnit4TestShouldUseBeforeAnnot…
…ation,JUnit4TestShouldUseAfterAnnotation (#940)
@pmd-bot

This comment has been minimized.

Contributor

pmd-bot commented Aug 6, 2018

1 Message
📖 Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected

Generated by 🚫 Danger

vovkss added some commits Aug 6, 2018

Add tests UseAssertEqualsInsteadOfAssertTrue,UseAssertNullInsteadOfAs…
…sertTrue,UseAssertSameInsteadOfAssertTrue,UnnecessaryBooleanAssertion (#940)
Add tests UseAssertEqualsInsteadOfAssertTrue,UseAssertNullInsteadOfAs…
…sertTrue,UseAssertSameInsteadOfAssertTrue,UnnecessaryBooleanAssertion (#940)
@vovkss

This comment has been minimized.

Contributor

vovkss commented Aug 6, 2018

@jsotuyod , thanks for your comment;
I've added the tests and extended descriptions to mention JUnit5.

@@ -459,7 +460,7 @@ JUnit 4 skips the tearDown method and executes all methods annotated with @After
<![CDATA[
//CompilationUnit[not(ImportDeclaration/Name[starts-with(@Image, "org.testng")])]
//ClassOrInterfaceBodyDeclaration[MethodDeclaration/MethodDeclarator[@Image='tearDown']]
[count(Annotation//Name[@Image='After'])=0]
[count(Annotation//Name[@Image='After' or @Image='AfterEach' or @Image='AfterAll'])=0]

This comment has been minimized.

@jsotuyod

jsotuyod Aug 17, 2018

Member

totally unrelated to this PR, but when merging we should use typeIs here /cc @adangel

This comment has been minimized.

@adangel

adangel Aug 17, 2018

Member

There are many rules here, that don't use typeresolution yet.... I can give it a try - if we are lucky, the test cases already import the correct classes....

@@ -497,7 +499,7 @@ JUnit 4 skips the setUp method and executes all methods annotated with @Before b
<![CDATA[
//CompilationUnit[not(ImportDeclaration/Name[starts-with(@Image, "org.testng")])]
//ClassOrInterfaceBodyDeclaration[MethodDeclaration/MethodDeclarator[@Image='setUp']]
[count(Annotation//Name[@Image='Before'])=0]
[count(Annotation//Name[@Image='Before' or @Image='BeforeEach' or @Image='BeforeAll'])=0]

This comment has been minimized.

@jsotuyod

@adangel adangel merged commit f2cb480 into pmd:master Aug 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Yay.
Details

adangel added a commit that referenced this pull request Aug 17, 2018

@adangel

This comment has been minimized.

Member

adangel commented Aug 17, 2018

@vovkss Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment