[java] DetachedTestCase reports abstract methods

[ylexus]

Affects PMD Version:
6.14.0

Rule:
DetachedTestCase

Description:
Reports abstract methods. They are surely not detached test cases.

Code Sample demonstrating the issue:

public abstract class BaseTest {
    abstract void doSetup();
}

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]
Maven

[jsotuyod]

@ylexus abstract methods can be annotated if desired, so I see no reason to whitelist any abstract methods… if this was a utility, using protected would avoid the violation altogether. On this case however, this seems like a candidate to be annotated as @Before…

However, I'm unsure as to why the rule flags package-private methods… JUnit only allows for public methods, so a package-private can't be a test… at most it would be a candidate for CommentDefaultAccessModifier. Removing the check for package-private would also avoid the violation in your case.

[ylexus]

@jsotuyod sorry abstract methods can be annotated with what? Do you mean @SuppressWarnings? Sure we do that now, but clearly they are not detached test cases, they are template methods for subclass test cases to implement. I agree that these should really be protected.

[jsotuyod]

@ylexus I meant JUnit annotations such as @Before, @After and @Test. You can annotate a public method in the super class (even if abstract) and the annotation will be applied to overrides. The fact that the method is abstract should not change it's intended usage.

If it's not meant to be a JUnit method, then it should probably be protected or package-private.

[KroArtem]

Hello, I saw it's a good first issue and decided to fix it. First of all, the code in original post won't generate a violation, it requires an import of @Test and one method to be annotated with it.

Second, JUnit5 now supports package-private methods to be tests.

It's not clear why can't a method have an annotation? I mean, if I add @Description that adds description in a reporting tool like allure it won't generate a violation, though it looks like such method should be private anyway.

From my point of view, either this rule should stay as it is, or it should somehow take into account whether class is abstract or not. WDYT?

[ylexus]

This case was for a template method pattern applied to a hierarchy of unit tests. Say, a superclass asking a subclass to do something. A tool like pmd should not force me as a test designer to use or not use a particular legal pattern. Such abstract methods are legal for java and junit and are clearly not detached test cases.

[adangel]

I think, first thing we need, is a example, that reproduces the problem.

Is the following a complete sample of the problem?

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public abstract class BaseTest {
    @Test
    public void someTest() { }

    @BeforeEach
    abstract void setup(); // doesn't trigger DetachedTestCase because of @Before annotation

    @Test
    abstract void abstractTest(); // doesn't trigger DetachedTestCase

    public abstract void thisShouldAlsoBeATest(); // triggers DetachtedTestCase

    abstract void doSetup(); // this triggers DetachedTestCase

    abstract void doSetup(int param); // doesn't trigger DetachedTestCase because of param

    abstract void thisShouldBeATest();  // this triggers DetachedTestCase

    protected abstract void doSetup2(); // this doesn't trigger DetachedTestCase because protected
}

(Note: I didn't run it, so I'm not even sure it compiles or makes sense at all)

It seems to be, that we either could say: we don't report abstract methods - then we have a false-negative for thisShouldBeATest - or we keep the rule as is - then we have a false-positive for doSetup. Or same with package-private - report and we have a false-positive, don't report and we have a false-negative.

Is there any reliable way, we can determine, whether a method is indeed a detached test case?
We could of course add a property to consider/ignore abstract and/or package-private methods, but then, you'll need to configure the rule like you need it of course...

[ylexus]

Why do we have a false negative on thisShouldBeATest if we do not report abstract methods? On this class, it’s not a test. It’s only a test on the subclass implementing this class, if that class implements this test method.

[KroArtem]

First of all, @adangel is correct in terms of an example, everything is like you said.

First I wanted to say that abstract classes can be ignored by this rule but then thought about public methods that shouldn't exist in a base abstract class. If you want something outside of this package, it's better to create a utility class or helper or something else.

From the other point of view, why should PMD report about anything in abstract class? Just implement your methods in a child class and deal with your problems there. But in that case if you

@Override
void thisShouldBeATest

then there will be no violations (as it has an annotation). If you do not override it, there will be a violation (there is no annotation but in fact this method is overriden)

Actually I'm stuck.

[Debamoy]

Hello Devs, I think an abstract method should only be implemented on a child class level as a result it is perhaps not a good idea to annotate it with any JUnit annotations. I have raised a PR for fixing this please let me know your valuable thoughts #4706

[Debamoy]

This is my first contribution can I be assigned as an assignee @ylexus ?

[adangel]

With the solution @Debamoy provided, abstract methods are now not anymore reported.
This will result now in less violations reported (one can say, we have now false-negatives), as abstract methods, which are indeed detached test cases, won't be reported anymore. But it fixes the false-positive, that abstract methods, that are not detached test cases, are not reported now.
This will be solution until someone provides a better solution.

Note: this will be part of PMD 7.0.0.