Re: November Community Meeting

[Alexander Murmann]

Big thank you to Evaristo for presenting on query resource management today! It 
was very intersting to learn about the problems you and your team had run into 
around query resource consumption, how you worked around them and how we might 
improve that area of Apache Geode further in the future.

I highly recommend for everyone who missed the meeting to watch the recording. 
It's linked on our 
wiki
 or you can find it directly on our Geode YouTube 
channel.

Also thank you to everyone who joined and maybe even took part in the 
discussion!

We are still looking for a topic for our next meeting which should be on 
December 2nd. If you have something to present or discuss, don't be shy to 
bring the topic.

From: Alexander Murmann 
Sent: Thursday, October 28, 2021 08:01
To: geode 
Subject: November Community Meeting

Hi everyone,

A week from now on Thursday November 4th at  15:00 UTC/8:00 Pacific  is our 
next Geode Community Meeting.

Currently the agenda is a talk on Query Resource Management.

Please find all details on the corresponging wiki 
page


Looking forward to see you all there!

Using Ports Safely in Tests

[Dale Emery]

As of July, 2021, Geode's build system no longer executes test worker processes 
in separate Docker containers.

This increases the risk of port collisions between tests. Each test worker JVM 
and each Java process started by a test executes directly in the environment 
provided by the host machine. If a test is using a port, any concurrently 
running test that tries to bind to the same port number will suffer a 
BindException.
Safely allocating ports. To reduce this risk, Geode's build system now assigns 
each test worker JVM a distinct range of ports. In CI, which runs 24 
distributed tests concurrently, each test gets a range of about 400 ports.
In JVMs running Geode version 1.14 or later, AvailablePort and 
AvailablePortHelper will choose ports from this assigned range. As a result, no 
two tests will ever receive the same port number from these classes, no matter 
how many are running concurrently.
When you write a test, keep the following guidelines in mind, to make sure your 
test uses only the ranges of ports that the Geode build system assigned to it:

  *   To assign an available port, call AvailablePortHelper or AvailablePort. 
These are the only classes that know about the reduced port range for the test.
  *   Call AvailablePort and AvailablePortHelper only in the current version of 
Geode. Do not call these classes in Child VMs running older versions of Geode. 
Older versions of Geode do not honor the new, reduced port ranges.
  *   Do not launch any service using Geode’s default port for that service. 
Always explicitly assign an available port. The only safe use of default ports 
is in a test to verify that a service uses its default port by default. Note 
that such a test will fail the stress test CI job. To exclude such a test from 
the stress test job, annotate it with @Category(IgnoreInRepeatTestTasks.class).
  *   Do not launch any service using a port number hard-coded into the test. 
Always explicitly assign an available port. There is no safe use of any 
hard-coded port number in tests that can run concurrently.
  *   Do not attempt to reuse an ephemeral port. Some tests start a member on 
an ephemeral port, stop the member, and attempt to restart it on the same port. 
This is not safe. It was never safe. During the time between when the member 
stops and when it restarts, the port is available for the operating system to 
give to any other process. In CI, it is very, very likely that one or more 
concurrently-running tests (or other processes) will request ephemeral ports 
during the time when your test is not bound to it. If one of those processes 
gets the port your test was using, your test will fail.
This information is available on the Geode wiki: 
https://cwiki.apache.org/confluence/display/GEODE/Using+Ports+Safely+in+Tests

Dale

@TestOnly or @VisibleForTesting

[Kirk Lund]

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.

Re: @TestOnly or @VisibleForTesting

[Kirk Lund]

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  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.
>

Re: Using Ports Safely in Tests

[Kirk Lund]

Dale, Thanks for doing this work and especially for adding these guidelines
to the Wiki!

On Thu, Nov 4, 2021 at 1:08 PM Dale Emery  wrote:

