let @VisibleForTesting tells original visibility of annotated element

[gissuebot]

Original issue created by skypencil on 2014-01-22 at 02:13 AM

---

Hello, I am trying to implement static code analysis tool which detects illegal method call like below:

  // an implementation which has a method annotated by @VisibleForTesting   class MyImplementation {     @VisibleForTesting     /* private */ void method() { ... }   }

  // another implementation   class AnotherImplementation {     void method() {       new MyImplementation().method(); // illegal method call, because this class is not test code     }   }

To find illegal method calling, we should know the original visibility of annotated element (method in this case). In some case it is difficult to guess original visibility, so I want to enhance @VisibleForTesting to explain it like below:

@VisibleForTesting(original = Visibility.PRIVATE)
void method() { ... }

I think all static analysis tool which uses this annotation needs method to judge illegality of method call, so I have added methods into Visibility enum in attached diff. It is also good to make this enum simple like com.android.internal.annotations.VisibleForTesting annotation. See https://github.com/android/platform_frameworks_base/blob/8b2c3a14603d163d7564e6f60286995079687690/core/java/com/android/internal/annotations/VisibleForTesting.java

[gissuebot]

Original comment posted by christianedwardgruber on 2014-01-23 at 11:45 PM

---

If we were to do this, and I am not suggesting we should, we should probably make it value() not original(), so we can just write @VisibleForTesting(PRIVATE). I think it's really obvious what's going on - the extra verbiage seems superfluous.

[gissuebot]

Original comment posted by skypencil on 2014-04-04 at 12:20 PM

---

Any progress?

[gissuebot]

Original comment posted by kak@google.com on 2014-04-04 at 03:18 PM

---

FWIW, we've had this internally for about 6 years (the Android version is nearly a copy of the Guava version), but until we have tooling to support this, it doesn't seem particularly useful.

Do you have some special static analysis tools that make it useful?

[gissuebot]

Original comment posted by skypencil on 2014-04-05 at 03:14 AM

---

Hello, thanks for your reply.

Do you have some special static analysis tools that make it useful?

I have a plan to implement new FindBugs detector which uses on this annotation. It might be a part of my FindBugs plugin(https://github.com/eller86/findbugs-plugin), or it might be merged into FindBugs itself (I'll try to send a patch).

I wish this post will help you to judge. Thanks.

[gissuebot]

Original comment posted by skypencil on 2014-04-08 at 08:55 AM

---

FYI: simple implementation is here.

https://github.com/eller86/findbugs-plugin/commit/e53e0e168d37101f2e7daeecdc53b148d84125b7

[d9n]

+1 to this issue. It's completely backwards compatible to add the feature, it should be trivial to implement, and even without static analysis tools, it can be useful for documentation purposes.

I have an abstract base class in my codebase which has a method that should be protected, but we have tests that want to access it. Essentially I want to mark it

@VisibleForTesting(visibility = Visibility.PROTECTED)
public final void doSomething() { ... }

but right now, I can't. Instead, I've left a comment:

// This method should only be called by tests and subclasses
@VisibleForTesting
public final void doSomething() { ... }

but that feels more easily ignored, plus I can't ever expect static analysis tools to help me detect it if those come online later.

I suspect there may be pushback, like: You should instead be writing your tests differently so they interact with a test subclass that exposes this method publicly, or something. Of course, it's doable, but because of much of the existing code, it wouldn't be trivial - it would involve punching holes in a bunch of nested classes.

So, instead, we are resorting to using VisibleForTesting anyway, just without the extra documentation information that would definitely be a nice to have. Some projects have gotten around this by adding the Android Annotations library, but it seems like a waste to do that just for this one feature.

[QStormDS]

Is there any progress on it ? The "simple" version is already in findbugs. How about the @VisibleForTesting extension (including Findbugs detector)?

[kluever]

@eaftan Any chance we could get error-prone to support (check) the visibility field?

[rice668]

Actually, We can use reflection tools as a helper. But It has limitations as we could not find whether it is under the folder such as src/test. We can just know it's package name.

[rice668]

Here is the code, but it has limitations I think. https://github.com/apache/flink/pull/4705/files#diff-217bc109514e09399be6f360d539821a

[jbduncan]

@zhangminglei I'm not entirely sure we can legally use the code snippet from Apache Flink you linked to, even if Apache Flink itself is licensed under Apache License 2.0, as the snippet uses Reflections, which is licensed under the WTFPL, and I seem to remember that some companies/organisations dislike the WTFPL because of its lack of legal clarifications. :/

[rice668]

@jbduncan Thank you for your reply. I agree with you about this issue. I put the code here just as a reference, that means, we can use reflections implements that ,but It has it's limitations.

[jbduncan]

Potential alternative to Reflections: https://github.com/lukehutch/fast-classpath-scanner

[Maaartinus]

@jbduncan I can't see there anything like get all fields/methods/constructors usages in code.

[jbduncan]

@Maaartinus Oh, many thanks for pointing that out! I wasn't aware that fast-classpath-scanner isn't an exact replacement for Reflections, feature-wise - I'd (mistakenly) assumed it was. So feel free to ignore my suggestion to use fast-classpath-scanner, everyone. :)