+1 Echo Dales suggestion for Duration over int for time.
-Jake On Wed, Jul 18, 2018 at 12:58 PM Dan Smith <dsm...@pivotal.io> wrote: > Hi Helena, > > Looks great! I think this will make writing multithreaded tests a lot less > error prone. A couple of minor comments below: > > I'm not sure we need runInSeries, someone could do that without using this > test helper. > > Could we make the default timeout something more like 5 minutes? See this > thread https://markmail.org/thread/yrcvdlbixqojl6oh > > > -Dan > > On Wed, Jul 18, 2018 at 12:16 PM, Dale Emery <dem...@pivotal.io> wrote: > > > Hi Helena, > > > > I like it! > > > > A few suggestions: > > > > - For the durations (repeatForDuration() and setTimeout()), consider > using > > a java.time.Duration rather than an int to express the duration. The > > Duration class lets most of the code not have to keep track of the time > > units. The caller creates the Duration using whatever time unit they > want. > > When some other code needs to convert it to a specific time unit, there > are > > methods for that. The rest of the code doesn’t have to care about the > time > > units. And there are methods for adding, subtracting, and comparing > > Durations and Instants, all handling the time units nicely. > > > > - Consider adding a version of runAndExpectValue that takes a Predicate > > instead of a value. And maybe one taking a Hamcrest Matcher. > > > > - Consider specifying the type of the Callable, and preferably for the > > value passed to runAndExpectValue()). And definitely for Predicates and > > Matchers, if you add those methods. > > > > Cheers, > > Dale > > > > > On Jul 18, 2018, at 11:53 AM, Helena Bales <hba...@pivotal.io> wrote: > > > > > > Hello All, > > > > > > Below I have included a proposed API for a concurrency test helper. The > > > goal of this test helper is to make it easier for tests to use multiple > > > threads. I believe providing a helper for this will reduce the number > of > > > tests that incorrectly use threads, and help make our DUnit tests more > > > readable. > > > > > > I would like feedback on the API below. I have provided the method > > > signatures that I plan on implementing, as well as an example usage. > > Please > > > let me know if there are any methods that seem unnecessary or if there > > are > > > any important use cases that I missed here. > > > > > > Thank you, > > > Helena Bales > > > > > > public class ConcurrencyTestHelper { > > > private List<Callable<T>> toRun; > > > private Integer timeout; > > > > > > private final Integer DEFAULT_TIMEOUT = 30; > > > > > > /** > > > * A default constructor that sets the timeout to a default of 30 > > seconds > > > */ > > > public ConcurrencyTestHelper() { > > > toRun = new ArrayList<Callable<T>>(); > > > timeout = DEFAULT_TIMEOUT; > > > } > > > > > > /** > > > * Run the given Runnable and assert that an exception of the given > > > type is thrown. The thread > > > * will be cancelled when the class timeout is reached. > > > * @param runnable a Runnable that throws an exception and responds > > > to cancellation signal > > > * @param exceptionType a Class of Exception that is expected to be > > > thrown by the runnable > > > */ > > > public void runAndExpectException(Runnable runnable, Class > > exceptionType); > > > > > > /** > > > * Run the given Runnable and assert that no exceptions are thrown. > > > The thread will be cancelled > > > * when the class timeout is reached. > > > * @param runnable a Runnable that does not throw an exception and > > > responds to cancellation signal > > > */ > > > public void runAndExpectNoException(Runnable runnable); > > > > > > /** > > > * Run the given Callable and assert that the result of that > > > callable is the given expected value. > > > * The thread will be cancelled when the class timeout is reached. > > > * @param callable a Callable that returns an object, does not throw > > > exceptions and responds to > > > * cancellation signal > > > * @param value an Object that is the expected return value from the > > Callable. > > > */ > > > public void runAndExpectValue(Callable callable, Object value); > > > > > > /** > > > * Run the given runnable in a loop until the rest of the threads > > > complete. Fails if the runnable > > > * throws an exception during any iteration. After the rest of the > > > threads complete the loop will > > > * not restart the runnable. The running iteration will be cancelled > > > when the timeout set on the > > > * class is reached. A cancellation will result in a test failure. > > > * @param runnable a Runnable that does not throw an exception and > > > responds to cancellation signal > > > */ > > > public void repeatUntilAllThreadsComplete(Runnable runnable); > > > > > > /** > > > * Run the given runnable in a loop for some number of iterations. > > > Fails if the runnable throws an > > > * exception in any iteration. After the number of iterations has > > > been met, the loop will not > > > * restart the runnable. The running iteration of the Runnable will > > > be cancelled when the > > > * timeout set on the class is reached, regardless of if the number > > > of desired iterations has > > > * been reached. A cancellation will result in a test failure. > > > * @param runnable a Runnable that does not throw an exception and > > > responds to cancellation signal > > > * @param iterations a maximum number of iterations to repeat for > > > */ > > > public void repeatForIterations(Runnable runnable, Integer > iterations); > > > > > > /** > > > * Run the given runnable in a loop for a certain duration. Fails if > > > the runnable throws an > > > * exception in any iteration. After the duration has been exceeded, > > > the loop will not restart > > > * the runnable, however the current iteration will not be cancelled > > > until the timeout value for > > > * the class has been reached. A cancellation will result in a test > > failure. > > > * @param runnable a Runnable that does not throw exceptions and > > > responds to cancellation signal > > > * @param duration a maximum amount of time to repeat for > > > */ > > > public void repeatForDuration(Runnable runnable, Integer duration); > > > > > > /** > > > * Set the timeout for the threads. After the timeout is reached, > > > the threads will be interrupted > > > * and will throw a CancellationException > > > */ > > > public void setTimeout(Integer seconds) { > > > timeout = seconds; > > > } > > > > > > /** > > > * Runs all callables in toRun in parallel and fail if threads' > > > conditions were not met. Runs > > > * until timeout is reached. > > > * @throws InterruptedException if interrupted before timeout > > > */ > > > public void executeInParallel(); > > > > > > /** > > > * Runs all callables in toRun in the order that they were added and > > > fail if threads' conditions > > > * were not met. Runs until timeout is reached. > > > * @throws InterruptedException if interrupted before timeout > > > */ > > > public void executeInSeries(); > > > } > > > > > > // example usage: > > > new ConcurrentTestHelper() > > > .runAndExpectException(runnable, UnsupportedOperationException. > > class) > > > .runAndExpectNoException(runnable) > > > .runAndExpectValue(callable, result) > > > .repeatUntilAllOtherRunnablesFinish(runnable) > > > .repeatFor(runnable, time) //keep doing this for time amount of > > time > > > .repeat(runnable, iterations) //keep doing this for N times > > > .setTimeout(seconds) //Throws exception if test takes too long > > > .executeInParallel(); > > > > — > > Dale Emery > > dem...@pivotal.io > > > > > > > > > > >
