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