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
>
