My opinion is that @VisibleForTesting is a code smell and either indicates that
there is refactoring needed or there are tests that are unnecessary. If there
is functionality in a private method that needs to be tested independently,
then that code should be extracted and placed in a separate class that has a
wider visibility that can be tested on its own.
The same could probably be said for @TestOnly unless there are actually static
analysis tools that need it for some reason.
From: Kirk Lund
Date: Thursday, November 4, 2021 at 15:13
To: dev@geode.apache.org
Subject: Re: @TestOnly or @VisibleForTesting
As everyone thinks about how we want to use these annotations, please keep
this in mind that both *@VisibleForTesting* and *@TestOnly* can be used on
Types (Class/Interface/Enum), Constructors, Methods and Fields. (not just
Methods)
On Thu, Nov 4, 2021 at 3:09 PM Kirk Lund wrote:
> Hey all,
>
> We're introducing a mess to the codebase. It's a small problem, but
> several small problems become a big problem and one of my missions is to
> clean up and improve the codebase.
>
> I recently started seeing lots of pull requests with usage of @TestOnly.
> Sometimes it's used instead of @VisibleForTesting, while sometimes I see
> both annotations added to the same method.
>
> Before we start using @TestOnly, I think we need some guidelines for when
> to use @TestOnly versus @VisibleForTesting. Or if we're going to replace
> @VisibleForTesting with @TestOnly, then we either need a PR for the
> replacement or, at a minimum, deprecation annotation and javadocs added to
> VisibleForTesting.java.
>
> The annotations appear similar but the javadocs describe slightly
> different meanings for them...
>
> *@VisibleForTesting* was created in Geode several years ago to mean that
> the method is either only for testing or the visibility of it was widened
> (example: a private method might be widened to be package-private,
> protected or public). The method might be used by product code, but it also
> has widened scope specifically to allow tests to call it. The javadocs say:
>
> "Annotates a program element that exists, or is more widely visible than
> otherwise necessary, only for use in test code.
>
> Introduced while mobbing with Michael Feathers. Name and javadoc borrowed
> from Guava and AssertJ (both are Apache License 2.0)."
>
> *@TestOnly* started appearing when we added org.jetbrains.annotations
> dependency earlier this year. It seems to indicate a method that is ONLY
> used for tests (never called by product). The javadocs say:
>
> "A member or type annotated with TestOnly claims that it should be used
> from testing code only.
>
> Apart from documentation purposes this annotation is intended to be used
> by static analysis tools to validate against element contract violations.
>
> This annotation means that the annotated element exposes internal data and
> breaks encapsulation of the containing class; the annotation won't prevent
> its use from production code, developers even won't see warnings if their
> IDE doesn't support the annotation. It's better to provide proper API which
> can be used in production as well as in tests."
>
> So... when do we use one over the other? I don't think both annotations
> should be on the same method. Also, some sort of guidelines are needed if
> we're going to start using @TestOnly.
>
