geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kl...@apache.org>
Subject Re: [DISCUSS] Deprecating JUnit4CacheTestCase (and maybe JUnit4DistributedTestCase?)
Date Fri, 09 Nov 2018 22:31:14 GMT
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 <jiliao@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 <klund@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 <
> bschuchardt@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 <
> > gosullivan@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message