Verify that all (base-) classes declaring @ClassRules are public

[ferstl]

An IllegalAccessException is thrown in case a test class derives from a non-public base class which declares a @ClassRule.

For example, this code ...

 abstract class BaseTest {
   @ClassRule
   public static ExpectedException expEx = ExpectedException.none();
 }

 public class RealTest extends BaseTest {
   @Test
   public void myTest() {
     ...
   }
 }

… will cause this exception

 java.lang.RuntimeException: How did getFields return a field we couldn't access?
    at org.junit.runners.model.TestClass.getAnnotatedFieldValues(TestClass.java:161)
    ...
  Caused by: java.lang.IllegalAccessException: Class org.junit.runners.model.FrameworkField can not access a member of class BaseTest with modifiers "public static final"
    at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:95)
    ...

To solve this issue, I added an additional verification to RuleFieldValidator that checks that all @ClassRules are declared in public classes. A similar validation is already done for @BeforeClass and @AfterClass methods.

[stefanbirkner]

It would be nice if getDeclaringClass() returns a TestClass object, which is JUnit's wrapper for Class objects. Then you can add an isPublic() method to TestClass and avoid the duplicate Modifier.isPublic(aClass.getModifiers())

[ferstl]

Indeed, that would make the checks simpler. However, TestClass does quite a few things in its constructor which are not necessary whatsoever to fix this bug. Are you sure you want to have getDeclaringClass() return a TestClass?

[stefanbirkner]

I'm sure :-), but I would like to hear at least @dsaff's or @kcooney's position.

[stefanbirkner]

Just two suggestions for improvement. Everything else is fine. Thanks @ferstl.

[ferstl]

@stefanbirkner I made two new commits for each of your comments. I'll squash them if these changes are OK for everyone.
One more thing about FrameworkMember#getDeclaringClass() returning a TestClass: I first tried to initialize the declaring class in the constructors of FrameworkField and FrameworkMethod and just return them in each call to getDeclaringClass(). However, this resulted in an StackOverflowError during construction of the TestClass. So getDeclaringClass() does now create a new TestClass instance on each call. I don't think that is a good idea. I'd still prefer getDeclaringClass() returning a simple Class object.

[dsaff]

I would indeed really like not to call the expensive TestClass constructor in multiple places. I agree that going back to getDeclaringClass() returning a simple Class object is the right approach.

Sorry I wasn't able to jump into the discussion before now.

[ferstl]

I would indeed really like not to call the expensive TestClass constructor in multiple places. I agree that going back to getDeclaringClass() returning a simple Class object is the right approach.

@dsaff Ok, I removed that change. getDeclaringClass() does now return a plain Class object again.

[dsaff]

Looks good to me. Please squash!

[ferstl]

@dsaff Done.

[dsaff]

Thanks!

[kcooney]

Just catching up on (very) old JUnit updates.

I think returning a TestClass is reasonable. We could revisit the idea of having TestClass having a creational method that uses a cached map of Class to TestClass. Since runners live for the live of the JVM, and runners keep references to the classes they run, in practice I think we keep TestClass instances around forever, so it might not be so bad to keep a map.

@dsaff What do you think of caching a map from Class to TestClass?