Revert changes to method search algorithm

[rdehuyss]

Hi there, I'm Ronald, the creator of JobRunr. When upgrading my dependencies to 5.10.1 from 5.10.0, I noticed a breaking change which was not declared in the release notes.

When using tests and inheritance (which I do for testing JobRunr against different DB's), I noticed that @Disabled was not taking into account anymore in v5.10.1 whereas that was the case in v5.10.0.

Steps to reproduce

Please see https://github.com/rdehuyss/junit-issue-disabled

If you run the test JUnitTest3, you notice that it fails. If you downgrade to 5.10.0, all is well again.

Context

• Used versions (Jupiter/Vintage/Platform): JUnit Jupiter
• Build Tool/IDE: both maven and gradle, Intellij

[rdehuyss]

Never mind, it has apparently been declared in 5.10.1... Three questions:

• is there a place where I can find the reasoning behind this?
• and would this change (if following semver and breaking) not validate a new major version change?
• as I used this feature a lot, how to solve this (instead of copying the @Disabled over and over)?

Thanks!

[rdehuyss]

Reopening as this really breaks a lot for me... Would you accept a PR for an annotation @DisabledAndInherited?

[sbrannen]

Never mind, it has apparently been declared in 5.10.1... Three questions:

What was declared in the release notes for 5.10.1 is that the documentation for the existing behavior was improved.

As far as I know, we did not intentionally change the behavior for @Disabled with regard to inheritance.

Are you saying your sample project demonstrates a change in behavior for @Disabled between 5.10.0 and 5.10.1?

[rdehuyss]

Hi @sbrannen! Thanks for getting back to me so quickly!

Are you saying your sample project demonstrates a change in behavior for @disabled between 5.10.0 and 5.10.1?

Yes

With 5.10.0, the JUnitTest3 is green, upgrading to 5.10.1 changes it to red.

[rdehuyss]

Relates to #3462

[rdehuyss]

Hi @sbrannen - where you able to verify the example project showing the change with @Disabled?

[marcphilipp]

Thanks for the reproducer! I can confirm that the behavior has indeed changed:

public abstract class AbstractJUnitTest {

    @Test
    protected void someTestMethod() {
        System.out.println("This should not run as disabled by AnstractJUnitTest2");
        throw new RuntimeException("Should not be thrown");
    }

    @Test
    protected void anotherTestMethod() {
        System.out.println("This should run");
    }
}

public abstract class AbstractJUnitTest2 extends AbstractJUnitTest {

    @Override
    @Disabled
    protected void someTestMethod() {
        super.someTestMethod();
    }
}

public class JUnitTest3 extends AbstractJUnitTest2{
}

In 5.10.0, JUnit3Test.someTestMethod is skipped, whereas 5.10.1 executes it. I think this is caused by 1752389.

[sbrannen]

If you rewrite AbstractJUnitTest2 as follows, someTestMethod() is properly disabled.

public abstract class AbstractJUnitTest2 extends AbstractJUnitTest {

	@Disabled
	@Test
	@Override
	protected void someTestMethod() {
		super.someTestMethod();
	}

}

If I recall correctly, we have never claimed in JUnit Jupiter that a @Test method can be overridden and still be considered a test method if the override does not redeclare @Test.

So, I consider that the expected behavior (or at lest the behavior we originally intended).

In light of that, I actually consider it a bug that the scenario without @Test worked before.

[sbrannen]

Here's a simplified all-in-one reproducer.

class DisabledReproTests extends AbstractJUnitTest {

	@Disabled
	// @Test
	@Override
	void someTestMethod() {
		super.someTestMethod();
	}

}

abstract class AbstractJUnitTest {

	@Test
	void someTestMethod() {
		throw new RuntimeException("Boom!");
	}

}

As has been pointed out, this scenario results in a test failure (unless you uncomment // @Test).

However, I think that should actually result in "no tests found", since DisabledReproTests overrides someTestMethod() without redeclaring that it is a @Test method.

So, let's discuss that behavior within the team.

[sbrannen]

After further analysis, I realized that @Disabled never had any influence on the scenarios discussed here, and I'll change the title to reflect that.

With 5.10.0 (and earlier versions), if you override a @Test method in a subclass and do not annotate the overriding method with @Test, you have effectively already disabled the test method.

The reason is that the overriding method will no longer be discovered as a @Test method, and consequently @Disabled has no effect. So you can safely delete @Disabled in such scenarios.

However, the change in 5.10.1 is in fact a regression since JUnit no longer "sees" the overriding method and therefore detects the overridden method in the super class as a valid @Test method which should not be the case.

In light of that, we have scheduled this for 5.10.2 and will investigate how best to address this in org.junit.platform.commons.util.ReflectionUtils.findAllMethodsInHierarchy(...).

[sbrannen]

For the sake of clarity for anyone following this issue...

In 5.10.0, JUnit3Test.someTestMethod is skipped,

In 5.10.0, it is not discovered as a test method and therefore is also not reported as skipped.

I think this is caused by 1752389.

That's correct.

[rdehuyss]

You guys are the best! Thanks for all the investigation...

[sbrannen]

Team Decision:

• In 5.10.2: revert the changes to our method search algorithm introduced in conjunction with #3498, #3500, and #3534.
• In 5.11.0: address the issue originally raised in #3498 when we resolve #3553.

[findepi]

With 5.10.0 (and earlier versions), if you override a @Test method in a subclass and do not annotate the overriding method with @Test, you have effectively already disabled the test method.

that's correct, but sometimes not desirable, especially that IDEs like IntelliJ make it easy to override a test method, but do not add @Test.
from that perspective, 5.10.1 had a nice behavior of overrides no longer being ignored. Is there a longer term plan to make 5.10.1's behavior available via configuration? is there an issue i should track?