@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
On Thu, Nov 4, 2021 at 3:13 PM Kirk Lund <kl...@apache.org> wrote:
> 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.
>>
>
