hamcrest
TypeSafeDiagnosingMatcher calls matchesSafely unnecessarily twice
When a matcher extending TypeSafeDiagnosingMatcher fails the method matchesSafely is executed twice. This is bad especially when the evaluation in matchesSafely is expensive. The second problem is that the item state could change between executing matches() and describeMismatch(). Especially when executing Selenium tests this could happen. In this case the mismatch description would not reflect the actual cause why matches() failed.
I found #144, but since this problem is repairable (we use an own implementation of TypeSafeDiagnosingMatcher that does not have this problem), I could make a PR, if desired.
This is an interesting use case. Our intention was that matching should be direct enough that the cost of re-evaluation should be negligible, and the current implementation has the benefit of simplicity from not retaining state. Could you post an example of the sort of matchers you're writing?
An example could be a matcher for Selenium WebElements that matches if a WebElement has a specific text. matchesSafely would look like this
protected boolean matchesSafely(WebElement item, Description mismatchDescription) {
String text = item.getText();
if (!text.equals(expectedText)) {
mismatchDescription.appendText("was " + text);
return false;
}
return true;
}
So when matchesSafely is called twice (for checking and for creating the mismatch description), item.getText() is also called twice. If the element's text changes between these two calls (due to some Javascript or whatever), the mismatch description does not reflect the state that led to the mismatch. This is a simplified example, but these problems do happen with the current implementation.
I'm not sure what to do about this. I think the "right" answer might be to separate the timing behaviour from the raw matching, we wrote about this in GOOS with the concept of "Probes".
I've been thinking about this again. It's true that we call matchesSafely twice (that actually happens in assertThat() but the problem you describe here is really one of waiting for stability. I think this could be better handled by something like #202. In your case, it might need a "never" matcher. Otherwise, you're at risk of creating time-sensitive tests without making that explicit. Thoughts?
Why not avoid calling matchesSafely twice like this?
public abstract class TypeSafeDiagnosingMatcher<T> extends TypeSafeMatcher<T> {
private Description mismatch;
@Override
protected final void describeMismatchSafely(T item, Description mismatchDescription) {
if (mismatch == null) {
super.describeMismatchSafely(item, mismatchDescription);
} else {
mismatchDescription.appendText(mismatch.toString());
}
}
@Override
protected final boolean matchesSafely(T item) {
mismatch = new StringDescription();
return matchesSafely(item, mismatch);
}
protected abstract boolean matchesSafely(T item, Description mismatchDescription);
}
That's possible but has a couple of problems. First, it breaks the contract that matchers are stateless, which has been a fundamental assumption so far.
Second, I think this is highlighting a problem in your test design, namely that there's a timing element but that it's not explicit. It looks like you want something more like a "probe" which will determine what to observe as values change over time.
It's not a problem in the test design. It's a problem that is quite normal to Selenium tests. And I don't want to observe anything changing over time - I don't want these "flickering" elements. It's OK that they cause a test failure, even if only occasionally. You can react to this, make it more stable, wait longer for the element to be ready, whatever. But what I don't want is a matcher that announces such a failure by possibly using two different states of the tested element for 1) failing and 2) generating the failure description.
What is so bad about a non-stateless matcher in practice? Instead of replacing the present TypeSafeDiagnosingMatcher you could even add it as a separate class and call it maybe StatefulDiagnosingMatcher or something like that to make that clear.
I am using this implementation anyway, because I don't care about stateful or stateless matchers. But I do care about correct failure descriptions. I was just asking for making it part of the Hamcrest code.
I see that this behavior has come up twice in issues, but each time it was dismissed because the discussion was not purely about the computational cost of the matcher.
I have a case where the only issue with the redundant call is the computational cost. My custom matcher is searching an arbitrary list of polygons for any overlapping pairs.
I'm hesitant to open a new issue now to request that redundant matcher calls be avoided, since apparently it was a by-design compromise. But it's certainly a disappointing limitation of custom Matchers. I hope that perhaps someday a refactor could be done to improve this case. (In general, for a widely-used library, I cringe at the "but users will never do that" argument for technically inferior solutions.)
