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
