Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/280#discussion_r19541273
--- Diff:
software/base/src/test/java/brooklyn/entity/brooklynnode/BrooklynNodeIntegrationTest.java
---
@@ -434,11 +435,24 @@ public void testAuthenticationAndHttps() throws
Exception {
Assert.assertEquals(response.getResponseCode(), 200);
}
- @Test(groups="Integration", expectedExceptions =
PropagatedRuntimeException.class)
+ @Test(groups="Integration")
public void testStopPlainThrowsException() throws Exception {
BrooklynNode brooklynNode = setUpBrooklynNodeWithApp();
- brooklynNode.stop();
+ // Not using annotation with `expectedExceptions =
PropagatedRuntimeException.class` because want to
+ // ensure exception comes from stop. On jenkins, was seeing
setUpBrooklynNodeWithApp fail in
+ // testStopAndKillAppsEffector; so can't tell if this method was
really passing!
+ try {
+ brooklynNode.stop();
+ fail("Expected "+brooklynNode+" stop to fail, because has
app");
+ } catch (Exception e) {
+ IllegalStateException ise =
Exceptions.getFirstThrowableOfType(e, IllegalStateException.class);
+ if (ise != null && ise.toString().contains("Can't stop
instance with running applications")) {
+ // success
+ } else {
+ throw e;
+ }
+ }
--- End diff --
I see the intention here, but testing for explicit strings in exception
messages always feels messy. But then so does
`expectedExceptions=PropagatedRuntimeException.class`. While it's arguably
neater to throw (and expect) a custom runtime exception type, since this is a
very narrow test for the class that defines the message I'm prepared to make an
exception (no pun intended).
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
