Java Style Guide: ForTesting section

[Andrew Grieve]

Proposed change: https://chromium-review.googlesource.com/c/chromium/src/+/4626901

Namely:

* Remove reference to BuildConfig.IS_FOR_TEST

* Say that ForTesting methods should not have @VisibleForTesting

Comments here or on review are welcome.

[Nate Fischer]

Generally supportive of this. I think it's good to have more consistency for how we use this annotation.

I think `@VisibleForTesting(otherwise = VisibleForTesting.NONE)` is designed to annotate ForTesting methods. Here's the javadoc: https://developer.android.com/reference/androidx/annotation/VisibleForTesting#NONE(). Should we relax the presubmit check to allow this usage, or is this too redundant? There's a handful of spots which currently use this: https://source.chromium.org/search?q=-f:%5Eout%2F%20-f:third_party%2F%20VisibleForTesting.NONE&ss=chromium

It would also be helpful if the style guide had a sentence or two as guidance for when it's better to use @VisibleForTesting as opposed to the "ForTesting" suffix.

Nate Fischer | Software Engineer | ntf...@google.com

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1XU77%2B-H_S_oP8EgyHhXNMeKfVbsJ32aPHOiWGD8uzUnQ%40mail.gmail.com.

[Boris Sazonov]

+1 to Nate's proposal.

For what it's worth, I find using the annotation much cleaner and more easy to discover and follow through, when compared to suffixes (some engineers might expect that suffixes have no effect on the behavior).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACiwn8buYFND-o4PXFf%3DFj-wB_dfrct7RQsS_0w%2BZ32GZUv89g%40mail.gmail.com.

[Tommy Nyquist]

Unless we enforce this with a presubmit I think it's going to be hard to enforce people never writing `@VisibleForTesting(otherwise = VisibleForTesting.NONE)` in our codebase, since we already use the other parts of the `@VisibleForTesting` annotation. So I think I fall on the either enforce with presubmit or explicitly allow otherwise = NONE usage in the style guide. I think I have a slight preference to have no annotation, since it is easy to forget to add it.

--

Tommy

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAPtdB4nueK3odKAzyZQikNJVuM11Q4XtHjMMi2RDRoa5oWZKbw%40mail.gmail.com.

[Andrew Grieve]

Thanks! I've added some text about when to use @VisibleForTesting, and made it more clear that otherwise=NONE is a thing that we are choosing not to use. 

I agree annotations are more conventional, but the suffix works in C++ as well and annotations do not survive optimization (so we can't use them the same way we use the suffix for ensuring they are optimized away).

[Andrew Grieve]

Just thought of a reason to need `@VisibleForTesting(otherwise = VisibleForTesting.NONE)`: test-only constructors. 

I'm adding a note about that here.