+1 to Donal.
And it also provides a way to easily find these places in the future that can
have some further refactoring. Just search for @VisibleForTesting and refactor
it away.
________________________________
From: Donal Evans <doev...@vmware.com>
Sent: Friday, November 5, 2021 9:19 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: @TestOnly or @VisibleForTesting
The main time I use the @VisibleForTesting annotation is when I'm unit testing
a method that includes static calls that can't be mocked or verified. A random
example from the codebase is LatestLastAccessTimeMessage.process() which has a
call to sendReply(dm, lastAccessed) as the last line of the method. This method
has been extracted and marked @VisibleForTesting to allow verification and
mocking in unit tests as it just wraps a static call:
@VisibleForTesting
void sendReply(ClusterDistributionManager dm, long lastAccessTime) {
ReplyMessage.send(getSender(), this.processorId, lastAccessTime, dm);
}
I agree that ideally we wouldn't need to use these annotations, but given a
choice between developers using them to improve unit test coverage on classes
that are severely lacking in it and not adding test coverage at all because, in
order to avoid using these annotations, a prohibitively huge refactor of
multiple classes would be required, I would definitely see using them as the
lesser of two evils.
________________________________
From: Alexander Murmann <amurm...@vmware.com>
Sent: Thursday, November 4, 2021 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.
>>
