ExecutorServiceRule is much more useful in an integration test or
distributed test. RegionVersionVectorTest (unit test) is a terrible example.
The following are much better examples...
PartitionedRegionCreationDUnitTest which uses a CompletableFuture with it:
CompletableFuture<Void> createdAccessor = executorServiceRule.runAsync(
() -> createPRAccessorConcurrently(signalRegionName, prNamePrefix,
numberOfRegions));
LockServiceLeakTest:
@Test
public void basicDLockUsage() throws InterruptedException,
ExecutionException, TimeoutException {
Lock lock = testRegion.getDistributedLock("testLockName");
lock.lockInterruptibly();
Future<Boolean> future = executorServiceRule.submit(() ->
lock.tryLock());
assertFalse("should not be able to get lock from another thread",
future.get(5, TimeUnit.SECONDS));
assertTrue("Lock is reentrant", lock.tryLock());
// now locked twice.
future = executorServiceRule.submit(() -> lock.tryLock());
assertFalse("should not be able to get lock from another thread",
future.get(5, TimeUnit.SECONDS));
lock.unlock();
future = executorServiceRule.submit(() -> lock.tryLock());
assertFalse("should not be able to get lock from another thread",
future.get());
lock.unlock();
future = executorServiceRule.submit(() -> {
boolean locked = lock.tryLock();
if (!locked) {
return false;
}
lock.unlock();
return true;
});
assertTrue("Another thread can now take out the lock", future.get(5,
TimeUnit.SECONDS));
DLockService lockService = (DLockService) testRegion.getLockService();
Collection<DLockToken> tokens = lockService.getTokens();
for (DLockToken token : tokens) {
assertEquals(0, token.getUsageCount());
}
}
FileProcessControllerIntegrationTest which shows proper error handling
using the JUnit ErrorCollector rule:
@Test
public void statusShouldReturnJsonFromStatusFile() throws Exception {
// given: FileProcessController with pidFile containing real pid
new IntegerFileWriter(pidFile).writeToFile(pid);
FileProcessController controller = new FileProcessController(params,
pid, 2, MINUTES);
// when: status is called in one thread
executorServiceRule.execute(() -> {
try {
statusRef.set(controller.status());
} catch (Exception e) {
errorCollector.addError(e);
}
});
// and: json is written to the status file
new StringFileWriter(statusFile).writeToFile(STATUS_JSON);
// then: returned status should be the json in the file
await().until(() -> assertThat(statusRef.get()).isEqualTo(STATUS_JSON));
}
InterruptDiskJUnitTest which shows interrupt testing (I would probably
change this test to also use Awaitility):
@Test
public void testDRPutWithInterrupt() throws Throwable {
Callable doPuts = new Callable() {
@Override
public Object call() {
puttingThread = Thread.currentThread();
long end = System.nanoTime() +
TimeUnit.MILLISECONDS.toNanos(MAX_WAIT);
while (!Thread.currentThread().isInterrupted()) {
region.put(0, nextValue.incrementAndGet());
if (System.nanoTime() > end) {
fail("Did not get interrupted in 60 seconds");
}
}
return null;
}
};
Future result = executorServiceRule.submit(doPuts);
Thread.sleep(50);
long end = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(MAX_WAIT);
while (puttingThread == null) {
Thread.sleep(50);
if (System.nanoTime() > end) {
fail("Putting thread not set in 60 seconds");
}
}
puttingThread.interrupt();
result.get(60, TimeUnit.SECONDS);
assertEquals(nextValue.get(), region.get(0));
}
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
> > >
> >
>
