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