Improve describeMismatch output across the board.

[louisth]

After running into confusing output from describeMismatch and reading the discussion at hamcrest/JavaHamcrest#9, I have tackled the problem head on. I believe @sf105 and @ihaworth have the right idea. Instead of trying to describe the failure, we need to describe the given item from the Matcher's point of view. According to this contract for describeMismatch, the existing implementations are basically correct.

What I've done is three things.

1. I extended and revised the Javadoc to specify this new contract for describeMismatch:

    /**
     * Generates a description of the given item from the Matcher's point of view.
     * The description will be part of a larger description of why a matching
     * failed, so it should be concise.
     * The description should be able to replace Y in the sentence "Expected X but Y," and
     * should be in the past tense, for example, "Expected null but <U>was &lt;"foo"&gt;</U>."
     * The description should NOT describe the matcher, but rather should highlight features of interest on the item as they actually are.
     * The description must ALWAYS be filled in, regardless of return value of {@link #matches}. 
      */

(DescribeMismatch is now rather misnamed, but I leave it to the maintainers to decide whether they want to break backwards compatibility to fix this.)

1. I wrote DescribeMismatchTortureTest, which calls describeMismatch on EVERY matcher in Hamcrest. I used not() with every matcher. I used a variety of matcher compositions including anyOf() and allOf(), and sub-matchers with a variety of descriptions, such as equalTo(), instanceOf(), and startsWith(). I matched against a variety of target objects. I'm sure there are interesting cases I missed but it's a start. What's nice is the torture test allows us to see if a description is phrased right to fit in a variety of contexts. It also allows us to check for reasonable consistency across all the matchers.
2. I fixed every broken description and mismatch description until every test in the torture test produced a reasonable result. Improvements are welcome, but I think this is a good start.

For anyone disagrees with my solution and wants to try their own hand at tackling this problem, I encourage you to at least grab the torture test. I think it does a pretty good job of illustrating and exercising the complexity of the problem.

[scarytom]

As much as I applaud your good work, this change-set is rather large, and hence difficult to digest.

Also, there appears to be a number of unrelated changes mixed in (for instance several classes like NullDescription and CombinableBothMatcher have had their final modifier removed), and this makes it hard to review your changes in isolation.

Is there any way you could break your work into smaller, incremental changes? I realise that your torture test probably mandates many of these changes in order to run, but perhaps we could add the individual matcher description changes first, in smaller batches, and finish up with the passing torture test?

Suggestions welcome.

[louisth]

I'll work on refactoring the change sets.

[sf105]

@louisth before you put all the work in, we should take a look as this is quite a big change, especially if you (rightly or wrongly) want to redefine mismatchDescription

[louisth]

OK. Now I'm confused. I admit, there are not the cleanest change sets and I should give more explaination. I also admit I'd rather not spend hours juggling change sets, especially if it's all going to be rejected anyway. What do you guys want?

[dharkness]

What's the quickest path to seeing the list of mismatch messages, especially in comparison to the old messages? If people can see the end result easily and like it, they'll be much more inclined to help move this forward.

[louisth]

@dharkness: That makes sense. I've made a commit here (louisth/JavaHamcrest@93f2e822e77ffe423a3e021abc3c2b3340bc1c8d) that shows the difference between the current and the proposed. You can use your diff tool of choice to make the comparison. (GitHub is OK, but in this case every line needs a line-by-line comparison and GitHub's block comparisons don't make the changes easy to see.)

This was an interesting exercise. While I picked examples to exercise all the code, I'm not sure I picked examples that demonstrate the potential for improvement, since with simple objects like Strings and Integers it's pretty easy to see why a match failed no matter how they're described. Oh well.

A lot of the changes I made were to make the Matchers produce inteligible descriptions when the match succeeded. Since not() currently describes the passed object itself (rather than delegating to the wrapped Matcher for a more context sensitive description), the current "not" messages are usually acceptible. Once we delegate to the matchers, a lot of breakage occurs that does not show up here.

Some of my favorite results:

Expected (an instance of java.lang.String and "bad") but an instance of java.lang.String <5> is a java.lang.Integer. becomes Expected (an instance of java.lang.String and "bad") but was a java.lang.Integer.

Expected hasProperty("good") but no "good" in <5>. becomes Expected has property "good" but didn't have property "good".

Expected not a collection containing "good" but was <[ugly, bad, good, other]>. becomes Expected not a collection containing "good" but item <2> was "good".

Expected not same property values as String [bytes: [<103>, <111>, <111>, <100>], empty: <false>] but was "good". becomes Expected not same property values as String [bytes: [<103>, <111>, <111>, <100>], empty: <false>] but bytes was [<103>, <111>, <111>, <100>] and empty was <false>.

Expected an array with size a value greater than <2> but array size <2> was equal to <2>. becomes Expected an array with size greater than <2> but array size was equal to <2>.

[derari]

I really like what you did there, but I would change the description should be able to replace Y in the sentence "Expected X but Y," to "the description should say whatever the factory method said".

The problem of the current version (without your changes) is that every matcher assumes it is the only one, which leads to verbose messages when you combine them. I think it is perfectly fine if some matcher does not fully fill the "Y" of the template sentence. If every matcher only added to the description what they say in the source code, readable code automatically guarantees good descriptions.

The other problem is the sentence structure "Expected X but ...". Take, for instance assertThat(3, is(lessThan(1)); assertThat(myList, size(is(lessThan(5))); which gives you either Expected a value less than <3> and Expected a collection with size a value less than <3>, or Expected less than <3> and Expected a collection with size less than <3>. Both variants can get it only right for one message, because the lessThan matcher is used in different grammatical contexts.

If the template was changed to "Expected that value X but ..." (again, closer to what the code says), and every matcher sticks to their factory method, you would get Expected that value is less than <1> and Expected that value size is less than <5> (Notice that in this scenario the "is"s were added by the is-matcher). Replace "value" with something from the getParameterType method that you added, and you will get wonderful messages.

[nhojpatrick]

@louisth fancy rebasing this from master, as hamcrest-core and hamcrest-library have been refactored a lot and also deprecated, so that everything is just in hamcrest.

[sf105]

Personally, I think this one will cause unexpected problems. The reason we kept it slightly clunky is that getting rid of the clumsiness takes a lot of code and is never quite right. The uses cases here are straightfoward, but it gets messy very quickly. Then you're left with a lot more complicated code and still the same problem. I strongly recommend not taking this one, or at least simplifying it.

[nhojpatrick]

Going to try and kick start hamcrest, so if you want to get it merged, please rebase from the branch v2.3-candidates. Still trying to understand how has permissions to perform a release.