I wonder if we could use some sort of assertions to validate that that
tests have cleaned up, at least in a few ways? For example, if a
Cache/Locator/DistributedSystem is still running after a test has finished,
that's a good sign of a dirty environment. If system properties are still
set, that's a good sign of a dirty environment. We could use a custom test
runner, or even add a rule to all our tests en masse that checks that
things are cleaned up.
Jinmei, for single-JVM tests, you could write a method for your test (or
test class) that sets whatever properties you need and returns a Cache
constructed with those properties. Then you can use try-with-resources to
make sure that the Cache is properly closed. Is that a good alternative to
using a rule?
On Fri, Nov 9, 2018 at 3:28 PM Helena Bales <hba...@pivotal.io> wrote:
> +1 for deprecating old test bases. Many of the tests that gave me the most
> trouble this summer were because of JUnit4CacheTestCase.
> Also +1 for pulling out Blackboard into a rule.
>
> I will, however, argue for continuing to use ClusterStartupRule. The
> benefit of that is that it makes sure that all JVMs started for servers and
> locators are cleaned up at the end of the tests, even if the tests fail. We
> could certainly spend time making that code easier to understand, but I
> don't think that starting clusters is straightforward enough to have
> confidence that it will get done correctly every time. Multi-threaded tests
> should be a cautionary tale for this; some implementations were fine, but
> many polluted the system with threads that never stopped and tests that
> didn't actually test anything.
> As I see it, we are paying in readability for tests that do things the
> right way.
>
> On Fri, Nov 9, 2018 at 2:31 PM Kirk Lund <kl...@apache.org> wrote:
>
> > I would love to see you apply some of your passion for that to improving
> > the User APIs so there's less boiler plate code for the Users as well.
> >
> > Please don't forget that to Developers who are not familiar with our
> Rules
> > such as ClusterStarterRule, that it can be very difficult to understand
> > what it has done and how it has configured Geode. The more the Rule does,
> > the greater this problem. The fact that most of these Rules use Internal
> > APIs instead of User APIs is also a problem in my opinion because we're
> not
> > testing exactly what a User would do or can do.
> >
> > To many of us Developers, figuring out what all the rules have configured
> > and done is a much bigger problem than it is to deal with verbose code
> in a
> > setUp method that uses CacheFactory directly. On one hand I want to say,
> do
> > as you prefer but we also have to consider that other Developers need to
> > maintain these tests that are using the Rules, so I will continue to
> > advocate for the writing of tests using Geode User APIs as much as
> possible
> > for the reasons I already stated.
> >
> > On Fri, Nov 9, 2018 at 1:44 PM, Jinmei Liao <jil...@pivotal.io> wrote:
> >
> > > I like using the rules because it reduces boiler plate code and the
> > chance
> > > of not cleaning up properly after your tests. It also make what you are
> > > really trying to do in the test stand out more in the test code.
> > >
> > > On Fri, Nov 9, 2018 at 1:37 PM Kirk Lund <kl...@apache.org> wrote:
> > >
> > > > We need to pull out the DUnit Blackboard from DistributedTestCase and
> > > > repackage it as a BlackboardRule. It makes sense to make that a JUnit
> > > Rule
> > > > because it's not part of Geode or its User APIs but it's really
> useful
> > > for
> > > > distributed testing in general. It's also probably the last useful
> > thing
> > > > that's still in DistributedTestCase and no where else.
> > > >
> > > > On Fri, Nov 9, 2018 at 12:12 PM, Bruce Schuchardt <
> > > bschucha...@pivotal.io>
> > > > wrote:
> > > >
> > > > > I agree with Kirk about the Rules and I agree with Galen about
> moving
> > > > away
> > > > > from the old abstract test classes. I think that is also in the
> > spirit
> > > > of
> > > > > what Kirk is saying.
> > > > >
> > > > > There are also tests that have complicated methods for creating
> > caches
> > > > and
> > > > > regions. These methods have many parameters and are sometimes in
> > > Helper
> > > > > classes. I've found these especially difficult to deal with when
> > > fixing
> > > > > flaky tests because changing one of the Helper methods affects many
> > > > tests.
> > > > >
> > > > >
> > > > > On 11/9/18 11:31 AM, Kirk Lund wrote:
> > > > >
> > > > >> *I would like to encourage all Geode developers to start writing
> > tests
> > > > >> directly against the Geode User APIs* even in DistributedTests.
> I'm
> > no
> > > > >> longer using *CacheRule, ClientCacheRule, ServerStarterRule,
> > > > >> LocatorStarterRule, or ClusterStarterRule* and I'm against
> > encouraging
> > > > >>
> > > > >> their use any longer. I'll explain why below.
> > > > >>
> > > > >> Here's an example for an IntegrationTest that needs a Cache but
> not
> > > any
> > > > >> CacheServers:
> > > > >>
> > > > >> private Cache cache;
> > > > >>
> > > > >> @Before
> > > > >> public void setUp() {
> > > > >> Properties config = new Properties();
> > > > >> config.setProperty(LOCATORS, "");
> > > > >> cache = new CacheFactory(config).create();
> > > > >> }
> > > > >>
> > > > >> @After
> > > > >> public void tearDown() {
> > > > >> cache.close();
> > > > >> }
> > > > >>
> > > > >> That's some pretty simple code and as a Developer, I can tell
> > exactly
> > > > what
> > > > >> it's doing and what the config is.
> > > > >>
> > > > >> Here's an example of the kind of Geode User API code that I use to
> > > > create
> > > > >> Servers in a DistributedTests now:
> > > > >>
> > > > >> private void createServer(String serverName, File serverDir,
> int
> > > > >> locatorPort) {
> > > > >> ServerLauncher.Builder builder = new
> ServerLauncher.Builder();
> > > > >> builder.setMemberName(serverName);
> > > > >> builder.setWorkingDirectory(serverDir.getAbsolutePath());
> > > > >> builder.setServerPort(0);
> > > > >> builder.set(LOCATORS, "localHost[" + locatorPort + "]");
> > > > >> builder.set(DISABLE_AUTO_RECONNECT, "false");
> > > > >> builder.set(ENABLE_CLUSTER_CONFIGURATION, "false");
> > > > >> builder.set(MAX_WAIT_TIME_RECONNECT, "1000");
> > > > >> builder.set(MEMBER_TIMEOUT, "2000");
> > > > >>
> > > > >> serverLauncher = builder.build();
> > > > >> serverLauncher.start();
> > > > >> assertThat(serverLauncher.isRunning()).isTrue();
> > > > >> }
> > > > >>
> > > > >> In particular, I think we should be using ServerLauncher and
> > > > >> LocatorLauncher instead of Rules when we want a full-stack Locator
> > or
> > > > >> full-stack Server that looks like what a User is going to startup.
> > > > >>
> > > > >> Here are my reasons:
> > > > >>
> > > > >> 1) I want to learn and use the Geode User APIs directly, not
> > someone's
> > > > >> (even mine) Testing API that hides the Geode User APIs. If I see a
> > > test
> > > > >> fail, I want to see exactly what was configured and what User APIs
> > > were
> > > > >> used right there in the test without having to open other
> classes. I
> > > > don't
> > > > >> want to have to spend even 15 minutes digging through some JUnit
> > Rule
> > > to
> > > > >> figure out how PDX was configured.
> > > > >>
> > > > >> 2) We need to make sure that the Geode User APIs are easy to use
> and
> > > are
> > > > >> complete. If we're writing tests against Testing APIs instead then
> > we
> > > > >> don't
> > > > >> feel the Users' pain if our API is painful. If the reason to use a
> > > Rule
> > > > is
> > > > >> because our User API is overly-verbose of difficult, then that's
> > even
> > > > more
> > > > >> reason to use the Geode User API, so we recognize that it needs to
> > > > change!
> > > > >>
> > > > >> GemFire had a long history of hiding its User APIs behind
> elaborate
> > > > >> Testing
> > > > >> APIs and we all used these fancy, easier to use, more compact
> > Testing
> > > > >> APIs.
> > > > >> This promotes complicated, inconsistent and potentially incomplete
> > > User
> > > > >> APIs for Users to actually use. The result: difficult to use
> product
> > > > with
> > > > >> difficult to use APIs and User APIs that are missing important
> > things
> > > > that
> > > > >> then Users have to resort to internal APIs to use. I'm strongly
> > > > convinced
> > > > >> that using elaborate Testing APIs is at least largely responsible
> > for
> > > > >> making GemFire and now Geode difficult to use and that's why I'm
> > > pushing
> > > > >> so
> > > > >> hard to write tests with Geode User APIs instead of convenient
> > custom
> > > > >> Rules
> > > > >>
> > > > >> Since I started using ServerLauncher and LocatorLauncher APIs
> > directly
> > > > in
> > > > >> my DistributedTests I made a very important discovery: the User
> has
> > no
> > > > way
> > > > >> to get a reference to the Cache. This is why I recently started a
> > > > >> discussion thread about add getCache and getLocator to these
> > Launcher
> > > > >> APIs.
> > > > >> If we keep using elaborate Testing APIs including custom Geode
> JUnit
> > > > Rules
> > > > >> to hide these APIs, we'll never make these discoveries that I feel
> > are
> > > > >> vital for our Users. We need to make things a LOT easier for the
> > Users
> > > > >> going forward.
> > > > >>
> > > > >> The above is why I think we should be using User APIs in the tests
> > > even
> > > > >> for
> > > > >> setUp and tearDown. Save the custom JUnit Rules for NON-GEODE
> things
> > > > like
> > > > >> configuring JSON or LOG4J or allowing use of ErrorCollector in all
> > > DUnit
> > > > >> VMs.
> > > > >>
> > > > >> Thanks,
> > > > >> Kirk
> > > > >>
> > > > >> On Fri, Nov 9, 2018 at 10:49 AM, Galen O'Sullivan <
> > > > gosulli...@pivotal.io>
> > > > >> wrote:
> > > > >>
> > > > >> I was looking at a test recently that extended JUnit4CacheTestCase
> > and
> > > > >>> only
> > > > >>> used a single server, and changed it to use ClusterStartupRule.
> > > > >>>
> > > > >>> JUnit4CacheTestCase adds additional complexity to
> > > > >>> JUnit4DistributedTestCase
> > > > >>> and with the move to ClusterStartupRule for distributed tests,
> > rather
> > > > >>> than
> > > > >>> class inheritance, I think we should deprecate
> JUnit4CacheTestCase
> > > and
> > > > >>> change the comment to imply that classes should inherit from it
> > just
> > > > >>> because they require a Cache.
> > > > >>>
> > > > >>> Is is worth deprecating JUnit4DistributedTestCase as well and
> > > > encouraging
> > > > >>> the use of ClusterStartupRule instead?
> > > > >>>
> > > > >>> Thanks,
> > > > >>> Galen
> > > > >>>
> > > > >>>
> > > > >
> > > >
> > >
> > >
> > > --
> > > Cheers
> > >
> > > Jinmei
> > >
> >
>