> As of July, 2021, Geode's build system no longer executes test worker
> processes in separate Docker containers.
>
> This increases the risk of port collisions between tests. Each test worker
> JVM and each Java process started by a test executes directly in the
> environment provided by the host machine. If a test is using a port, any
> concurrently running test that tries to bind to the same port number will
> suffer a BindException.
> Safely allocating ports. To reduce this risk, Geode's build system now
> assigns each test worker JVM a distinct range of ports. In CI, which runs
> 24 distributed tests concurrently, each test gets a range of about 400
> ports.
> In JVMs running Geode version 1.14 or later, AvailablePort and
> AvailablePortHelper will choose ports from this assigned range. As a
> result, no two tests will ever receive the same port number from these
> classes, no matter how many are running concurrently.
> When you write a test, keep the following guidelines in mind, to make sure
> your test uses only the ranges of ports that the Geode build system
> assigned to it:
>
>   *   To assign an available port, call AvailablePortHelper or
> AvailablePort. These are the only classes that know about the reduced port
> range for the test.
>   *   Call AvailablePort and AvailablePortHelper only in the current
> version of Geode. Do not call these classes in Child VMs running older
> versions of Geode. Older versions of Geode do not honor the new, reduced
> port ranges.
>   *   Do not launch any service using Geode’s default port for that
> service. Always explicitly assign an available port. The only safe use of
> default ports is in a test to verify that a service uses its default port
> by default. Note that such a test will fail the stress test CI job. To
> exclude such a test from the stress test job, annotate it with
> @Category(IgnoreInRepeatTestTasks.class).
>   *   Do not launch any service using a port number hard-coded into the
> test. Always explicitly assign an available port. There is no safe use of
> any hard-coded port number in tests that can run concurrently.
>   *   Do not attempt to reuse an ephemeral port. Some tests start a member
> on an ephemeral port, stop the member, and attempt to restart it on the
> same port. This is not safe. It was never safe. During the time between
> when the member stops and when it restarts, the port is available for the
> operating system to give to any other process. In CI, it is very, very
> likely that one or more concurrently-running tests (or other processes)
> will request ephemeral ports during the time when your test is not bound to
> it. If one of those processes gets the port your test was using, your test
> will fail.
> This information is available on the Geode wiki:
> https://cwiki.apache.org/confluence/display/GEODE/Using+Ports+Safely+in+Tests
>
> Dale
>
>

Apache Geode CI upgrade

[Robert Houghton]

Hello Geode Developers,

Next week, Wednesday, 10 November 2021, we are planning to upgrade the Apache 
Geode Concourse deployment from version 6.7.4 to 7.6.0. We will also be moving 
the backend for Concourse from Bosh to k8s. We will deploy all production 
pipelines (Apache repo, develop and support/* branches). Developer or personal 
pipelines will not be deployed. We are not able to bring job history along.

What this means to you:
Please anticipate some downtime in the develop pipelines, but PR checks should 
continue to run as normal. Links from GitHub PRs to specific Concourse jobs in 
the old deployment will break. If this happens to you, change the domain from 
https://concourse.apachegeode-ci.info to 
https://concourse-calcite.apachegeode-ci.info to reach the broken link. The 
team is brainstorming on how to not break these links in the future.


Thank you,
-Robert Houghton

Re: @TestOnly or @VisibleForTesting

[Eric Zoerner]

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 
Date: Thursday, November 4, 2021 at 15:13
To: 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  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.
>

Re: @TestOnly or @VisibleForTesting

[Owen Nichols]

It would be really great if there were java tooling to actually compile 
separate production binaries and test binaries based on annotations like this, 
rather than having to ship methods that were only exposed for testing.

If such a preprocessor does exist, @TestOnly provides a stronger, unambiguous 
contract: any method with than annotation can be dropped for the production 
build.  However, @VisibleForTesting is weak, because it does not express what 
the correct lower visibility ought to be, so automation would be impossible.

If we decide to allow both, I agree that both annotations should never appear 
on the same entity, since the contracts are incompatible: @TestOnly must be 
used by tests only, while @VisibleForTesting by definition is also used in 
production.

It seems that @VisibleForTesting is a "lazy" shortcut -- instead of adding that 
annotation, you could instead create a delegating method annotated with 
@TestOnly, at the cost of 4 "extra" lines of code.

On 11/4/21, 3:30 PM, "Eric Zoerner"  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 
Date: Thursday, November 4, 2021 at 15:13
To: 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  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.
>

Re: @TestOnly or @VisibleForTesting

[Patrick Johnson]

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  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 
> Date: Thursday, November 4, 2021 at 15:13
> To: 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  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.
>> 

Re: @TestOnly or @VisibleForTesting

[Jinmei Liao]

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.

From: Patrick Johnson 
Date: Thursday, November 4, 2021 at 3:56 PM
To: 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  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 
> Date: Thursday, November 4, 2021 at 15:13
> To: 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  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.
>>

Re: @TestOnly or @VisibleForTesting

[Alexander Murmann]

Another +1 to Eric's point. What are others seeing as valid use cases for those 
annotations?

From: Patrick Johnson 
Sent: Thursday, November 4, 2021 15:55
To: 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  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 
> Date: Thursday, November 4, 2021 at 15:13
> To: 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  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.
>>