Fixes #1189: JUnit4TestAdapter blocks JUnit-3-style suites that are included via @RunWith(Suite.class)

[mkeller]

No description provided.

[marcphilipp]

The indentation seems off here.

[marcphilipp]

@dsaff Do you have time to take a look at this?

[kcooney]

I worry that this change may have unintended consequences. For one, if someone subclassedAllDefaultPossibilitiesBuilder this change would break them.

I am having problems wrapping my head around the problem (possibly due to lack of sleep :-). Can you include a simple reproducible test case in the bug?

[kcooney]

As far as I can tell, the only way canUseSuiteMethod can be false is if you either explicitly created AllDefaultPossibilitiesBuilder and passed in false, or if you used Request.classWithoutSuiteMethod(). In either case, not using the suite method sounds like expected behavior. Am I missing something?

Edit: I see it now. JUnit4TestAdapter uses Request.classWithoutSuiteMethod().

I think a safer way would be to fix this in ClassRequest.getRunner(). In that method if canUseSuiteMethod is false, it can return a subclass of AllDefaultPossibilitiesBuilder that overrides suiteMethodBuilder() as follows:

protected RunnerBuilder suiteMethodBuilder() {
  RunnerBuilder delegateRunnerBuilder = super.suiteMethodBuilder();
  return new RunnerBuilder() {
    public Runner runnerForClass(Class<?> testClass) throws Throwable {
      if (testClass == fTestClass) {
        return NullBuilder();
      }
      return delegateRunnerBuilder.runnerForClass(testClass);
    }
  };
}

Even this is likely to break someone, so I suggest we apply the fix to the junit5 branch

[mkeller]

AllDefaultPossibilitiesBuilder is clearly an internal class without any API, so nobody playing by the rules can be broken by my proposed fix.

There is no use case for the current behavior of new AllDefaultPossibilitiesBuilder(false). It's just an implementation bug that should be fixed in JUnit 4. The Javadoc of Request#classWithoutSuiteMethod() supports that view by saying that it "will run all the tests in a class. If the class has a suite() method, it will be ignored."
=> Only the suite method of the given class should be ignored.

The only reason why that special request exists is to avoid the endless nesting that would otherwise occur if a test class uses the JUnit4TestAdapter in its suite() method (according to the API of JUnit4TestAdapter, see the added Javadoc in my fix).

[mkeller]

If you don't like the additional AllDefaultPossibilitiesBuilder or the additional RunnerBuilder, you can also take this variant, which solves the problem by replacing the bad AllDefaultPossibilitiesBuilder#canUseSuiteMethod with a field that holds the class for which to block the suite() method:
mkeller@5fea7c6

[kcooney]

@mkeller Unfortunately, many people directly use JUnit internal classes. With just a few seconds of searching I found this: http://www.programcreek.com/java-api-examples/index.php?api=org.junit.internal.builders.AllDefaultPossibilitiesBuilder

We have a very large number of users, so a breaking change that should affect no one will affect someone.

What's wrong with my proposal?

[mkeller]

What's wrong with my proposal?

It could probably be cleaned up and would work, but it would just be wrong to keep the bad constructor and hack around it to implement JUnit4TestAdapter properly. It doesn't make sense to have internal packages and then still treat them as untouchable API. To consistently take the wrong way, wouldn't you have to do the same dance with ClassRequest as well (to avoid breaking users that rely on undocumented behavior)? Once you're in the mud, you never get out again.

[kcooney]

@mkeller regardless of whether we believe that other third-party projects should depend on code in internal packages, they do (and I don't blame them, since multiple times we made classes internal that should have been external). Since we know that this particular class is used by many projects, we should consider the cost of breaking them vs. the cost of having a clean internal API.

How about we compromise and add a second constructor to AllDefaultPossibilitiesBuilder that takes in a class instance, and update ClassRequest to pass in the class? That way we get the behavior you want, a mostly clean API, and we won't break anyone who creates or subclasses AllDefaultPossibilitiesBuilder.

I am less concerned about the behavior change in ClassRequest; I think the API makes the intent clear, and we wouldn't be causing compile-time breakages. It still could cause different tests to run then ran before the change, so I would recommend making these changes in the JUnit5 branch.

[kcooney]

I'm closing this in favor of #1344