You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/07/28 08:48:50 UTC
[GitHub] brooklyn-server pull request #277: Fix TestCases, and add TestSensor.abortCo...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/277
Fix TestCases, and add TestSensor.abortConditions
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix/test-cases-strike2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/277.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 #277
----
commit 3a2a7e0b16e8a3deef74e5ecdcbeb34cab8f4906
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T12:40:14Z
Improve TestEffector etc tests
- Make code more concise and clearer
- Make it a lot faster by tweaking timeouts
commit d2d55c379f70050beeeef7dac0a135de97d7d5d3
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T12:54:34Z
TestEffector: handle non-string return types
commit b93e82c7d623bdca853e66b62d4b3c134c31959d
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T13:41:00Z
Test case improvements: config error-handling
commit 96e6269c40d84a314f0ccd60d9ff94f3a31b7ec8
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T13:46:33Z
TestEffector: fix timeout for effector call
commit 88891c838a735502aa1e1fc29833f5cd4f9afb7c
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T15:34:45Z
Refactor TestFrameworkAssertions
commit dc156bed7f5d1fc79d95ad90e6adee41b3797c87
Author: Aled Sage <al...@gmail.com>
Date: 2016-07-26T21:16:30Z
TestSensor: support abortConditions
----
---
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 #277: Fix TestCases, and add TestSensor.abortCondition...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/277
@neykov interesting discussion. The "validConditions" makes a lot of sense. I was imagining we'd list abortConditions as {on-fire, stopping, stopped, destroyed} (which is quite verbose compared to your `validConditions`).
I also like the idea of being able to assert about a different sensor. We can do that in the Java code's `DependentConfiguration` (unrelated, but I used that as inspiration). It's a bit more fiddly to wire in here.
I'm going to leave in the `abortConditions`, and think about adding `validConditions` as well (in a different PR). My one concern about `validConditions` is that it sounds like it could be a valid termination condition, rather than us keeping going until the assert succeeds.
---
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 #277: Fix TestCases, and add TestSensor.abortCo...
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/277#discussion_r72638625
--- Diff: test-framework/src/main/java/org/apache/brooklyn/test/framework/TargetableTestComponentImpl.java ---
@@ -116,4 +125,14 @@ private static Entity getTargetById(final ExecutionContext executionContext, fin
protected <T> T getRequiredConfig(ConfigKey<T> key) {
return checkNotNull(config().get(key), "config %s must not be null", key);
}
+
+ protected void setUpAndRunState(boolean up, Lifecycle status) {
+ if (up) {
+ sensors().set(SERVICE_UP, up);
+ setExpectedState(this, status);
+ } else {
+ setExpectedState(this, status);
+ sensors().set(SERVICE_UP, up);
+ }
--- End diff --
Agreed we should change that. Will it mean that app.start() can return when the service.isUp still equals false? I believe so - e.g. the `TestCase` just calls start sequentially on its children, without waiting for service.isUp.
If so, we'll also need to change our test assertions to "eventually".
I'll hold off doing that until another PR.
---
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 #277: Fix TestCases, and add TestSensor.abortCondition...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/277
To clarify what I meant by "validConditions sounds like it could be a valid termination condition"...
I mean that if someone is writing a test-case or is reading someone else's test-case, then "validConditions" might seem like it could be used as the condition that must be satisfied for the test to complete. So they might try to use that rather than "assertions". But I'm sure we can avoid that with suitable config descriptions. And I can't think of a better name than "validConditions" (without getting ridiculously technical like "validTransientConditions" or some such).
---
Build was previously failing because I'd missed the `AbortError.java` file from my commits! Fixed that 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 #277: Fix TestCases, and add TestSensor.abortCondition...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/277
`testPersistenceVolumeFastNoTrigger` failed in jenkins PR build, but that is unrelated to this PR. I've added a comment to `testPersistenceVolumeFastNoTrigger` to explain what I think the problem is. I've increased the "persistPeriod" to make it more likely to pass.
---
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 #277: Fix TestCases, and add TestSensor.abortCo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/277
---
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 #277: Fix TestCases, and add TestSensor.abortCo...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/277#discussion_r72673817
--- Diff: test-framework/src/main/java/org/apache/brooklyn/test/framework/TargetableTestComponentImpl.java ---
@@ -116,4 +125,14 @@ private static Entity getTargetById(final ExecutionContext executionContext, fin
protected <T> T getRequiredConfig(ConfigKey<T> key) {
return checkNotNull(config().get(key), "config %s must not be null", key);
}
+
+ protected void setUpAndRunState(boolean up, Lifecycle status) {
+ if (up) {
+ sensors().set(SERVICE_UP, up);
+ setExpectedState(this, status);
+ } else {
+ setExpectedState(this, status);
+ sensors().set(SERVICE_UP, up);
+ }
--- End diff --
We are waiting for `service.isUp` in `SoftwareProcess` so makes sense to do the same here for consistency.
---
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 #277: Fix TestCases, and add TestSensor.abortCondition...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/277
After a very quick look I think that's very useful and something I felt the need for. When previously thinking about the problem the approach I came up with is having `validConditions` instead of `abortConditions`. This way you only define the expected states the blueprint will go through instead of any combination of invalid state. For `service.state` that would usually be `STARTING`, `RUNNING`, but where it becomes even more useful is asserting on any random sensor. For example resizing a cluster and asserting the new size - the expected sizes are the initial one and the end one. Using the `abortConditions` here would not be too practical.
Or may be both are useful - would be easier to use one or the other for different use cases.
---
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 #277: Fix TestCases, and add TestSensor.abortCondition...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/277
Merging this now. I agree that `validConditions` is also worth having. We can add that later.
---
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 #277: Fix TestCases, and add TestSensor.abortCo...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/277#discussion_r72601245
--- Diff: test-framework/src/main/java/org/apache/brooklyn/test/framework/TargetableTestComponentImpl.java ---
@@ -116,4 +125,14 @@ private static Entity getTargetById(final ExecutionContext executionContext, fin
protected <T> T getRequiredConfig(ConfigKey<T> key) {
return checkNotNull(config().get(key), "config %s must not be null", key);
}
+
+ protected void setUpAndRunState(boolean up, Lifecycle status) {
+ if (up) {
+ sensors().set(SERVICE_UP, up);
+ setExpectedState(this, status);
+ } else {
+ setExpectedState(this, status);
+ sensors().set(SERVICE_UP, up);
+ }
--- End diff --
As you are updating this anyway would be nice to make the `service.isUp` + `service.state` managed through the notUp + problems maps, instead of directly.
Current approach is valid as well so happy to go this way if you think it's out of scope.
---
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.
---