Test improvement: removed conditional test logic (test smell)

[eas5]

This is a test improvement.

Problem:
Conditions within the test method will alter the behavior of the test and its expected output and would lead to situations where (I) the test fails to detect defects in the production method since test statements were not executed as a condition was not met, or (II) the test passes without executing the test steps.

Solution:
In this test class, we changed the conditional logic (which tests for a precondition) for an assumption, which will ignore the test execution if not met.

Result (Sample):
Before:

@Test
public void removeNullFromKeySet()
{
    if (this.newMap() instanceof ConcurrentMap || this.newMap() instanceof SortedMap)
    {
        return;
    }
    ...
}

After:

@Test
public void removeNullFromKeySet()
{
    Assume.assumeFalse(this.newMap() instanceof ConcurrentMap || this.newMap() instanceof SortedMap);
    ...
}

[motlin]

In my experience, failed assumptions are noisy. They show up in the IDE and maybe also in console output, as if they are TODOs (almost like ignored tests).

[eas5]

A failed assumption indeed causes test execution to be ignored and may be shown in the IDE and console output.
The idea is not having a passed test if this test was not executed, which is the current behavior. But if the current behavior is indeed the desired one, then verifying preconditions with assumptions may not be the case.

An alternative solution, but also noisy, is to use assumeFalse(String message, boolean b) passing, a "Test no applicable to this class" message to the AssumptionViolatedException. This way the execution log would not look like a TODO list. But again, this noisy behavior may not be the desired one.

[donraab]

Interesting idea and discussion. If we put assumptions such as these @motlin , they can act as reminders to override the behavior in the subclass tests for ConcurrentMap and SortMap. Once the overrides are in, the assumptions should never fail, correct?

[motlin]

@donraab I'm not sure I understand the question. But there's a bit more context I'd like to share. Failed assumptions are implemented almost the same way as failed tests. That is, they throw a special subclass of Error.

    public static <T> void assumeThat(T actual, Matcher<T> matcher) {
        if (!matcher.matches(actual)) {
            throw new AssumptionViolatedException(actual, matcher);
        }
    }

Say you write a test that makes some assertions, makes an assumption, and then makes more assertions. For example, assert that Map.put() behaves a certain way, and assuming the map can tolerate null keys, it also conforms to additional assertions.

This makes failed assumptions similar to expected exceptions.

Because of all this, I'm more likely to use return or continue than Assumptions. Just like using try/catch instead of expected exceptions.