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 2015/11/24 16:51:07 UTC
[GitHub] incubator-brooklyn pull request: Updates to Test framework asserti...
GitHub user geomacy opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1066
Updates to Test framework assertions to support multiple assertion failures.
At the moment in the test framework, as noted in [#1030](https://github.com/apache/incubator-brooklyn/pull/1030#issuecomment-158995925), when assertions fail it's not easy to see from the error text in the Activity log what went wrong. In particular if there are multiple assertions that failed this isn't apparent.
This update merges the assertion code from SimpleShellCommand and TestFrameworkAssertions to give:
- Updates test framework assertions so that multiple failures are described in the assertion failure message, such as
```
Assertions failed: exit code equals 1 stdout equals bogus stdout contains porkpie
```
- Supports generic 'actual' parameter, so we can avoid doing type coercions to String in
order to check assertions.
- Adds "hasTruthValue" condition.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/geomacy/incubator-brooklyn update-test-assertions-for-multiple-failures
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1066.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 #1066
----
commit d3d120c5c0ed96231cbc7a4034ada8ae387cc9d1
Author: Geoff Macartney <ge...@cloudsoftcorp.com>
Date: 2015-11-24T09:05:08Z
Updates to Test framework assertions to support multiple assertion failures.
Merges the assertion code from SimpleShellCommand and TestFrameworkAssertions to give:
Updates test framework assertions so that multiple failures are described
in the assertion failure message, such as
Assertions failed: exit code equals 1 stdout equals bogus stdout contains porkpie stderr
Supports generic 'actual' parameter, so we can avoid doing type coercions to String in
order to check assertions.
Adds "hasTruthValue" condition.
----
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45933970
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTestImpl.java ---
@@ -228,89 +238,14 @@ private static String getLastPartOfPath(Iterable<String> path, String defaultNam
return scriptName;
}
- private <T> void checkAssertions(AssertionSupport support, Map<?, ?> assertions, String target, T actual) {
- if (null == assertions) {
- return;
- }
- if (null == actual) {
- support.fail(target, "no actual value", "");
- return;
- }
- for (Map.Entry<?, ?> assertion : assertions.entrySet()) {
- String condition = assertion.getKey().toString();
- Object expected = assertion.getValue();
- switch (condition) {
- case EQUALS :
- if (!actual.equals(expected)) {
- support.fail(target, EQUALS, expected);
- }
- break;
- case CONTAINS :
- if (!actual.toString().contains(expected.toString())) {
- support.fail(target, CONTAINS, expected);
- }
- break;
- case IS_EMPTY:
- if (!actual.toString().isEmpty() && truth(expected)) {
- support.fail(target, IS_EMPTY, expected);
- }
- break;
- case MATCHES :
- if (!actual.toString().matches(expected.toString())) {
- support.fail(target, MATCHES, expected);
- }
- break;
- default:
- support.fail(target, "unknown condition", condition);
- }
- }
- }
- private Map<?, ?> exitCodeAssertions() {
- Map<?, ?> assertStatus = getConfig(ASSERT_STATUS);
- if (isEmpty(assertStatus)) {
- assertStatus = ImmutableMap.of(EQUALS, SUCCESS);
+ private List<Map<String, Object>> exitCodeAssertions() {
+ List<Map<String, Object>> assertStatus = getConfig(ASSERT_STATUS);
+ if (assertStatus.isEmpty()) {
+ Map<String, Object> shouldSucceed = ImmutableMap.<String,Object>of(EQUALS, SUCCESS);
--- End diff --
Might be nice to define this as a member variable `DEFAULT_ASSERTION` or the like, for clarity.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46173608
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +42,216 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
--- End diff --
I have a personal preference of not supporting lots of variants. It feels nice sometimes, but then when someone starts looking at different examples and sees different text in each, it looks confusing (*). But no strong feelings.
(*) there are exceptions to that, such as in Duration where we accept `1h` and `1hr` etc.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#issuecomment-159426757
Looks good overall but I will finish a closer review on Wednesday!
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45844605
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -87,21 +90,24 @@
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<List<Map<String, Object>>> ASSERT_STATUS = ConfigKeys.newConfigKey(
--- End diff --
This change doesn't alter the syntax of the assertions in the Yaml, (except for the new SimpleShellCommand which it just brings in to line with the other tests), i.e.
```
assert:
- equals: true
```
But in fact any tests that were written before the new test assertions class were put in wouldn't work now (actually since https://github.com/apache/incubator-brooklyn/pull/1049) as they read
```
assert:
equals: true
```
However I think there are very few such tests, as people had only started using the framework when 1049 was done.
At the same time it does seem a good improvement to make it work with a single map and wrap that in a list if need be. (Is there code already somewhere that does that sort of thing? How can you have a config key that might be a map or might be a list?)
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46172328
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -36,27 +40,10 @@
@ImplementedBy(SimpleShellCommandTestImpl.class)
public interface SimpleShellCommandTest extends BaseTest {
- String TMP_DEFAULT = "/tmp";
-
- /**
- * Equals assertion on command result.
- */
- String EQUALS = "equals";
-
/**
- * String contains assertion on command result.
+ * Default location for temporary files.
*/
- String CONTAINS = "contains";
-
- /**
- * Regex match assertion on command result.
- */
- String MATCHES = "matches";
-
- /**
- * Is-empty match assertion on command result.
- */
- String IS_EMPTY = "isEmpty";
+ String TMP_DEFAULT = "/tmp";
--- End diff --
Actually, I misread the context - this is in main rather than test. But it's the path on the *target* machine rather than the Brooklyn machine, so using `/tmp` is right.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#issuecomment-160699587
Only extremely minor comments from me - the only one that needs addressed is the mutable values for config key defaults, but that wasn't introduced in this PR.
Good to merge now. Comments can be reviewed and selectively addressed 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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46174015
--- Diff: usage/test-framework/src/test/java/org/apache/brooklyn/test/framework/SimpleShellCommandIntegrationTest.java ---
@@ -96,15 +102,70 @@ public void shouldInsistOnJustOneOfCommandAndScript(String command, String scrip
}
}
+ private List<Map<String, Object>> makeAssertions(Map<String, Object> ...maps) {
+ ArrayList<Map<String, Object>> assertions = new ArrayList<>();
+ for (Map<String, Object> map : maps) {
+ assertions.add(map);
+ }
+ return assertions;
+ }
+
+
+
+ @Test(groups = "Integration")
+ public void shouldSucceedUsingSuccessfulExitAsDefaultCondition() {
+ TestEntity testEntity = app.createAndManageChild(EntitySpec.create(TestEntity.class).location(machine));
+
+ SimpleShellCommandTest uptime = app.createAndManageChild(EntitySpec.create(SimpleShellCommandTest.class)
+ .configure(TARGET_ENTITY, testEntity)
+ .configure(COMMAND, "uptime"));
+
+ app.start(ImmutableList.of(loc));
+
+ assertThat(uptime.sensors().get(SERVICE_UP)).isTrue()
+ .withFailMessage("Service should be up");
+ assertThat(ServiceStateLogic.getExpectedState(uptime)).isEqualTo(Lifecycle.RUNNING)
+ .withFailMessage("Service should be marked running");
+ }
+
+
+ @Test(groups = "Integration")
+ public void shouldFailUsingSuccessfulExitAsDefaultCondition() {
+ TestEntity testEntity = app.createAndManageChild(EntitySpec.create(TestEntity.class).location(machine));
+
+ SimpleShellCommandTest uptime = app.createAndManageChild(EntitySpec.create(SimpleShellCommandTest.class)
+ .configure(TARGET_ENTITY, testEntity)
+ .configure(COMMAND, "ls /tmp/bogus-" + Identifiers.randomLong()));
+
+ try {
+ app.start(ImmutableList.of(loc));
--- End diff --
Do we need to fail if the exception was not thrown?
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45845108
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestHttpCallImpl.java ---
@@ -66,31 +68,35 @@ public void start(Collection<? extends Location> locations) {
}
}
- private Supplier<String> buildDataSupplier(final HttpAssertionTarget httpAssertionTarget, final String url) {
-
- switch (httpAssertionTarget) {
+ private void doRequestAndCheckAssertions(Map<String, Duration> flags, List<Map<String, Object>> assertions,
+ HttpAssertionTarget target, final String url) {
+ switch (target) {
case body:
- return new Supplier<String>() {
+ Supplier<String> getBody = new Supplier<String>() {
--- End diff --
Thanks, those are good to know, especially about the risk of the anonymous classes getting into persistence.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45842996
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestHttpCallImpl.java ---
@@ -66,31 +68,35 @@ public void start(Collection<? extends Location> locations) {
}
}
- private Supplier<String> buildDataSupplier(final HttpAssertionTarget httpAssertionTarget, final String url) {
-
- switch (httpAssertionTarget) {
+ private void doRequestAndCheckAssertions(Map<String, Duration> flags, List<Map<String, Object>> assertions,
+ HttpAssertionTarget target, final String url) {
+ switch (target) {
case body:
- return new Supplier<String>() {
+ Supplier<String> getBody = new Supplier<String>() {
--- End diff --
for checks like this which will be immediate this (anonymous class) is how we'd do it (just FYI). if we need this in more than one or two places then I'd create a `ResourceUtils.getResourceAsStringSupplier()` to do this.
btw we normally use `ResourceUtils` for reading URL's because it will use classpath URL's and can be context aware. not important in this case but again just FYI.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46172907
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +42,216 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
+ public static final String EQUAL_TO = "equalTo";
+ public static final String EQUALS = "equals";
+ public static final String MATCHES = "matches";
+ public static final String CONTAINS = "contains";
+ public static final String IS_EMPTY = "isEmpty";
+ public static final String NOT_EMPTY = "notEmpty";
+ public static final String HAS_TRUTH_VALUE = "hasTruthValue";
+ public static final String UNKNOWN_CONDITION = "unknown condition";
+
private TestFrameworkAssertions() {
}
+
/**
- * Evaluates all assertions against dataSupplier
+ * Get assertions tolerantly from a configuration key.
+ * This supports either a simple map of assertions, such as
*
- * @param dataSupplier
- * @param flags
- * @param assertions
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final List<Map<String, Object>> assertions) {
- //Iterate through assert array
- for (final Map<String, Object> assertionsMap : assertions) {
- checkAssertions(dataSupplier, flags, assertionsMap);
- }
+ <pre>
+ assertOut:
+ contains: 2 users
+ matches: .*[\d]* days.*
+ </pre>
+ * or a list of such maps, (which allows you to repeat keys):
+ <pre>
+ assertOut:
+ - contains: 2 users
+ - contains: 2 days
+ </pre>
+ or
+ private static List<Map<String,Object>> getAssertions(ConfigKey<Object> key) {
}
+ */
+ public static List<Map<String, Object>> getAssertions(Entity entity, ConfigKey<Object> key) {
+ Object config = entity.getConfig(key);
+ Maybe<Map<String, Object>> maybeMap = TypeCoercions.tryCoerce(config, new TypeToken<Map<String, Object>>() {});
+ if (maybeMap.isPresent()) {
+ return Collections.singletonList(maybeMap.get());
+ }
- /**
- * Evaluates all assertions against dataSupplier
- *
- * @param dataSupplier
- * @param flags
- * @param assertionsMap
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final Map<String, Object> assertionsMap) {
- for (final Map.Entry<String, Object> assertion : assertionsMap.entrySet()) {
- final Maybe<Predicate<String>> optionalPredicate = getPredicate(assertion.getKey(), assertion.getValue());
- Asserts.succeedsEventually(flags, new PredicateChecker(dataSupplier, optionalPredicate.get()));
+ Maybe<List<Map<String, Object>>> maybeList = TypeCoercions.tryCoerce(config,
+ new TypeToken<List<Map<String, Object>>>() {});
+ if (maybeList.isPresent()) {
+ return maybeList.get();
}
+
+ throw new FatalConfigurationRuntimeException(key.getDescription() + " is not a map or list of maps");
}
- /**
- * Returns the predicate associated with the predicateKey if one exists
- *
- * @param predicateKey
- * @param predicateTarget
- * @return {@link Maybe} of {@Link Predicate}
- */
- public static Maybe<Predicate<String>> getPredicate(final String predicateKey, final Object predicateTarget) {
- if (StringUtils.equalsIgnoreCase("isNull", predicateKey)) {
- return Maybe.of(Predicates.<String>isNull());
- } else if (StringUtils.equalsIgnoreCase("notNull", predicateKey)) {
- return Maybe.of(Predicates.<String>notNull());
- } else if (StringUtils.equalsIgnoreCase("isEqualTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equalTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equals", predicateKey)) {
- return Maybe.of(Predicates.equalTo(TypeCoercions.coerce(predicateTarget.toString(), String.class)));
- } else if (StringUtils.equalsIgnoreCase("matches", predicateKey)) {
- return Maybe.of(buildMatchesPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
- } else if (StringUtils.equalsIgnoreCase("contains", predicateKey)) {
- return Maybe.of(buildContainsPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
+
+ public static <T> void checkAssertions(Map<String,?> flags,
+ Map<String, Object> assertions,
--- End diff --
General convention in Brooklyn is to indent this 8 spaces, rather than aligning with the previous param. No particularly strong feelings, but I do like 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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46172837
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +42,216 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
+ public static final String EQUAL_TO = "equalTo";
+ public static final String EQUALS = "equals";
+ public static final String MATCHES = "matches";
+ public static final String CONTAINS = "contains";
+ public static final String IS_EMPTY = "isEmpty";
+ public static final String NOT_EMPTY = "notEmpty";
+ public static final String HAS_TRUTH_VALUE = "hasTruthValue";
+ public static final String UNKNOWN_CONDITION = "unknown condition";
+
private TestFrameworkAssertions() {
}
+
/**
- * Evaluates all assertions against dataSupplier
+ * Get assertions tolerantly from a configuration key.
+ * This supports either a simple map of assertions, such as
*
- * @param dataSupplier
- * @param flags
- * @param assertions
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final List<Map<String, Object>> assertions) {
- //Iterate through assert array
- for (final Map<String, Object> assertionsMap : assertions) {
- checkAssertions(dataSupplier, flags, assertionsMap);
- }
+ <pre>
+ assertOut:
+ contains: 2 users
+ matches: .*[\d]* days.*
+ </pre>
+ * or a list of such maps, (which allows you to repeat keys):
+ <pre>
+ assertOut:
+ - contains: 2 users
+ - contains: 2 days
+ </pre>
+ or
+ private static List<Map<String,Object>> getAssertions(ConfigKey<Object> key) {
}
+ */
+ public static List<Map<String, Object>> getAssertions(Entity entity, ConfigKey<Object> key) {
+ Object config = entity.getConfig(key);
+ Maybe<Map<String, Object>> maybeMap = TypeCoercions.tryCoerce(config, new TypeToken<Map<String, Object>>() {});
+ if (maybeMap.isPresent()) {
+ return Collections.singletonList(maybeMap.get());
+ }
- /**
- * Evaluates all assertions against dataSupplier
- *
- * @param dataSupplier
- * @param flags
- * @param assertionsMap
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final Map<String, Object> assertionsMap) {
- for (final Map.Entry<String, Object> assertion : assertionsMap.entrySet()) {
- final Maybe<Predicate<String>> optionalPredicate = getPredicate(assertion.getKey(), assertion.getValue());
- Asserts.succeedsEventually(flags, new PredicateChecker(dataSupplier, optionalPredicate.get()));
+ Maybe<List<Map<String, Object>>> maybeList = TypeCoercions.tryCoerce(config,
+ new TypeToken<List<Map<String, Object>>>() {});
+ if (maybeList.isPresent()) {
+ return maybeList.get();
}
+
+ throw new FatalConfigurationRuntimeException(key.getDescription() + " is not a map or list of maps");
--- End diff --
I'd include in error as much context as possible (as long as it doesn't risk logging security info) - e.g. `key.getDescription() + " of " + entity + " must be map or list of maps, but is " + (config == null ? null : config.getClass().getName())`
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46173382
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +42,216 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
+ public static final String EQUAL_TO = "equalTo";
+ public static final String EQUALS = "equals";
+ public static final String MATCHES = "matches";
+ public static final String CONTAINS = "contains";
+ public static final String IS_EMPTY = "isEmpty";
+ public static final String NOT_EMPTY = "notEmpty";
+ public static final String HAS_TRUTH_VALUE = "hasTruthValue";
+ public static final String UNKNOWN_CONDITION = "unknown condition";
+
private TestFrameworkAssertions() {
}
+
/**
- * Evaluates all assertions against dataSupplier
+ * Get assertions tolerantly from a configuration key.
+ * This supports either a simple map of assertions, such as
*
- * @param dataSupplier
- * @param flags
- * @param assertions
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final List<Map<String, Object>> assertions) {
- //Iterate through assert array
- for (final Map<String, Object> assertionsMap : assertions) {
- checkAssertions(dataSupplier, flags, assertionsMap);
- }
+ <pre>
+ assertOut:
+ contains: 2 users
+ matches: .*[\d]* days.*
+ </pre>
+ * or a list of such maps, (which allows you to repeat keys):
+ <pre>
+ assertOut:
+ - contains: 2 users
+ - contains: 2 days
+ </pre>
+ or
+ private static List<Map<String,Object>> getAssertions(ConfigKey<Object> key) {
}
+ */
+ public static List<Map<String, Object>> getAssertions(Entity entity, ConfigKey<Object> key) {
+ Object config = entity.getConfig(key);
+ Maybe<Map<String, Object>> maybeMap = TypeCoercions.tryCoerce(config, new TypeToken<Map<String, Object>>() {});
+ if (maybeMap.isPresent()) {
+ return Collections.singletonList(maybeMap.get());
+ }
- /**
- * Evaluates all assertions against dataSupplier
- *
- * @param dataSupplier
- * @param flags
- * @param assertionsMap
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final Map<String, Object> assertionsMap) {
- for (final Map.Entry<String, Object> assertion : assertionsMap.entrySet()) {
- final Maybe<Predicate<String>> optionalPredicate = getPredicate(assertion.getKey(), assertion.getValue());
- Asserts.succeedsEventually(flags, new PredicateChecker(dataSupplier, optionalPredicate.get()));
+ Maybe<List<Map<String, Object>>> maybeList = TypeCoercions.tryCoerce(config,
+ new TypeToken<List<Map<String, Object>>>() {});
+ if (maybeList.isPresent()) {
+ return maybeList.get();
}
+
+ throw new FatalConfigurationRuntimeException(key.getDescription() + " is not a map or list of maps");
}
- /**
- * Returns the predicate associated with the predicateKey if one exists
- *
- * @param predicateKey
- * @param predicateTarget
- * @return {@link Maybe} of {@Link Predicate}
- */
- public static Maybe<Predicate<String>> getPredicate(final String predicateKey, final Object predicateTarget) {
- if (StringUtils.equalsIgnoreCase("isNull", predicateKey)) {
- return Maybe.of(Predicates.<String>isNull());
- } else if (StringUtils.equalsIgnoreCase("notNull", predicateKey)) {
- return Maybe.of(Predicates.<String>notNull());
- } else if (StringUtils.equalsIgnoreCase("isEqualTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equalTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equals", predicateKey)) {
- return Maybe.of(Predicates.equalTo(TypeCoercions.coerce(predicateTarget.toString(), String.class)));
- } else if (StringUtils.equalsIgnoreCase("matches", predicateKey)) {
- return Maybe.of(buildMatchesPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
- } else if (StringUtils.equalsIgnoreCase("contains", predicateKey)) {
- return Maybe.of(buildContainsPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
+
+ public static <T> void checkAssertions(Map<String,?> flags,
+ Map<String, Object> assertions,
+ String target,
+ final Supplier<T> actualSupplier) {
+
+ AssertionSupport support = new AssertionSupport();
+ checkAssertions(support, flags, assertions, target, actualSupplier);
+ support.validate();
+ }
+
+
+ public static <T> void checkAssertions(Map<String,?> flags,
+ List<Map<String, Object>> assertions,
+ String target,
+ final Supplier<T> actualSupplier) {
+
+ AssertionSupport support = new AssertionSupport();
+ for (Map<String, Object> assertionMap : assertions) {
+ checkAssertions(support, flags, assertionMap, target, actualSupplier);
}
- return Maybe.absent(String.format("No predicate found with signature [%s]", predicateKey));
+ support.validate();
}
- /**
- * Builds a predicate that checks if a string contains the supplied value
- *
- * @param predicateTarget
- * @return {@link Predicate}
- */
- private static Predicate<String> buildContainsPredicate(final String predicateTarget) {
- return new Predicate<String>() {
+ public static <T> void checkAssertions(final AssertionSupport support,
+ Map<String,?> flags,
+ final List<Map<String, Object>> assertions,
+ final String target,
+ final Supplier<T> actualSupplier) {
- @Override
- public boolean apply(@Nullable final String input) {
- return StringUtils.contains(input, predicateTarget);
- }
+ for (Map<String, Object> assertionMap : assertions) {
+ checkAssertions(support, flags, assertionMap, target, actualSupplier);
+ }
+ }
- @Override
- public String toString() {
- return String.format("TestFrameworkAssertions.contains(%s)", predicateTarget);
- }
- };
+ public static <T> void checkAssertions(final AssertionSupport support,
+ Map<String,?> flags,
+ final Map<String, Object> assertions,
+ final String target,
+ final Supplier<T> actualSupplier) {
+
+ if (null == assertions) {
+ return;
+ }
+ try {
+ Asserts.succeedsEventually(flags, new Runnable() {
+ @Override
+ public void run() {
+ T actual = actualSupplier.get();
+ checkActualAgainstAssertions(support, assertions, target, actual);
+ }
+ });
+ } catch (Throwable t) {
--- End diff --
I'm very wary of swallowing the wrong kind of exception - e.g. if it was an `InterruptedException`, then important the thread is left in an interrupted state (see http://www.ibm.com/developerworks/library/j-jtp05236/).
Perhaps do `support.fail(t); Exceptions.propagateIfFatal(t);`?
(but I haven't checked what support.fail does).
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45934617
--- Diff: usage/test-framework/src/test/java/org/apache/brooklyn/test/framework/TestFrameworkAssertionsTest.java ---
@@ -19,6 +19,7 @@
package org.apache.brooklyn.test.framework;
import com.google.common.base.Supplier;
+import org.apache.brooklyn.test.Asserts;
--- End diff --
Not in the diff, but `import static org.assertj.core.api.Assertions.assertThat;` is unused.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45843746
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +35,179 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
+ public static final String EQUAL_TO = "equalTo";
+ public static final String EQUALS = "equals";
+ public static final String MATCHES = "matches";
--- End diff --
in time (not now) i'd like to introduce aliases across brooklyn, and then allow `matches_regex` and `regex` here. `matches` is too vague, is it literal, is it glob, is it regex. but okay for now (and i see you've not changed that in this PR anyway).
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46173056
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java ---
@@ -39,122 +42,216 @@
* @author m4rkmckenna on 11/11/2015.
*/
public class TestFrameworkAssertions {
- private static final Logger LOG = LoggerFactory.getLogger(TestFrameworkAssertions.class);
+
+ public static final String IS_NULL = "isNull";
+ public static final String NOT_NULL = "notNull";
+ public static final String IS_EQUAL_TO = "isEqualTo";
+ public static final String EQUAL_TO = "equalTo";
+ public static final String EQUALS = "equals";
+ public static final String MATCHES = "matches";
+ public static final String CONTAINS = "contains";
+ public static final String IS_EMPTY = "isEmpty";
+ public static final String NOT_EMPTY = "notEmpty";
+ public static final String HAS_TRUTH_VALUE = "hasTruthValue";
+ public static final String UNKNOWN_CONDITION = "unknown condition";
+
private TestFrameworkAssertions() {
}
+
/**
- * Evaluates all assertions against dataSupplier
+ * Get assertions tolerantly from a configuration key.
+ * This supports either a simple map of assertions, such as
*
- * @param dataSupplier
- * @param flags
- * @param assertions
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final List<Map<String, Object>> assertions) {
- //Iterate through assert array
- for (final Map<String, Object> assertionsMap : assertions) {
- checkAssertions(dataSupplier, flags, assertionsMap);
- }
+ <pre>
+ assertOut:
+ contains: 2 users
+ matches: .*[\d]* days.*
+ </pre>
+ * or a list of such maps, (which allows you to repeat keys):
+ <pre>
+ assertOut:
+ - contains: 2 users
+ - contains: 2 days
+ </pre>
+ or
+ private static List<Map<String,Object>> getAssertions(ConfigKey<Object> key) {
}
+ */
+ public static List<Map<String, Object>> getAssertions(Entity entity, ConfigKey<Object> key) {
+ Object config = entity.getConfig(key);
+ Maybe<Map<String, Object>> maybeMap = TypeCoercions.tryCoerce(config, new TypeToken<Map<String, Object>>() {});
+ if (maybeMap.isPresent()) {
+ return Collections.singletonList(maybeMap.get());
+ }
- /**
- * Evaluates all assertions against dataSupplier
- *
- * @param dataSupplier
- * @param flags
- * @param assertionsMap
- */
- public static void checkAssertions(final Supplier<String> dataSupplier, final Map flags, final Map<String, Object> assertionsMap) {
- for (final Map.Entry<String, Object> assertion : assertionsMap.entrySet()) {
- final Maybe<Predicate<String>> optionalPredicate = getPredicate(assertion.getKey(), assertion.getValue());
- Asserts.succeedsEventually(flags, new PredicateChecker(dataSupplier, optionalPredicate.get()));
+ Maybe<List<Map<String, Object>>> maybeList = TypeCoercions.tryCoerce(config,
+ new TypeToken<List<Map<String, Object>>>() {});
+ if (maybeList.isPresent()) {
+ return maybeList.get();
}
+
+ throw new FatalConfigurationRuntimeException(key.getDescription() + " is not a map or list of maps");
}
- /**
- * Returns the predicate associated with the predicateKey if one exists
- *
- * @param predicateKey
- * @param predicateTarget
- * @return {@link Maybe} of {@Link Predicate}
- */
- public static Maybe<Predicate<String>> getPredicate(final String predicateKey, final Object predicateTarget) {
- if (StringUtils.equalsIgnoreCase("isNull", predicateKey)) {
- return Maybe.of(Predicates.<String>isNull());
- } else if (StringUtils.equalsIgnoreCase("notNull", predicateKey)) {
- return Maybe.of(Predicates.<String>notNull());
- } else if (StringUtils.equalsIgnoreCase("isEqualTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equalTo", predicateKey)
- || StringUtils.equalsIgnoreCase("equals", predicateKey)) {
- return Maybe.of(Predicates.equalTo(TypeCoercions.coerce(predicateTarget.toString(), String.class)));
- } else if (StringUtils.equalsIgnoreCase("matches", predicateKey)) {
- return Maybe.of(buildMatchesPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
- } else if (StringUtils.equalsIgnoreCase("contains", predicateKey)) {
- return Maybe.of(buildContainsPredicate(TypeCoercions.coerce(predicateTarget, String.class)));
+
+ public static <T> void checkAssertions(Map<String,?> flags,
+ Map<String, Object> assertions,
+ String target,
+ final Supplier<T> actualSupplier) {
+
+ AssertionSupport support = new AssertionSupport();
--- End diff --
Minor: perhaps just call `checkAssertions(flags, ImmutableList.of(assertions), target, actualSupplier)`
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45849941
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -87,21 +90,24 @@
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<List<Map<String, Object>>> ASSERT_STATUS = ConfigKeys.newConfigKey(
--- End diff --
Ok, thanks!
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45843346
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/TestHttpCallImpl.java ---
@@ -66,31 +68,35 @@ public void start(Collection<? extends Location> locations) {
}
}
- private Supplier<String> buildDataSupplier(final HttpAssertionTarget httpAssertionTarget, final String url) {
-
- switch (httpAssertionTarget) {
+ private void doRequestAndCheckAssertions(Map<String, Duration> flags, List<Map<String, Object>> assertions,
+ HttpAssertionTarget target, final String url) {
+ switch (target) {
case body:
- return new Supplier<String>() {
+ Supplier<String> getBody = new Supplier<String>() {
@Override
public String get() {
return HttpTool.getContent(url);
}
};
+ TestFrameworkAssertions.checkAssertions(flags, assertions, target.toString(), getBody);
+ break;
case status:
- return new Supplier<String>() {
+ Supplier<Integer> getStatusCode = new Supplier<Integer>() {
@Override
- public String get() {
+ public Integer get() {
try {
- return String.valueOf(HttpTool.getHttpStatusCode(url));
+ return HttpTool.getHttpStatusCode(url);
} catch (Exception e) {
- LOG.error("HTTP call to [{}] failed due to [{}] ... returning Status code [0 - Unreachable]", url, e.getMessage());
- return "0";
+ LOG.error("HTTP call to [{}] failed due to [{}] ... returning Status code [500]",
--- End diff --
definitely not ERROR level, since here we're just in a status check.
normally in catch blocks where we swallow generic exceptions we include
Exceptions.propagateIfFatal(e);
in case it's a severe exception. but here' i'd simply propagate it regardless (`Exceptions.propagate(e)') and make sure the wrapper code treats errors appropriately. returning `500` is dishonest :) .
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46171793
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/BaseTest.java ---
@@ -47,16 +47,16 @@
ConfigKey<String> TARGET_ID = ConfigKeys.newStringConfigKey("targetId", "Id of the entity under test");
/**
- * The assertions to be made
+ * The assertions to be made.
*/
- ConfigKey<List<Map<String, Object>>> ASSERTIONS = ConfigKeys.newConfigKey(
- new TypeToken<List<Map<String, Object>>>() {},
- "assert", "Assertions to be evaluated", new ArrayList<Map<String, Object>>());
+ ConfigKey<Object> ASSERTIONS = ConfigKeys.newConfigKey(Object.class, "assert", "Assertions to be evaluated",
+ new ArrayList<Map<String, Object>>());
--- End diff --
Personal preference for the default value of the config key to be immutable. Otherwise if someone modifies it, things get very strange (it changes the default for all other entities that use it, until Brooklyn is restarted!)
I'd therefore use `ImmutableList.of()` (presumably the generics don't matter, now that it's change to be a config key of type `Object`).
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45934555
--- Diff: usage/test-framework/src/test/java/org/apache/brooklyn/test/framework/SimpleShellCommandIntegrationTest.java ---
@@ -28,7 +28,9 @@
import org.apache.brooklyn.location.localhost.LocalhostMachineProvisioningLocation;
import org.apache.brooklyn.location.ssh.SshMachineLocation;
import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.collections.MutableMap;
--- End diff --
Not in the diff, but `import com.google.common.collect.ImmutableMap;` is unused.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45842448
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTestImpl.java ---
@@ -107,13 +107,23 @@ protected void handle(Result result) {
LOG.debug("{}, Result is {}\nwith output [\n{}\n] and error [\n{}\n]", new Object[] {
this, result.getExitCode(), shorten(result.getStdout()), shorten(result.getStderr())
});
+ ImmutableMap<String, Duration> flags = ImmutableMap.of("timeout", getConfig(TIMEOUT));
AssertionSupport support = new AssertionSupport();
- checkAssertions(support, exitCodeAssertions(), "exit code", result.getExitCode());
- checkAssertions(support, getConfig(ASSERT_OUT), "stdout", result.getStdout());
- checkAssertions(support, getConfig(ASSERT_ERR), "stderr", result.getStderr());
+ TestFrameworkAssertions.checkAssertions(support, flags, exitCodeAssertions(), "exit code", supply(result.getExitCode()));
+ TestFrameworkAssertions.checkAssertions(support, flags, getConfig(ASSERT_OUT), "stdout", supply(result.getStdout()));
+ TestFrameworkAssertions.checkAssertions(support, flags, getConfig(ASSERT_ERR), "stderr", supply(result.getStderr()));
support.validate();
}
+ private static <T> Supplier<T> supply(final T t) {
--- End diff --
see `Suppliers.ofInstance`. in fact for most such helpers you'll probably find it exists. worth asking.
also, avoid anonymous classes except for very transient operations. if they get in to config values and get persisted the persistence gets ugly!
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#issuecomment-159544934
a few minor comments from me
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45849884
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -87,21 +90,24 @@
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<List<Map<String, Object>>> ASSERT_STATUS = ConfigKeys.newConfigKey(
--- End diff --
sadly you can't do `ConfigKey<Map | List>` :) so the way is just `ConfigKey<Object>` and use a static method to coerce the result.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#issuecomment-159767552
Tests all pass, LGTM after @ahgittin's comments are resolved (mine are minor and could be merged without being addressed).
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46172019
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -36,27 +40,10 @@
@ImplementedBy(SimpleShellCommandTestImpl.class)
public interface SimpleShellCommandTest extends BaseTest {
- String TMP_DEFAULT = "/tmp";
-
- /**
- * Equals assertion on command result.
- */
- String EQUALS = "equals";
-
/**
- * String contains assertion on command result.
+ * Default location for temporary files.
*/
- String CONTAINS = "contains";
-
- /**
- * Regex match assertion on command result.
- */
- String MATCHES = "matches";
-
- /**
- * Is-empty match assertion on command result.
- */
- String IS_EMPTY = "isEmpty";
+ String TMP_DEFAULT = "/tmp";
--- End diff --
You can use `org.apache.brooklyn.util.os.Os.tmp()`. But it doesn't matter given that this is just for tests. So consider this just information, rather than needing changed.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45793815
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -19,6 +19,7 @@
package org.apache.brooklyn.test.framework;
import com.google.common.collect.Maps;
--- End diff --
Unused import
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r46172079
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -81,27 +68,35 @@
*/
@SetFromFlag("runDir")
ConfigKey<String> RUN_DIR = newConfigKey(String.class, "run.dir", "directory where downloaded scripts should be run from");
+
+
+ /**
+ * If no assertions are configured in the test then the default is this assertion that exit status of the command
+ * is zero (successful).
+ */
+ Map<String, Object> DEFAULT_ASSERTION = ImmutableMap.<String,Object>of(TestFrameworkAssertions.EQUALS, 0);
+
/**
* Assertions on the exit code of the simple command.
*
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<Object> ASSERT_STATUS = ConfigKeys.newConfigKey(Object.class, "assert.status", "Assertions on command exit code",
+ new ArrayList<Map<String, Object>>());
--- End diff --
Again, it's good if config key default values are immutable. Same below.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#issuecomment-159929897
Thanks @ahgittin, @mikezaccardo, code updated as per comments above.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1066
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45794188
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -87,21 +90,24 @@
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<List<Map<String, Object>>> ASSERT_STATUS = ConfigKeys.newConfigKey(
+ new TypeToken<List<Map<String, Object>>>() {},
+ "assert.status", "Assertions on command exit code", new ArrayList<Map<String, Object>>());
--- End diff --
Perhaps use `Collections.emptyList()` for an immutable empty list as the default value here and below.
It looks the list is replaced and never added to directly so this should be fine.
---
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] incubator-brooklyn pull request: Updates to Test framework asserti...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1066#discussion_r45842311
--- Diff: usage/test-framework/src/main/java/org/apache/brooklyn/test/framework/SimpleShellCommandTest.java ---
@@ -87,21 +90,24 @@
* If not explicitly configured, the default assertion is a non-zero exit code.
*/
@SetFromFlag("assertStatus")
- ConfigKey<Map> ASSERT_STATUS = ConfigKeys.newConfigKey(Map.class, "assert.status",
- "Assertions on command exit code", Maps.newLinkedHashMap());
+ ConfigKey<List<Map<String, Object>>> ASSERT_STATUS = ConfigKeys.newConfigKey(
--- End diff --
do the old tests still work? and do we want to be this strict or make best effort to understand -- e.g. if this is `ConfigKey<Object>` we can have a method e.g. `getAssertions(ASSERT_STATUS)` which retrieves it and creates a list of maps, but which would be more forgiving, e.g. if it's a single map we could wrap it in a list.
this is new so i don't mind either way. i prefer making best-efforts to parse people's intentions and warning but still working if they're wrong (especially if we're changing a syntax which people are already using, as we are here). however other people in the community prefer fail fast even on user input so either is fine...
---
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.
---