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 <jil...@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 <gosulli...@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 <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
> > > > >
> > > >
> > >
> >
>
--
Juan José Ramos Cassella
Senior Technical Support Engineer
Email: jra...@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>
