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