I have a PR up to deprecate these classes and several other distributed test classes that use inheritance (I actually forgot to mention it here when I first put it up, whoops): https://github.com/apache/geode/pull/2841 I intend to merge it Monday evening if there are no objections.
I think I'll try to use the public facing APIs the next time I write a new test and see how it goes. Thanks, Galen On Mon, Nov 12, 2018 at 8:41 AM Jinmei Liao <jil...@pivotal.io> wrote: > I feel like we are mixing two topic in this discussion. One is to improve > our user API, and one is to write clean tests. Doing one doesn't have to > sacrifice the other. If our rules are using internal APIs, then it's a > chance to check if we can improve our APIs like Kirk has done. But stop > using the rules or change the rules to use a more verbose API should not > the way to go. My 2 cents. > > On Mon, Nov 12, 2018 at 4:22 AM Juan José Ramos <jra...@pivotal.io> wrote: > > > 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> > > > > > -- > Cheers > > Jinmei >
