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 <kl...@apache.org> Date: Thursday, November 4, 2021 at 15:13 To: dev@geode.apache.org <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 <kl...@apache.org> 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. >
