Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/816#discussion_r141606551
--- Diff:
core/src/test/java/org/apache/brooklyn/util/core/task/BasicTasksFutureTest.java
---
@@ -174,29 +175,53 @@ public T call() {
}
@Test
- public void testCancelAfterStartTriggersListenableFuture() throws
Exception {
- doTestCancelTriggersListenableFuture(Duration.millis(50));
+ public void testCancelAfterStartTriggersListenableFutureDynamic()
throws Exception {
+ doTestCancelTriggersListenableFuture(Duration.millis(50), true);
}
@Test
- public void testCancelImmediateTriggersListenableFuture() throws
Exception {
+ public void testCancelImmediateTriggersListenableFutureDynamic()
throws Exception {
// if cancel fires after submit but before it passes to the
executor,
- // that needs handling separately; this doesn't guarantee this
code path,
- // but it happens sometimes (and it should be handled)
- doTestCancelTriggersListenableFuture(Duration.ZERO);
+ // that needs handling separately as it falls into an edge where
the future is set and cancelled
+ // so the executor won't run it, and our wrapper logic doesn't
apply;
+ // this test doesn't guarantee this code path, but makes it likely
enough it happens once in a while.
+ doTestCancelTriggersListenableFuture(Duration.ZERO, true);
}
- public void doTestCancelTriggersListenableFuture(Duration delay)
throws Exception {
- Task<String> t = waitForSemaphore(Duration.TEN_SECONDS, true, "x");
+ @Test
+ public void testCancelBeforeTriggersListenableFutureDynamic() throws
Exception {
+ doTestCancelTriggersListenableFuture(Duration.millis(-50), true);
+ }
+ @Test
+ public void testCancelAfterStartTriggersListenableFutureSimple()
throws Exception {
+ doTestCancelTriggersListenableFuture(Duration.millis(50), true);
+ }
+ @Test
+ public void testCancelImmediateTriggersListenableFutureSimple() throws
Exception {
+ doTestCancelTriggersListenableFuture(Duration.ZERO, false);
+ }
+ @Test
+ public void testCancelBeforeTriggersListenableFutureSimple() throws
Exception {
+ doTestCancelTriggersListenableFuture(Duration.millis(-50), false);
+ }
+ public void doTestCancelTriggersListenableFuture(Duration delay,
boolean dynamic) throws Exception {
--- End diff --
No particularly strong feelings, but this kind of reuse of a parameter to
mean two things (based on whether it's negative or positive) makes things
harder to understand. On a quick look, it feels like you have two parameters:
`delayBeforeSubmit` and `delayBeforeCancel`. But it's just relatively simple
test code.
---
