Rather than change an in-production class scope for testing reasons, I would
like to see a new pass-through method with required scope and the @TestOnly
annotation. This leaves the in-use method alone with correct
encapsulation/visibility, and makes a clear, concise way to access it for test
that is un-ambiguous as to its intent (testing only, non-production).
As a consumer of Geode, I think the broader @VisibleForTesting is worse, in
that I no longer know the scope from which I am “supposed” to consume the
method. Whereas for @TestOnly, I know the answer is “nowhere”
-Robert Houghton
From: Mark Hanson <hans...@vmware.com>
Date: Friday, November 5, 2021 at 10:11 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: @TestOnly or @VisibleForTesting
I generally see @VisibleForTesting used to change permissions of a private
method, which is inaccessible to an external object, into a public method to
allow a test to reach in and set or get fields within a class.
I see it as an invaluable bridge between the reality of where we are now with
our less than perfect codebase and where we want to go. I know a number of
classes could be rewritten to enable better dependency injection, but I see
this as a necessary and beneficial stop gap to that end.
With regard to @TestOnly, I have yet to really have a need to use this as
@VisibleForTesting, generally, seems broader than @TestOnly. If @TestOnly
causes methods to be optimized out and thus reduces object size, I think there
can be real benefit there. But I don't see much harm in having a few extra
methods on some of our main objects which occur infrequently, such as
DistributedMember or PoolImpl.
Unless there is more substantial benefit just as reduce object size, I would
suggest that we just use @VisibleForTesting and avoid adding dependencies.
Thanks,
Mark
On 11/5/21, 9:32 AM, "Dale Emery" <dem...@vmware.com> wrote:
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.
Dale
From: Alexander Murmann <amurm...@vmware.com>
Date: Thursday, November 4, 2021 at 5:02 PM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: @TestOnly or @VisibleForTesting
Another +1 to Eric's point. What are others seeing as valid use cases for
those annotations?
________________________________
From: Patrick Johnson <jpatr...@vmware.com>
Sent: Thursday, November 4, 2021 15:55
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: @TestOnly or @VisibleForTesting
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.
>>
