Sometimes, doing things properly is hard. When that happens, there’s always @VisibleForTesting.
To quote the Javadoc: @VisibleForTesting annotates a program element that exists, or is more widely visible than otherwise necessary, only for use in test code.
There are occasions where @VisibleForTesting is the best solution, I suspect: I’m not entirely sure what they are. If something’s worth testing, that should be possible via its interface: if something’s worth testing but shouldn’t be visible, then extract it into a class and make that class package-private.
But it’s easy to get on a high horse: I wouldn’t be talking about @VisibleForTesting if we didn’t use it. Often, it’s an incremental improvement on exposing too much without making major changes (particularly when the annotated method is a constructor).
The real danger about @VisibleForTesting is all it does is represent intent, and that intent isn’t always immediately obvious to the end user. Nothing stops people calling such methods from non-test code, and that’s a quick step towards an incoherent design.
What would be nice is if we could enforce that intent, and prevent people from calling @VisibleForTesting methods from real code. That’s not something you can do with plain Java, though.
You can do it in Findbugs. In fact, this is the first Findbugs detector I wrote. It looks a little like this:
public class VisibleForTestingDetector extends BytecodeScanningDetector {
private static final ClassDescriptor VISIBLE_FOR_TESTING_ANNOTATION =
DescriptorFactory.createClassDescriptor(VisibleForTesting.class);
private final BugReporter bugReporter;
public VisibleForTestingDetector(final BugReporter bugReporter) {
this.bugReporter = bugReporter;
}
@Override
public void sawMethod()
{
checkMethod();
}
@Override
public void sawIMethod()
{
checkMethod();
}
private void checkMethod()
{
final XMethod invokedMethod = getXMethodOperand();
final XMethod invokingMethod = getXMethod();
if(isVisibleForTesting(invokedMethod))
{
if(allowedToInvokeMethod(invokingMethod))
{
bugReporter.reportBug(
new BugInstance(this, "VISIBLE_FOR_TESTING_BUG", HIGH_PRIORITY)
.addClassAndMethod(this)
.addSourceLine(this));
}
}
}
private boolean isAnnotated(final AnnotatedObject annotatedObject)
{
return annotatedObject != null &&
annotatedObject.getAnnotation(VISIBLE_FOR_TESTING_ANNOTATION) != null;
}
private boolean allowedToInvokeMethod(final XMethod invokingMethod)
{
return !isAnnotated(invokingMethod) &&
!invokingLocalMethod() &&
!isTestClass();
}
private boolean isTestClass()
{
final ClassDescriptor invokingClass = getClassDescriptor();
return invokingClass.getClassName().endsWith("Test");
}
private boolean invokingLocalMethod()
{
final ClassDescriptor methodOwner = getClassDescriptorOperand();
final ClassDescriptor invokingClass = getClassDescriptor();
return invokingClass.equals(methodOwner);
}
}
The logic here is simple enough: whenever we see a method invocation, we check to see if the method we’re calling is annotated @VisibleForTesting. If it is, then we check whether or not we’re allowed to invoke it.
We’re allowed to call it from tests, of course: that’s why it’s annotated.
We’re also allowed to call it from the class itself: @VisibleForTesting usually takes something which should be private and makes it public, but that private method is still called by the public interface of the class.
Finally, we allow methods in other, non-test classes to call @VisibleForTesting methods if they themselves are @VisibleForTesting.
Let’s step through some of the interesting parts of this detector:
private static final ClassDescriptor VISIBLE_FOR_TESTING_ANNOTATION = DescriptorFactory.createClassDescriptor(VisibleForTesting.class);
We saw something very like this in the Files.lines() detector in the last post: we’re getting the class descriptor for the feature we want to analyse up front. What’s interesting this time is this now introduces a dependency. This will neither compile nor run without the appropriate jarfile on the classpath.
This isn’t the only way to get a class descriptor: you can also do it from a String, specifying the signature. That wouldn’t introduce a dependency and would work identically. So why do it this way?
Firstly, it’s easy to mistype the signature and the net result is the detector would just never detect anything. This is more robust: either it matches exactly or it fails to compile. Of course, if you test your detectors then you should be OK.
Secondly, it’s way more readable. The signature for @VisibleForTesting is “Lcom/google/common/annotations/VisibleForTesting;”: that’s not too bad. For an array of long, it’s “[J”.
Thirdly, and this doesn’t apply in this particular case: it’s robust to refactoring. This really comes into play when you start introducing domain-specific tests, where your Findbugs tests depend on your code – we’ll get onto some of the cool things this allows you to do in a future post. IDEs will always catch class renames, which you often won’t get with signature renames.
On the other hand, it means that the detector couldn’t be added to, say, findbugs-contrib without requiring all users to include the Google jar on their classpath. If we used the signature, then we could include a check which provided value to those it applied to, without imposing a burden on those it doesn’t.
@Override
public void sawMethod()
{
checkMethod();
}
@Override
public void sawIMethod()
{
checkMethod();
}
Would we ever make an interface method visible for testing? I don’t think so, but it’s important to remember that sawMethod() does not catch all method calls.
final XMethod invokedMethod = getXMethodOperand(); final XMethod invokingMethod = getXMethod();
XMethods are the cool, ’90s version of MethodDescriptors. Whereas a MethodDescriptor knows a method’s name, location, and signature and not much else – being basically a unique identifier for a method – XMethods are chock-full of useful data about a method, like its annotations.
This is also a good place to compare getFoo() vs getFooOperand(): we care both about the method we’re in and the method we’re currently calling, and it’s clear why one or the other has to have a slightly awkward name (although in such situations I favour giving *both* an awkward name so you don’t accidentally default to the wrong one).
private boolean isAnnotated(final AnnotatedObject annotatedObject)
{
return annotatedObject != null &&
annotatedObject.getAnnotation(VISIBLE_FOR_TESTING_ANNOTATION) != null;
}
There’s no isAnnotated(annotation) or hasAnnotation(annotation) method on an AnnotatedObject: instead, we get the @VisibleForAnnotation annotation off the AnnotatedObject, and if we get anything back then it’s annotated. Getting the annotation back is useful when you have parameterised annotations, but that doesn’t happen all that often.
private boolean allowedToInvokeMethod(final XMethod invokingMethod)
{
return !isAnnotated(invokingMethod) &&
...
}
This was borne of necessity when we (felt we) needed to manipulate class internals of a deeply nested class in integration tests. The idea is that if a @VisibleForTesting method can be called by another @VisibleForTesting method in a test, and therefore transitively any extra-class invocations are coming from tests.
Alas, this is not robust: this allows for the possibility of a public method of class A to call a should-be-private method of A, which can then drill into a should-be-private method of class B, allowing real code a sideways route into B, and this should be reconsidered. This is fixable in a number of ways, but the simplest is to refactor said code to not require @VisibleForTesting any more.
private boolean isTestClass()
{
final ClassDescriptor invokingClass = getClassDescriptor();
return invokingClass.getClassName().endsWith("Test");
}
Our definition of a test is not super robust here. It’s worth considering what the philosophy of these tests is: we’re not trying to catch as much as possible while minimising false positives at the expense of everything else here.
Simplicity is a virtue: we want anyone who’s never dealt with the Findbugs API to look at a detector and understand what it’s doing and how, because it’s part of our codebase which anyone should be able to modify easily.
It may not be a fair definition in your codebase (if you ever have real classes called *Test or don’t call all your test classes *Test), but it is in ours.
In short: many of the decisions made here are not perfect, but are reasonable, for something which is not easy to be robust and objective about at the same time. If we do find any of these decisions turn out to not work well for us, we can easily modify the detector to suit our newly observed requirements.
Ultimately, that’s the reason it wouldn’t be well suited to something like findbugs-contrib: the above philosophy is grand within a team, for our specific requirements, but isn’t a great fit for general requirements.
Coming up next time: how domain-specific detectors can complement your designs.
writeoncereadmany 