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 2017/04/05 20:40:14 UTC

[GitHub] brooklyn-server pull request #619: Add greaterThan and lessThan test framewo...

Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/619#discussion_r110021849
  
    --- 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 --
    
    I wonder if we should do any coercion, or perhaps even just give it a go to compare! For example, comparing `(Integer)1` with `(Long)2` might well be possible. However, on balance I think your code is right as it is the simplest (and avoid `ClassCastException`).


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