How about ConcurrencyRule or ConcurrencyTestRule? On Mon, Jul 30, 2018 at 2:56 PM, Galen O'Sullivan <gosulli...@pivotal.io> wrote:
> I like this API. I think it saves a lot of boilerplate, and a lot of subtle > possible mistakes. I think the new API would be a bit cleaner than the > RegionVersionVectorTest.doesNotHangIfOtherThreadChangedVersion that Kirk > mentions. > > I would like to see generics, especially for `public <T> void > runAndExpectValue(Callable<T> callable, <T> value);` > > However, I don't think that toRun should care what the types of the > callables are -- I'd probably have it be a List<Runnable<?>> and put the > wrapped invocations (expecting exception or a particular result) in there. > > I disagree with Dan and think that we should keep `runInSeries`. I think > runInSeries is a good part of the API as if developers get in the habit of > using this API, it will make it easier to set up a test the same way and > run it serially in one case and in parallel in another. > > > Runs all callables in toRun in parallel and fail if threads' conditions > were not met. > > I think it would be clearer to say "fail if any thread's condition is not > met." I think it would be useful to specify whether it will short-circuit > in the serial case and whether, for `runInParallel`, the API will run all > Runnables to completion or may fail to start some or interrupt them early > when some condition fails. > > If you implemented it as a Rule, you could throw an exception if any > Runnables are still running when the test finishes, and make sure whatever > Executor you use is cleaned up. I like it! > > As for name, I agree with Kirk that "Helper" is not a particularly > descriptive name. Perhaps you could call it AsyncTaskAssertionRule or such? > > Best, > Galen > > > On Wed, Jul 18, 2018 at 2:59 PM, Kirk Lund <kl...@apache.org> wrote: > > > I'd love to collaborate on improving ExecutorServiceRule. Let me know if > > you or anyone else is interested. > > > > Honestly, I can't remember why I had it default to one thread unless I > was > > favoring very explicit and tight control over what resources the tests > use. > > Unlimited sounds reasonable for this. > > > > GMSEncryptJUnitTest isn't a good example (for ExecutorServiceRule or for > > testing). When I introduced ExecutorServiceRule, I expected most use > cases > > to involve Future which does handle exceptions, or a developer might use > > ErrorCollector (a JUnit rule). I didn't fix all the issues in > > GMSEncryptJUnitTest > > -- I simply refactored any tests that were using ExecutorService to use > the > > rule when I introduced ExecutorServiceRule. The test is no worse than > > before changing it to use the rule but yes, I see now that the test could > > use some further cleanup. > > > > See the use of Future within > > RegionVersionVectorTest.doesNotHangIfOtherThreadChangedVersion > > (this is the test for which I created rule): > > > > Future<Void> result = executorServiceRule.runAsync(() -> > > rvv.updateLocalVersion(newVersion)); > > ... > > assertThatCode(() -> result.get(2, SECONDS)). > > doesNotThrowAnyException(); > > > > Other than using Futures with the rule, I would have expected a developer > > to use ErrorCollector in an IntegrationTest or SharedErrorCollector in a > > DistributedTest. Ultimately, I imagined that the use of Futures would be > > the primary usage pattern for ExecutorServiceRule and Future does handle > > and rethrow (wrapped within ExecutionException) any exceptions thrown by > > the code being executed. > > > > ExecutorServiceRule, ErrorCollector/SharedErrorCollector and > > AsyncInvocation (which is a Future) could maybe be combined in some > > interesting ways. > > > > On Wed, Jul 18, 2018 at 1:49 PM, Dan Smith <dsm...@pivotal.io> wrote: > > > > > > See our ExecutorServiceRule which is in geode-junit. Is that similar > to > > > > what you're trying to create? Maybe we need just to > > > > modify ExecutorServiceRule instead of introduce yet another Helper > > > class. I > > > > don't see why we need to add another class that is so similar to both > > > > ExecutorServiceRule > > > > and AsyncInvocation -- I vote to evolve those existing classes > instead > > of > > > > adding something new. > > > > > > > > > > I see this as potentially wrapping that ExecutorServiceRule (which, > btw, > > I > > > had no idea existed. Nice!). That rule lets you fire off asynchronous > > > tasks. But generally a test doesn't want to just fire off tasks; it > needs > > > to get the results back and see what happened. For example, if you look > > at > > > GMSEncryptJUnitTest which uses ExecutorServiceRule. It has to use a > > > CountDownLatch to make sure the threads finish, and in fact it never > > checks > > > to see if any of the threads throw an exception! With this helper that > > test > > > could be more readable and would actually fail if it hit an error. > > > > > > Maybe this could be an another rule that embeds and > ExecutorServiceRule, > > or > > > just have a constructor that takes an executor service, to work with > that > > > existing rule? > > > > > > BTW, why does ExecutorServiceRule default to 1 thread? Unlimited > threads > > > seems like a more sensible default for tests that want an executor. > > > > > > -Dan > > > > > >
