For constructors, I suggest that we don't need to use either annotation. A
package-private constructor that accepts parameters for all dependencies is
not breaking encapsulation or exposing internals and is very appropriate
for constructor based injection. It adheres to best practices as set out
for OOP and TDD for both new code and legacy code. The key best practice
for this is to make sure multiple constructors use constructor chaining.
-Kirk
On Fri, Nov 5, 2021 at 11:58 AM Owen Nichols <onich...@vmware.com> wrote:
> @VisibileForTesting is bad. It does NOT tell you if the method SHOULD
> HAVE BEEN private, or package-private, or protected, so there is no way to
> reconstruct the originally-intended code if the annotation is ever
> removed. It also prevents the exposed methods from being named in a way
> that clearly indicates that user code should never call them.
>
> @TestOnly is good. It tells you that for production, it is safe to remove
> the method entirely. We might someday like automatically remove all
> @TestOnly methods when we ship official Geode releases -- not just to save
> bytes as Mark mentioned, but TO AVOID EXPOSING PRIVATE INTERALS (or worse,
> exposing the ability to manipulate and modify private internals!)
>
>
> ANY use of @VisibileForTesting CAN be rewritten to use @TestOnly instead,
> for example:
>
> @VisibileForTesting
> public Foo getInternalStuff() {}
>
> Could instead be:
>
> private Foo getInternalStuff() {}
>
> @TestOnly
> public Foo getInternalStuffForTestOnly() {
> return getInternalStuff();
> }
>
>
> That said, I understand that *having tests* is more valuable than having
> perfect encapsulation, so if the extra 4 lines of code needed to do the
> right thing is a disincentive to writing tests...I'd rather have the
> tests...
>
>
> On 11/5/21, 11:37 AM, "Kirk Lund" <kl...@apache.org> wrote:
>
> I'm ok with not using these annotations, but we still need to write
> *unit
> tests* in addition to end-to-end *system tests*.
>
> I started this thread primarily to standardize on how we use one or
> both
> annotations. We shouldn't be using both arbitrarily without some sort
> of
> guidelines.
>
> If we want to actually stop using these annotations, then someone who
> feels
> strongly about removing them should volunteer to submit a pull request
> that
> *removes* all usages from the codebase. However, the purpose of these
> annotations is to document some important information about the code
> for
> better understanding by the developers working on it. So, please
> realize
> that we will be losing this information. Also, removing the annotations
> doesn't change the fact that we still need constructors and methods
> with
> scopes that allow for testing.
>
> Jinmei's description matches how I would want to use these annotations
> (if
> we want to use both):
>
> My understanding is @VisibileForTesting methods are used by the
> products,
> > while @TestOnly methods are used only by the tests.
> >
> > In practice, I don’t like to add @TestOnly methods (although I like
> to
> > mark those methods with this annotation if I found out a method is
> only
> > used for testing for better identification), but I do find it useful
> to add
> > @VisibleForTesting methods while making unit tests.
> >
>
> Dale's description represents a good usage of *@VisibleForTesting* for
> chaining constructors to inject dependencies for unit testing:
>
> Kirk and I often use @VisibleForTesting as part of a technique for
> making
> > complex code more testable.
> >
> > Many classes have “hidden” dependencies that make that class hard to
> test.
> > A useful technique is to add a constructor parameter to allow a test
> to
> > inject a more controllable, more observable instance.
> >
> > But we don’t want to force every use of the existing constructor to
> have
> > to create that instance. So we leave the old constructor’s signature
> as is,
> > create a new constructor that accepts that new parameter, and make
> the old
> > constructor call the new one.
> >
> > This new, intermediate constructor is called only by the old
> constructor
> > and by tests. In order for tests to call it, we have to make it
> visible, so
> > we mark it as @VisibleForTesting.
> >
>
> If a large constructor such as the one for *PartitionedRegion*
> constructs
> its own dependencies using constructors new Dependency(...) and static
> factories *Dependency.create*, then that prevents unit testing.
>
> Untestable class:
>
> *public UntestableClass() {*
> * // fetches its own dependencies*
> * dependency1 = new Dependency1();*
>
> * dependency2 = Dependency2.create(...);}*
>
> Testable class:
>
> *public TestableClass(Dependency1 dependency1, Dependency2
> dependency2) {*
> * // all dependencies are passed in*
> * this.dependency1 = dependency1;*
> * this.dependency2 = dependency2;*
> *}*
>
> Now if you're trying to write unit tests for a class like
> *PartitionedRegion*, the best technique is to use constructor chaining
> so
> that the last constructor accepts all dependencies:
>
> *public RefactoredClass() {*
> * this(new Dependency1(), Dependency2.create(...));*
> *}*
>
>
> *RefactoredClass(Dependency1 dependency1, Dependency2 dependency2) {*
> * // all dependencies are passed in*
> * ...*
> *}*
>
> Most of us have been applying *@VisibleForTesting* to the
> package-private
> constructor that accepts all dependencies. Using the annotation doesn't
> change the fact that we need to pass in the dependencies for unit
> testing.
> The only other option is extremely large pull requests that change that
> first constructor to require ALL dependencies.
>
> -Kirk
>
