IsTestableMethod silently ignores @Test annotated methods that return a value

[george-hawkins]

IsTestableMethod silently ignores @Test annotated methods that return a value (or are static or are private).

IsTestClassWithTests uses IsTestMethod to discover tests. IsTestMethod is a subclass of IsTestableMethod that sets IsTestableMethod.mustReturnVoid to true.

If you annotate a method with @Test it's clear you want it run as a test. So if you've mistakenly annotated a method that doesn't conform to the requirements of a test then this should result in an error. It shouldn't be just a warning, it should actively fail the test run. However, at the moment such methods are simply silently ignored.

Steps to reproduce

Run the following test class, it contains four tests but only one of them will be run. There is no warning or indication that the other three have been skipped - they are silently ignored despite being annotated with @Test. One is ignored because it returns a value, one because it's static and one because it's private.

package example;

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class SilentlyIgnoredTests {
    @Test
    public void testNonIgnored() {
        assertEquals(42, 42);
    }

    @Test
    public boolean testIgnoredDueToReturnType() {
        assertEquals(42, 0);

        return true;
    }

    @Test
    public static void testIgnoredDueToStatic() {
        assertEquals(42, 0);
    }

    @Test
    private void testIgnoredDueToPrivate() {
        assertEquals(42, 0);
    }
}

This issue came up in Kotlin code where the value issue silently turned up when I made what I thought was an unproblematic change. I started with something like this:

import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test

class FooTests {
    @Test
    fun `foo test1`() = fooTest {
        assertEquals(42, 42)
    }

    private fun fooTest(block: () -> Unit) {
        block()
    }
}

Obviously fooTest does more than just invoke the passed in lambda but that's the core of it. We had a huge number of tests like this and everything worked fine. Then, in certain cases, I wanted to call fooTest more than once within a single test and I wanted it to return a result that could be checked in such tests (and ignored in all the existing tests). So I made this change, there were no warnings but suddenly many of our tests stopped running. They stopped because the existing tests now no longer returned void.

If you've annotated something with @Test you clearly want it run - it's a mistake if you've used @Test with something that can't be run as a test and it should fail the test run (printing a warning isn't enough - in a huge Maven build these things can get lost).

Context

• Used versions (Jupiter/Vintage/Platform):

org.junit.jupiter:junit-jupiter-api:jar:5.5.2:compile
org.junit.jupiter:junit-jupiter-api:jar:5.5.2:test
org.junit.jupiter:junit-jupiter-engine:jar:5.5.2:test
org.junit.jupiter:junit-jupiter-params:jar:5.5.2:test
org.junit.jupiter:junit-jupiter:jar:5.5.2:compile
org.junit.jupiter:junit-jupiter:jar:5.5.2:test
org.junit.platform:junit-platform-commons:jar:1.5.2:compile
org.junit.platform:junit-platform-engine:jar:1.5.2:compile
org.junit.platform:junit-platform-launcher:jar:1.5.2:compile

• Build Tool/IDE:

  • IntelliJ IDEA Community 2019.3.4
  • Apache Maven 3.6.3

Deliverables

I would suggest changing the logic for IsTestableMethod.test to something like this:

@Override
public boolean test(Method candidate) {
    if (!isAnnotated(candidate, this.annotationType)) {
        return false;
    }
    // Beyond this point you know the test annotation is present.

    // Please do not collapse the following into a single statement.
    if (isStatic(candidate)) {
        throw new JUnitException(candidate.getName() + " cannot be static");
    }
    if (isPrivate(candidate)) {
        throw new JUnitException(candidate.getName() + " cannot be private");
    }
    if (isAbstract(candidate)) {
        return false;
    }
    if (returnsVoid(candidate) != this.mustReturnVoid) {
        throw new JUnitException(candidate.getName() + " must return " + (this.mustReturnVoid ? "void" : "a value"));
    }

    return true;
}

[sbrannen]

Thanks for taking the time to submit such a detailed issue.

We are painfully aware of the lacking diagnostics for such configuration errors.

In fact, this is a duplicate of #242.

In light of that, I am closing this issue.

Feel free to comment on or vote for #242.

[george-hawkins]

Damn - now I feel silly, I didn't find the earlier issue and I put some effort into filing this properly 😢Thanks for directing me to #242.

[sbrannen]

Damn - now I feel silly, I didn't find the earlier issue and I put some effort into filing this properly 😢

No worries. It's not always easy to find related issues.

Thanks for directing me to #242.

Sure thing. I just cross-referenced this issue from there.