Deprecating CriteriaHelper’s Criteria Proposal

[Ted Choc]

(Explicitly adding folk that have reviewed some of my prior patches to remove usage of Criteria)

Goal
To provide a more concise way of validating the expected conditions while at the same time defaulting to providing more useful error logging. Useful error logging should be the default instead of requiring each dev to understand how to work around the quirks of Criteria.

Proposal - Remove extending Criteria and introduce CriteriaNotSatisfiedException

Instead of clients extending Criteria directly, convert clients to pass in Runnables that will be tried until they pass without Exception being thrown. We will introduce a new Exception type that if thrown indicates that the Criteria can be retried without concern of the system being in an invalid state. By introducing a new Exception type, we avoid conflating an existing Exception that other parts of the application expect to result in immediate termination (and thus might have left it in a state where subsequent retries could not be trusted).

Design

Criteria is converted to a set of static utilities that simply throw the desired Exception if a condition is not met. You can no longer extend it. Criteria.equals is removed because it would be too easy to mistakenly use (as it would return a Runnable to be passed into CriteriaHelper).

public final class Criteria {

    public static <T> void checkThat(T actual, Matcher<T> matcher) {

        if (matcher.matches(actual)) return;

        Description description = new StringDescription();

        description.appendText("Expected: ")

                .appendDescriptionOf(matcher)

                .appendText(System.lineSeparator())

                .appendText("     but: ");

        matcher.describeMismatch(actual, description);

        throw new CriteriaNotSatisfiedException(description.toString());

    }

    ... potentially other helper methods ...

}

Example code - Old Way:

CriteriaHelper.pollInstrumentationThread(new Criteria() {

    @Override

    public boolean isSatisfied() {

        if (doesUrlBarHaveFocus(urlBar) != active) {

            updateFailureReason(

"URL Bar did not have expected focus: " + active);

            return false;

        }

        updateFailureReason(

                "The keyboard did not reach the expected active state: "

+ active);

        return isKeyboardActiveForView(urlBar) == active;

    }

});

Example code - Proposed New Way:

CriteriaHelper.pollInstrumentationThread(() -> {

    Criteria.checkThat(“URL Bar did not have expected focus”, 

            doesUrlBarHaveFocus(urlBar), Matchers.is(active));

    Criteria.checkThat(

"The keyboard did not reach the expected active state", 

            isKeyboardActiveForView(urlBar), Matchers.is(active));

});

For the full details, check out this doc that describes some alternatives explored.

Let me know if you have any concerns or comment on the doc.

Other open question:

• Should we add additional utility methods like checkEquals to the Criteria API or should everything rely on checkThat and Matchers? Should we avoid adding APIs that can accomplish the same thing but only aim to save some characters of typing + potentially importing Matchers?
  
  e.g. these two would be the same:
  Criteria.checkEquals(expected, actual);
  Criteria.checkThat(actual, Matchers.is(expected));

Thank you,

Ted

[Bo Liu]

So the major difference from the existing refactoring is using CriteriaNotSatisfiedException rather than AssertionError to indicate that the criteria is not satisfied. I guess it's obvious in hindsight that there are other things that could raise AssertionError as well. Sounds ok to me.

[Wei-Yin Chen (陳威尹)]

This proposal sounds good to me!

Similar to the open question, do we keep the Callable<Boolean> variation for ease of use? I guess we only need to slightly revise toAssertionRunnable() to fit the new exception.

[Ted Choc]

On Fri, May 29, 2020 at 11:21 AM Wei-Yin Chen (陳威尹) <wyc...@chromium.org> wrote:

This proposal sounds good to me!

Similar to the open question, do we keep the Callable<Boolean> variation for ease of use? I guess we only need to slightly revise toAssertionRunnable() to fit the new exception.

I'm a bit conflicted. For very simple boolean checks, I think it is OK. But we need to convince folk to never use it for things like:

if (conditionX) return false;

else if (conditionY) return false;

else if (conditionZ) return false;

return true;

You lose the line number of the failed condition in this case, and it will make failed tests much harder to reason about. If we see it constantly misused, then I could see an argument for pruning the Callable variant.

 

On Fri, May 29, 2020 at 8:59 AM Bo Liu <bo...@chromium.org> wrote:

So the major difference from the existing refactoring is using CriteriaNotSatisfiedException rather than AssertionError to indicate that the criteria is not satisfied.

Yep, that is correct.

[Tommy Nyquist]

Overall this update makes sense to me.

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAFs4gPq_4uyaUUFtMe5Ak5NpzJOx9RxSc2VcZVq0nzVgo_vOVw%40mail.gmail.com.