-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug: ParentRunner lost test Class from a separate class loader #1252
Bug: ParentRunner lost test Class from a separate class loader #1252
Conversation
Any updates? |
Sorry, @Gordiychuk, but I did not get to doing a thorough review, yet. |
getRunnerAnnotations()); | ||
Description description = | ||
Description.createSuiteDescription( | ||
getTestClass().getJavaClass(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case Description wouldn't have a class name either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
LGTM |
@kcooney I think the new factory method should have an additional Edit: So this would be Description.createSuiteDescription(getTestClass().getJavaClass(), getName(), getRunnerAnnotations()); |
@marcphilipp why would a suite description need both a class and a display name? |
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 |
The big issue I had with the original version of this pull is the new method in This change should properly handle classes that override I am personally fine with the slight amount of complexity in the implementation of Edit: If we can find a way to have the changes to the |
I've read the entire comment thread but still don't see why |
@marcphilipp the problems are
For the second point, I personally dislike methods that silently allow For the third point, the behavior in 4.12 is to use |
@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. |
It PR can be merge or I should do more changes? |
@kcooney Your call! :) |
@Gordiychuk I don't think the new test classes would run in the maven build. Can you add them to the appropriate test suite? |
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.
…erride getName method
bb41b51
to
6f15096
Compare
@kcooney done, i also rebase branch on actual master state |
LGTM! @kcooney Do you want to merge it? |
@marcphilipp will do soon. Want to take one more look. |
@Gordiychuk could you please update the release notes? |
@kcooney in what file? |
Done. Thanks for review changes. |
…-team#1252) * Bug: ParentRunner lost test Class from a separate class loader 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. * Restore backward compatibility with sub class of ParentRunner that override getName method * Add ParentRunnerClassLoaderTest to AllClassesTests suite
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, becauseParentRunner
instead of set Class as is toDescription
tranform it to class name string, as resultorg.junit.runner.Description#getTestClass
executeClass.forName
and can't find test class.Spring-test fail with exception if we try use
SpringClassRule
As solution, now
ParentRunner
createDescriptor
with explicit specify Class, name and uses annotations.