You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by geomacy <gi...@git.apache.org> on 2016/11/24 17:40:44 UTC
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
GitHub user geomacy opened a pull request:
https://github.com/apache/brooklyn-server/pull/464
Avoid uncaught exceptions in testOnDoneCallback
All tasks hit the listener, including tasks for starting the application
etc, so before this change the test logged errors as below. These fail the
assertion on the name, giving the messages below.
Change the test to avoid the assertion on name and to explicitly check for
the two tasks we want to find.
Note had to add the synchronization on 'completedTasks' as without it the
test would occasionally fail (~ one time in a dozen); as the state in the
exec thread wouldn't necessarily be propagated to the main thread without
synchronization.
2016-11-24 10:02:33,097 INFO TESTNG INVOKING: "Surefire test" - org.apache.brooklyn.core.mgmt.internal.EntityExecutionManagerTest.testOnDoneCallback()
2016-11-24 10:02:33,099 INFO TESTNG PASSED: "Surefire test" - org.apache.brooklyn.core.mgmt.internal.EntityExecutionManagerTest.testOnDoneCallback() finished in 2 ms
2016-11-24 10:02:33,100 INFO TESTNG INVOKING CONFIGURATION: "Surefire test" - @AfterMethod org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport.tearDown()
2016-11-24 10:02:33,104 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-4
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-3
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-6
java.lang.AssertionError: expected [foo] but found [[]]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-0
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-9
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-2
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-10
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,116 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-7
java.lang.AssertionError: expected [foo] but found [[[]]]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/geomacy/brooklyn-server test-on-done-callback
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/464.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #464
----
commit cf72ca08ed4fbd733fdd8810a78b642bb836b394
Author: Geoff Macartney <ge...@cloudsoftcorp.com>
Date: 2016-11-24T14:52:28Z
Avoid uncaught exceptions in test.
All tasks hit the listener, including tasks for starting the application
etc, so before this change the test logged errors as below. These fail the
assertion on the name, giving the messages below.
Change the test to avoid the assertion on name and to explicitly check for
the two tasks we want to find.
Note had to add the synchronization on 'completedTasks' as without it the
test would occasionally fail (~ one time in a dozen); as the state in the
exec thread wouldn't necessarily be propagated to the main thread without
synchronization.
2016-11-24 10:02:33,097 INFO TESTNG INVOKING: "Surefire test" - org.apache.brooklyn.core.mgmt.internal.EntityExecutionManagerTest.testOnDoneCallback()
2016-11-24 10:02:33,099 INFO TESTNG PASSED: "Surefire test" - org.apache.brooklyn.core.mgmt.internal.EntityExecutionManagerTest.testOnDoneCallback() finished in 2 ms
2016-11-24 10:02:33,100 INFO TESTNG INVOKING CONFIGURATION: "Surefire test" - @AfterMethod org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport.tearDown()
2016-11-24 10:02:33,104 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-4
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-3
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-6
java.lang.AssertionError: expected [foo] but found [[]]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,105 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-0
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-9
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-2
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,115 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-10
java.lang.AssertionError: expected [foo] but found [null]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
2016-11-24 10:02:33,116 ERROR Uncaught exception in thread brooklyn-execmanager-AxEAJeYA-7
java.lang.AssertionError: expected [foo] but found [[[]]]
at org.testng.Assert.fail(Assert.java:94) ~[testng-6.9.10.jar:na]
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #464: Avoid uncaught exceptions in testOnDoneCallback
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/464
Merged. (Note jenkins failure was for `ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed` which I believe is fixed by https://github.com/apache/brooklyn-server/pull/456, already merged into master.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/464
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89767565
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -82,18 +83,33 @@ public void testOnDoneCallback() throws InterruptedException {
@Override
public void onTaskDone(Task<?> task) {
Assert.assertTrue(task.isDone());
- Assert.assertEquals(task.getUnchecked(), "foo");
- completedTasks.put(task, Duration.sinceUtc(task.getEndTimeUtc()));
- sema4.release();
+ Object result = task.getUnchecked();
+ if(result != null && result.equals("foo")) {
+ synchronized (completedTasks) {
+ completedTasks.put(task, Duration.sinceUtc(task.getEndTimeUtc()));
+ }
+ sema4.release();
+ }
}
});
- Task<String> t1 = em.submit( Tasks.<String>builder().displayName("t1").dynamic(false).body(Callables.returning("foo")).build() );
- t1.getUnchecked();
- Task<String> t2 = em.submit( Tasks.<String>builder().displayName("t2").dynamic(false).body(Callables.returning("foo")).build() );
+ Task<String> t1 = em.submit(
+ Tasks.<String>builder()
+ .displayName("t1")
+ .dynamic(false)
+ .body(Callables.returning("foo"))
+ .build());
+ Task<String> t2 = em.submit(
+ Tasks.<String>builder()
+ .displayName("t2")
+ .dynamic(false)
+ .body(Callables.returning("foo"))
+ .build());
sema4.acquire();
--- End diff --
I much prefer:
```
boolean acquired = sema4.tryAcquire(Asserts.DEFAULT_LONG_TIMEOUT.toMilliseconds(), TimeUnit.MILLISECONDS);
assertTrue(acquired);
```
The current code will hang the build if someone changes something that breaks the test.
Also (for future reference - not worth changing here unless you really want to) a `CountDownLatch` is a more appropriate mechanism in this case - doing `new CountDownLatch(2)` is much simpler/clearer in its intent, compared to `new Semaphore(-1)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #464: Avoid uncaught exceptions in testOnDoneCallback
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the issue:
https://github.com/apache/brooklyn-server/pull/464
Looks good. Couple of minor comments but happy for this to be merged with or without the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89779181
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -68,6 +68,7 @@
import com.google.common.util.concurrent.Callables;
/** Includes many tests for {@link BrooklynGarbageCollector} */
+@Test
--- End diff --
Yes, leave the `groups="Integration"` as-is. We (unfortunately!) use that group to indicate slow running tests that we don't want to have to run every time we are doing a build. Definitely not worth trying to change that in this PR. I started drafting an email to dev@brooklyn about that, which I really need to finish.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89770668
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -68,6 +68,7 @@
import com.google.common.util.concurrent.Callables;
/** Includes many tests for {@link BrooklynGarbageCollector} */
+@Test
--- End diff --
I'll take out the plain `@Test`s - I take it, though, that it's better to leave the `@Test(groups="Integration")` as-is?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89765709
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -68,6 +68,7 @@
import com.google.common.util.concurrent.Callables;
/** Includes many tests for {@link BrooklynGarbageCollector} */
+@Test
--- End diff --
All the tests are annotated - does this have any effect?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #464: Avoid uncaught exceptions in testOnDoneCallback
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/464
@geomacy looks good - only a couple of very minor comments from me. Then good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89767445
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -82,18 +83,33 @@ public void testOnDoneCallback() throws InterruptedException {
@Override
public void onTaskDone(Task<?> task) {
Assert.assertTrue(task.isDone());
- Assert.assertEquals(task.getUnchecked(), "foo");
- completedTasks.put(task, Duration.sinceUtc(task.getEndTimeUtc()));
- sema4.release();
+ Object result = task.getUnchecked();
+ if(result != null && result.equals("foo")) {
--- End diff --
Nitpick: add a space between the `if` and the bracket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89768040
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -68,6 +68,7 @@
import com.google.common.util.concurrent.Callables;
/** Includes many tests for {@link BrooklynGarbageCollector} */
+@Test
--- End diff --
Agreed - seems unnecessary.
It's also a little confusing because some tests in this class are marked with `@Test(groups="Integration")`, which will (I believe) take precedence over the class' test annotation, but makes people pause and think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #464: Avoid uncaught exceptions in testOnDoneCa...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/464#discussion_r89778844
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/EntityExecutionManagerTest.java ---
@@ -69,32 +71,47 @@
import com.google.common.util.concurrent.Callables;
/** Includes many tests for {@link BrooklynGarbageCollector} */
+@Test
public class EntityExecutionManagerTest extends BrooklynAppUnitTestSupport {
private static final Logger LOG = LoggerFactory.getLogger(EntityExecutionManagerTest.class);
-
- @Test
+
public void testOnDoneCallback() throws InterruptedException {
ExecutionManager em = mgmt.getExecutionManager();
BasicExecutionManager bem = (BasicExecutionManager)em;
final Map<Task<?>,Duration> completedTasks = MutableMap.of();
- final Semaphore sema4 = new Semaphore(-1);
+ final CountDownLatch latch = new CountDownLatch(2);
bem.addListener(new ExecutionListener() {
@Override
public void onTaskDone(Task<?> task) {
Assert.assertTrue(task.isDone());
- Assert.assertEquals(task.getUnchecked(), "foo");
- completedTasks.put(task, Duration.sinceUtc(task.getEndTimeUtc()));
- sema4.release();
+ Object result = task.getUnchecked();
+ if (result != null && result.equals("foo")) {
+ synchronized (completedTasks) {
+ completedTasks.put(task, Duration.sinceUtc(task.getEndTimeUtc()));
+ }
+ latch.countDown();
+ }
}
});
- Task<String> t1 = em.submit( Tasks.<String>builder().displayName("t1").dynamic(false).body(Callables.returning("foo")).build() );
- t1.getUnchecked();
- Task<String> t2 = em.submit( Tasks.<String>builder().displayName("t2").dynamic(false).body(Callables.returning("foo")).build() );
- sema4.acquire();
- Assert.assertEquals(completedTasks.size(), 2, "completed tasks are: "+completedTasks);
- completedTasks.get(t1).isShorterThan(Duration.TEN_SECONDS);
- completedTasks.get(t2).isShorterThan(Duration.TEN_SECONDS);
+ Task<String> t1 = em.submit(
+ Tasks.<String>builder()
+ .displayName("t1")
+ .dynamic(false)
+ .body(Callables.returning("foo"))
+ .build());
+ Task<String> t2 = em.submit(
+ Tasks.<String>builder()
+ .displayName("t2")
+ .dynamic(false)
+ .body(Callables.returning("foo"))
+ .build());
+ latch.await(Asserts.DEFAULT_LONG_TIMEOUT.toMilliseconds(), TimeUnit.MILLISECONDS);
--- End diff --
FYI This will return `false` if not satisfied within the given time. But that's fine. The assertion below will then fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #464: Avoid uncaught exceptions in testOnDoneCallback
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/464
Thanks for comments @sjcorbett and @aledsage, will update now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #464: Avoid uncaught exceptions in testOnDoneCallback
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/464
hi @aledsage , @sjcorbett , changes made, rebased, squashed and pushed; hope this look ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---