Bug: ParentRunner lost test Class from a separate class loader

[Gordiychuk]

When junit.jar located in one ClassLoader but runing tests in another ParentRunner can lost information about run class. For example if use @ClassRule and request test class(org.junit.runner.Description#getTestClass) we can get null, because ParentRunner instead of set Class as is to Description tranform it to class name string, as result org.junit.runner.Description#getTestClass execute Class.forName and can't find test class.

Spring-test fail with exception if we try use SpringClassRule

java.lang.NullPointerException
 at org.springframework.test.context.junit4.rules.SpringClassRule.validateSpringMethodRuleConfiguration(SpringClassRule.java:186)
 at org.springframework.test.context.junit4.rules.SpringClassRule.apply(SpringClassRule.java:134)
 at org.junit.rules.RunRules.applyAll(RunRules.java:26)
 at org.junit.rules.RunRules.<init>(RunRules.java:15)
 at org.junit.runners.ParentRunner.withClassRules(ParentRunner.java:245)
 at org.junit.runners.ParentRunner.classBlock(ParentRunner.java:194)
 at org.junit.runners.ParentRunner.run(ParentRunner.java:362)
 at org.junit.runner.JUnitCore.run(JUnitCore.java:137)

As solution, now ParentRunner create Descriptor with explicit specify Class, name and uses annotations.

[Gordiychuk]

Any updates?

[marcphilipp]

Sorry, @Gordiychuk, but I did not get to doing a thorough review, yet.

[kcooney]

It looks like getJavaClass() can in theory return null. Just to be safe, I suggest you check for that here, and if it is null use getName() like we did before.

[Gordiychuk]

@kcooney is it work? If I set like test class null, method getName() return "null" how it will process next? But even if class can be null, code will work as they work before, because in that casen we initialize org.junit.runner.Description#fTestClass by null and in then if they null will try resolve by name

org.junit.runner.Description#getTestClass

public Class<?> getTestClass() {
        if (fTestClass != null) {
            return fTestClass;
        }
        String name = getClassName();
        if (name == null) {
            return null;
        }
        try {
            fTestClass = Class.forName(name, false, getClass().getClassLoader());
            return fTestClass;
        } catch (ClassNotFoundException e) {
            return null;
        }
    }

[kcooney]

But in this case Description wouldn't have a class name either.

[kcooney]

I see what you mean.

I think it's confusing to add a creational method to Description that takes in a class and a name, since the name parameter will only be used if the class parameter is null. I would prefer we do this:

Class<?> clazz = getTestClass().getJavaClass();
Description description;
if (clazz == null) {
  description = Description.createSuiteDescription(
          getName(), 
          getRunnerAnnotations());
} else {
  description = Description.createSuiteDescription(
          getTestClass().getJavaClass(),
          getRunnerAnnotations());
}

Even then, this would be a change in behavior for subclasses of ParentRunner that override getName()

Edit: we could do this:

Class<?> clazz = getTestClass().getJavaClass();
Description description;
// if subclass overrides `getName()` then we should use it
// to maintain backwards compatibility with JUnit 4.12
if (clazz == null || !clazz.getName().equals(getName())) {
  description = Description.createSuiteDescription(
          getName(), 
          getRunnerAnnotations());
} else {
  description = Description.createSuiteDescription(
          getTestClass().getJavaClass(),
          getRunnerAnnotations());
}

[Gordiychuk]

I fix it in new commit. But i think this kind of backward compatibility very confusing, Because firs, in ParentRunner we specify some class, and after that override it by getName() О_о

    @Test
    public void testBackwardCompatibilityWithOverrideGetName() throws Exception {
        final Class<TestWithClassRule> originalTestClass = TestWithClassRule.class;
        final Class<?> waitClass = ParentRunnerClassLoaderTest.class;

        ParentRunner<FrameworkMethod> subParentRunner = new BlockJUnit4ClassRunner(originalTestClass) {
            @Override
            protected String getName() {
                return waitClass.getName();
            }
        };

        Description description = subParentRunner.getDescription();
        Class<?> result = description.getTestClass();

        assertEquals("Subclass of ParentRunner can override getName method and specify another test class for run, " +
                "we should  maintain backwards compatibility with JUnit 4.12",
                waitClass, result
        );
    }

[kcooney]

@Gordiychuk I have seen ParentRunnner used in cases where there is no Java class (for example, a JavaScript test). There are probably other cases where geName() is overridden (tests in JVM-based languages perhaps).

Unfortunately, the Runner APIs have many extension points so are hard to change without breaking someone. I think what you have here is safe.

[kcooney]

LGTM

[marcphilipp]

@kcooney I think the new factory method should have an additional displayName parameter. Then we could get rid of the if, couldn't we?

Edit: So this would be

Description.createSuiteDescription(getTestClass().getJavaClass(), getName(), getRunnerAnnotations());

[kcooney]

@marcphilipp why would a suite description need both a class and a display name?

[marcphilipp]

ParentRunner.getName() is designed to be overridden to return a name different from the name of the test class. Custom runners may do so and there is even an example in our own codebase: BlockJUnit4ClassRunnerWithParameters overrides getName() and stores its testClass.

Therefore, I think it would make sense to create suite descriptions that store both display name and class. As a bonus, it would simplify code in ParentRunner.getDescription().

[kcooney]

The big issue I had with the original version of this pull is the new method in Description had very confusing symantics regaeding null. Please review the original change and the entire comment thread.

This change should properly handle classes that override getName().

I am personally fine with the slight amount of complexity in the implementation of ParentRunner if that leads to the new method in Description being simpler to understand.

Edit: If we can find a way to have the changes to the ParentRunner code and the Deacription API be both simple, that would be perfect.

[marcphilipp]

I've read the entire comment thread but still don't see why createSuiteDescription with name and class parameters would be confusing. Sorry... :-/

[kcooney]

@marcphilipp the problems are

1. TestClass.getJavaClass() can return null
2. It's unclear what Description.createSuiteDescription((Class<?) null, "name") should do
3. It's unclear what ParentRunner.getDescription() should do if TestClass.getJavaClass() returns null and getName() was overridden.

For the second point, I personally dislike methods that silently allow null with special behavior, because it's easy to accidentally propagate nulls, so I think If createSuiteDescription(Class<?> testClass, String name, Annotation... annotations) exists then Description.createSuiteDescription((Class<?) null, "name") should throw an exception (note that createTestDescription((Class<?) null, "name") will throw a NullPointerException).

For the third point, the behavior in 4.12 is to use ParentRunner.getName() as the display name of the returned Description object. Unfortunately, when this happens, the behavior of Description.getTestClass() depends on the name that was passed in. In theory, if a subclass of ParentRunner could override getName() such that the test class is completely different type than the test class passed into the ParentRunner constructor.

[marcphilipp]

@kcooney Thanks for the explanation! I see your point now.

I just thought it would be beneficial if we could support creating a suite description with a class and a custom name. However, I'm fine with merging this pull requests as it is.

[Gordiychuk]

It PR can be merge or I should do more changes?

[marcphilipp]

@kcooney Your call! :)

[kcooney]

@Gordiychuk I don't think the new test classes would run in the maven build. Can you add them to the appropriate test suite?

[Gordiychuk]

@kcooney done, i also rebase branch on actual master state

[marcphilipp]

LGTM!

@kcooney Do you want to merge it?

[kcooney]

@marcphilipp will do soon. Want to take one more look.

[kcooney]

@Gordiychuk could you please update the release notes?

[Gordiychuk]

@kcooney in what file?

[baev]

@Gordiychuk https://github.com/junit-team/junit4/wiki/4.13-Release-Notes

[Gordiychuk]

Done. Thanks for review changes.