Provide better mismatch descriptions for any_of and only_contains

[zsoldosp]

What problem does this merge request solve?

running

from hamcrest import *

if __name__ == '__main__':
    assert_that(['a'], only_contains(has_length(3)))

in master reports

Traceback (most recent call last):
...
AssertionError:
Expected: a sequence containing items matching (an object with length of <3>)
     but: was <['a']>

after the merge request, the error message becomes

Traceback (most recent call last):
...
AssertionError:
Expected: a sequence containing items matching (an object with length of <3>)
     but: was <['a']>, first sequence item not matching was 'a' with length of <1>

However, this merge request is not perfect.

Known (Guessed) Problems

• if sequence is a stream generator that can only be iterated once, then describe_mismatch will fail trying to iterate it again.
  I believe it would be safe to encapsulate list(sequence) as an instance variable (or hide behind a memoized property) where it could be used from both methods, but the tutorial/docs are pushing in the direction of stateless matcher preferences. Thus not being familiar enough with the architecture and usages of hamcrest, I didn't want to do that initially

Potentatial Improvements

• prefix/postfix could be handled as TestCase class variables (like a "Template Method")
• any_of had the same problem (8f0bcce) as only_contains (5a6c990) - not having a proper delegation to the actual matchers to provide the mismatch description. I suspect there can be other (composite)
  matchers that have the same issue.
• now issequence_onlycontaining_test is coupled to the has_length implementation. Probably there is (should be) a way to have a matcher with test-controllable describe_foo values, so there is no need for coupling. Then the initial assertions from testDescribeMismatchDisplayUsedMatchersDescriptionForFirstFailingItem could be removed
• there is too much duplication between describe_mismatch and _matches. However, to resolve that by introducing a get_first_mismatch method would need to be an object (can't use None due to the need to distinguish between no mismatches and TypeError return type cases), and I didn't want to invest in that until I knew that change was welcome

[landscape-bot]

Repository health increased by 0.27% when pulling bf48141 on zsoldosp:propagate-original-matcher-failure-message-to-only_contains into edcf3f4 on hamcrest:master.

• No new problems were introduced.
• 1 problem was fixed (including 1 error and 0 code smells).

[zsoldosp]

I would be happy to address to issues marked as problems/improvements, but I would like to get feedback whether the suggestions outlined are OK. Thanks!

[offbyone]

@zsoldosp Sorry about the long delay on this.

Yeah, matchers are intended to be stateless in the current design. That's not to say we couldn't revisit that, but certainly right now adding persistent state is not ideal. While state is useful in test cases, matchers are not a purely test-oriented concept and retaining stateless matching is useful in a lot of cases.

So in order to merge this, it must support streaming iterables as well as non-streaming.

[svetlyak40wt]

@zsoldosp Peter, what is matches method will return all state necessary for problem description? Right now it return bool value, but it could return an object which can be casted to boolean when needed but also will carry all information, necessary for displaying description.

What do you think?

[zsoldosp]

@svetlyak40wt that sounds good to me. However, how do you "cast" something to bool?

• direct inheritance doesn't work

  >>> class Foo(bool):
  ...     pass
  ...
  Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  TypeError: type 'bool' is not an acceptable base type
  

• returning any class object will satisfy existing if matcher.matches(...) checks

  >>> class NoMatch(object):
  ...      pass
  ...
  >>> if NoMatch():
  ...    print("match!")
  ...
  match!
  

• is backwards compatibility important? Thinking of people's existing custom matchers, but there might be other scenarios I'm not aware of ....

[offbyone]

Backwards compatibility is important in the 1.x stream; I'm willing to revisit it for the 2.x

To make something boolean-ish, an object has to implement __nonzero__.

The barrier here, though, is pretty high. You're proposing revamping the interface of all matchers, including those written by people who have used Hamcrest to implement custom ones. I don't know that the value gained by such a change is enough to justify the amount of incompatibility woes involved.

[zsoldosp]

@offbyone quick note re: backwards compatibility - while not-so-pythonic, the hamcrest core can be changed in such a way to support both return types (isinstance, hasattr, etc.). It would allow for gradual transitioning and be compatible with existing custom matchers (and possibly even the core ones)

[svetlyak40wt]

I agree with @zsoldosp we just need to play a little with such soltuion, rewrite one core matcher and see if there will be so problematic to use it in mix with old style matchers.

[offbyone]

You're not going to be able to see the full scope of the uses you'll get. The issue is that there's nothing out there that wraps the matcher. You can have assert_that do something nice, sure, but other code will have to be either built to support this "DescribedBoo" or not, and in one direction there will be a lot of repeated testing code to check it.

Remember that the hamcrest core doesn't actually consume the output of the match function, it only emits it. The assert_that helper function isn't part of the core, nor is it the only way to use matchers (I use them in code that has nothing to do with testing, for example).

[zsoldosp]

after digging into the code more, I'm confused

• it seems that the "recommended" way to deal with this situation is to directly override matches instead of _matches as per BaseMatcher

  Most implementations can just implement :py:obj:_matches, leaving the
  handling of any mismatch description to the matches method. But if it
  makes more sense to generate the mismatch description during the matching,
  override :py:meth:~hamcrest.core.matcher.Matcher.matches instead.

• that is used by IsSequenceContainingInAnyOrder, where essentially matches gets called twice, once for checking for a match and once for generating the mismatch_description

• However, that implementation also fails for generator sequences. IsGeneratorSequenceContainingInAnyOrderTest.testDescribeMismatchAfterMatch has a bug - it actually uses two different sequence objects instead of using the same one. The correct test would look like the following

    def testDescribeMismatchAfterMatch(self):
        matcher = contains_inanyorder(1, 2, 3)
        sequence = self._sequence(3, 1)
        matcher.matches(sequence)
        self.assert_describe_mismatch('no item matches: <2> in [<3>, <1>]',
                                      matcher, sequence)
  

  which fails with the current implementation (master)

  E       AssertionError: assert 'no item matc...in [<3>, <1>]' == 'no item match...2>, <3> in []'
  E         - no item matches: <2> in [<3>, <1>]
  E         + no item matches: <1>, <2>, <3> in []
  

[offbyone]

This has sat long enough I think it probably needs a revisit post-2.0