You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/06/30 13:33:46 UTC
[GitHub] brooklyn-server pull request #226: Expose assertServiceFailed(Entity) and as...
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-server/pull/226
Expose assertServiceFailed(Entity) and assertServiceHealthy(Entity)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-server move_entity_assertions
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/226.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 #226
----
commit 69cce17fdcc1dc79a4e7780179d57ce74010eaf7
Author: Valentin Aitken <bo...@gmail.com>
Date: 2016-06-30T13:32:47Z
Expose assertServiceFailed(Entity) and assertServiceHealthy(Entity)
----
---
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 #226: Expose assertServiceFailed(Entity) and assertSer...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/226
Thanks @bostko - LGTM; merging.
---
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 #226: Expose assertServiceFailed(Entity) and as...
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/226#discussion_r69489506
--- Diff: test-framework/src/test/java/org/apache/brooklyn/test/framework/TestSshCommandTest.java ---
@@ -30,6 +30,8 @@
import static org.apache.brooklyn.test.framework.TestSshCommand.DOWNLOAD_URL;
import static org.apache.brooklyn.test.framework.TestSshCommand.SHELL_ENVIRONMENT;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.apache.brooklyn.core.entity.EntityAsserts.assertServiceFailed;
--- End diff --
This name doesn't match the new method names you've used - `assertEntityHealthy` etc.
Presumably you'd also need to update the calling code.
---
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 #226: Expose assertServiceFailed(Entity) and as...
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/226#discussion_r69454029
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/EntityAsserts.java ---
@@ -223,4 +225,14 @@ public void run() {
});
}
+ public static void assertServiceFailed(Entity test) {
+ assertFalse(test.sensors().get(SERVICE_UP), "Service should be down");
--- End diff --
I prefer the error message you'd get if you called `assertAttributeEquals(test, SERVICE_UP, false)`. That would also cope better with the sensor being null, giving us a nicer message.
---
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 #226: Expose assertServiceFailed(Entity) and as...
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/226#discussion_r69453865
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/EntityAsserts.java ---
@@ -223,4 +225,14 @@ public void run() {
});
}
+ public static void assertServiceFailed(Entity test) {
+ assertFalse(test.sensors().get(SERVICE_UP), "Service should be down");
+ assertEquals(ServiceStateLogic.getExpectedState(test), Lifecycle.ON_FIRE, "Service should be marked on fire");
--- End diff --
Why is this using `getExpectedState`, rather than `assertAttributeEquals(test, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.ON_FIRE)`?
---
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 #226: Expose assertServiceFailed(Entity) and assertSer...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/226
I made it this way because `ServiceStateLogic.getExpectedState` is used in sever tests .
See `TestSshCommandIntegrationTest`, `SimpleShellCommandDeprecatedIntegrationTest`.
When I think about it I see that both are used and may be it is reasonable to add assertion for `SERVICE_STATE_EXPECTED`, at least for `assertServiceHealthy`
@aledsage what do you 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 issue #226: Expose assertServiceFailed(Entity) and assertSer...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/226
@bostko why is it checking the value of `SERVICE_STATE_EXPECTED`, rather than the value of `SERVICE_STATE_ACTUAL`?
---
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 #226: Expose assertServiceFailed(Entity) and assertSer...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/226
@bostko we could include `SERVICE_STATE_EXPECTED` in the `assertServiceHealthy` - no strong feelings. It probably will never say actual is "running" if expected is not also something like running.
Can you also add javadoc, then good to merge. For example:
/**
* Asserts sensors {@code service.isUp} is true, and that {@code service.state} is "running".
* Setting these sensors is common behaviour for entities, but depends on the particular entity
* implementation.
*/
And something similar for `assertServiceFailed`.
---
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 #226: Expose assertServiceFailed(Entity) and as...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/226
---
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 #226: Expose assertServiceFailed(Entity) and assertSer...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/226
@bostko I'm unsure about this. The problem is whether `assertServiceFailed` and `assertServiceHealthy` are general enough for all entities. The name "service" also looks out of place, compared to the other methods (but I do see that it agrees with the name in "SERVICE_UP").
At the very least, we'd need clear javadoc to say what is being asserted. And probably should mark it `@Beta`.
I suspect there are a lot of places in our code that we do this pair of assertions, so overall I do lean towards including it.
---
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.
---