brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sjcorbett <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #619: Add greaterThan and lessThan test framewo...
Date Thu, 06 Apr 2017 12:04:37 GMT
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/619#discussion_r110143902
  
    --- Diff: test-framework/src/main/java/org/apache/brooklyn/test/framework/TestFrameworkAssertions.java
---
    @@ -287,140 +292,86 @@ public Boolean call() {
         protected static <T> void checkActualAgainstAssertions(Map<String, ?>
assertions,
                 String target, T actual) {
             for (Map.Entry<String, ?> assertion : assertions.entrySet()) {
    -            String condition = assertion.getKey().toString();
    +            String condition = assertion.getKey();
                 Object expected = assertion.getValue();
    -            switch (condition) {
    -
    -                case IS_EQUAL_TO :
    -                case EQUAL_TO :
    -                case EQUALS :
    -                    if (null == actual || !actual.equals(expected)) {
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_EQUAL :
    -                    if (Objects.equals(actual, expected)) {
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -                    
    -                case IS_NULL :
    -                    if (isTrue(expected) != (null == actual)) {
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_NULL :
    -                    if (isTrue(expected) != (null != actual)) {
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case CONTAINS :
    -                    if (null == actual || !actual.toString().contains(expected.toString()))
{
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case IS_EMPTY :
    -                    if (isTrue(expected) != (null == actual || Strings.isEmpty(actual.toString())))
{
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_EMPTY :
    -                    if (isTrue(expected) != ((null != actual && Strings.isNonEmpty(actual.toString()))))
{
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case MATCHES :
    -                    if (null == actual || !actual.toString().matches(expected.toString()))
{
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case HAS_TRUTH_VALUE :
    -                    if (isTrue(expected) != isTrue(actual)) {
    -                        failAssertion(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                default:
    -                    failAssertion(target, UNKNOWN_CONDITION, condition, actual);
    +            if (!knownCondition(condition)) {
    +                failAssertion(target, UNKNOWN_CONDITION, expected, actual);
    +            } else if (!conditionHolds(condition, actual, expected)) {
    +                failAssertion(target, condition, expected, actual);
                 }
             }
         }
     
         protected static <T> void checkActualAgainstAbortConditions(Map<String,
?> assertions, String target, T actual) {
             for (Map.Entry<String, ?> assertion : assertions.entrySet()) {
    -            String condition = assertion.getKey().toString();
    +            String condition = assertion.getKey();
                 Object expected = assertion.getValue();
    -            switch (condition) {
    -
    -                case IS_EQUAL_TO :
    -                case EQUAL_TO :
    -                case EQUALS :
    -                    if (null != actual && actual.equals(expected)) {
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_EQUAL :
    -                    if (!Objects.equals(actual, expected)) {
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -                    
    -                case IS_NULL :
    -                    if (isTrue(expected) == (null == actual)) {
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_NULL :
    -                    if (isTrue(expected) == (null != actual)) {
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case CONTAINS :
    -                    if (null != actual && actual.toString().contains(expected.toString()))
{
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case IS_EMPTY :
    -                    if (isTrue(expected) == (null == actual || Strings.isEmpty(actual.toString())))
{
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case NOT_EMPTY :
    -                    if (isTrue(expected) == ((null != actual && Strings.isNonEmpty(actual.toString()))))
{
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case MATCHES :
    -                    if (null != actual && actual.toString().matches(expected.toString()))
{
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                case HAS_TRUTH_VALUE :
    -                    if (isTrue(expected) == isTrue(actual)) {
    -                        abort(target, condition, expected, actual);
    -                    }
    -                    break;
    -
    -                default:
    -                    abort(target, condition, expected, actual);
    +            if (!knownCondition(condition)) {
    +                abort(target, UNKNOWN_CONDITION, expected, actual);
    +            } else if (conditionHolds(condition, actual, expected)) {
    +                abort(target, condition, expected, actual);
                 }
             }
         }
    -    
    +
    +    private static boolean conditionHolds(String condition, Object actual, Object expected)
{
    +        switch (condition) {
    +        case IS_EQUAL_TO:
    +        case EQUAL_TO:
    +        case EQUALS:
    +            return null != actual && actual.equals(expected);
    +        case NOT_EQUAL:
    +            return !Objects.equals(actual, expected);
    +        case IS_NULL:
    +            return isTrue(expected) == (null == actual);
    +        case NOT_NULL:
    +            return isTrue(expected) == (null != actual);
    +        case CONTAINS:
    +            return null != actual && actual.toString().contains(expected.toString());
    +        case IS_EMPTY:
    +            return isTrue(expected) == (null == actual || Strings.isEmpty(actual.toString()));
    +        case NOT_EMPTY:
    +            return isTrue(expected) == ((null != actual && Strings.isNonEmpty(actual.toString())));
    +        case MATCHES:
    +            return null != actual && actual.toString().matches(expected.toString());
    +        case HAS_TRUTH_VALUE:
    +            return isTrue(expected) == isTrue(actual);
    +        case GREATER_THAN:
    +            return canCompare(actual, expected) && compare(actual, expected)
> 0;
    +        case LESS_THAN:
    +            return canCompare(actual, expected) && compare(actual, expected)
< 0;
    +        default:
    +            return false;
    +        }
    +    }
    +
    +    private static boolean knownCondition(String condition) {
    +        // The conditions should really be an enum!
    +        Set<String> allConditions = ImmutableSet.of(
    +                IS_NULL, NOT_NULL, IS_EQUAL_TO, EQUAL_TO, EQUALS, NOT_EQUAL,
    +                MATCHES, CONTAINS, IS_EMPTY, NOT_EMPTY, HAS_TRUTH_VALUE,
    +                GREATER_THAN, LESS_THAN, UNKNOWN_CONDITION);
    +        return allConditions.contains(condition);
    +    }
    +
    +    /** @return True if actual and expected are both non-null instances of {@code Comparable<T>}.
*/
    +    private static boolean canCompare(@Nullable Object actual, @Nullable Object expected)
{
    +        return actual != null
    +                && expected != null
    +                && actual instanceof Comparable
    +                && actual.getClass().equals(expected.getClass());
    --- End diff --
    
    Interesting one. This implementation has already bitten me: "webapp.reqs.perSec.last expected
greaterThan 80 but found 96.8063872255489". I'm inclined to leave this as-is until there are
more complaints. I do think that this is to restrictive though - `actual` and `expected` could
be different types if `Comparable` is implemented by a shared superclass. Don't think it'll
be a common problem though.


---
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.
---

Mime
View raw message