geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Juan José Ramos <jra...@pivotal.io>
Subject Re: [DISCUSS] Deprecating JUnit4CacheTestCase (and maybe JUnit4DistributedTestCase?)
Date Mon, 12 Nov 2018 12:22:00 GMT
Hello all,

What about mixing both approaches?.
We can use the *Rules* to avoid duplication of code *but re-write them* to
directly use the Geode User APIs instead of the Geode Internal API.
Just for the record, https://issues.apache.org/jira/browse/GEODE-5739 was
created for something similar (use [Server|Locator]Launcher instead of
internal API in [Server|Locator]StarterRule), but it didn't get enough
consent to be merged (leaving aside the huge amount of unrelated changes in
the PR, the idea itself was rejected).
Cheers.


On Fri, Nov 9, 2018 at 11:55 PM Jinmei Liao <jiliao@pivotal.io> wrote:

> Yeah, I guess. But why going out of this way to avoid using rules? Why
> rules are bad? Rules just encapsulate common befores and afters to avoid
> duplicate code in every tests. I don't see why we should avoid using it.
>
> On Fri, Nov 9, 2018, 3:44 PM Galen O'Sullivan <gosullivan@pivotal.io
> wrote:
>
> > 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 <hbales@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 <klund@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 <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
> > > > >
> > > >
> > >
> >
>


-- 
Juan José Ramos Cassella
Senior Technical Support Engineer
Email: jramos@pivotal.io
Office#: +353 21 4238611
Mobile#: +353 87 2074066
After Hours Contact#: +1 877 477 2269
Office Hours: Mon - Thu 08:30 - 17:00 GMT. Fri 08:30 - 16:00 GMT
How to upload artifacts:
https://support.pivotal.io/hc/en-us/articles/204369073
How to escalate a ticket:
https://support.pivotal.io/hc/en-us/articles/203809556

[image: support] <https://support.pivotal.io/> [image: twitter]
<https://twitter.com/pivotal> [image: linkedin]
<https://www.linkedin.com/company/3048967> [image: facebook]
<https://www.facebook.com/pivotalsoftware> [image: google plus]
<https://plus.google.com/+Pivotal> [image: youtube]
<https://www.youtube.com/playlist?list=PLAdzTan_eSPScpj2J50ErtzR9ANSzv3kl>

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