I agree with Eric. Maybe rather than standardizing on one, we should stop adding anymore @VisibleForTesting or @TestOnly to the codebase. Possibly deprecate @VisibleForTesting.
> On Nov 4, 2021, at 3:30 PM, Eric Zoerner <zoern...@vmware.com> wrote: > > 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. >>
