assert_that does not issue warning when actual is not a bool

[rittneje]

PyHamcrest/src/hamcrest/core/assert_that.py

Lines 60 to 62 in 405b753

	if isinstance(actual, Matcher):
	warnings.warn("arg1 should be boolean, but was {}".format(type(actual)))
	_assert_bool(assertion=cast(bool, actual), reason=cast(str, matcher))

It would stand to reason that that should be if not isinstance(actual, bool).

[brunns]

Any truthy or falsy value will do here - we don't need an actual boolean.

[rittneje]

We had several developers write assert_that(A, B), thinking it did an equality check, when in fact it always passes. Also, the existing warning message and documentation make it clear it is supposed to be a bool specifically.

[brunns]

It wouldn't be very pythonic to require a boolean type, and it would break a lot of existing tests. If we were going to do anything so drastic, I'd be more inclined to deprecate and eventually remove the non-boolean signature of assert_that() altogether.

[rittneje]

How is issuing a warning a breaking change?

[brunns]

OK, maybe not break as such, but there will be loads of warnings issued for (currently) perfectly valid tests.

[rittneje]

Do you know that such occurrences are actually correct, and not people misunderstanding what assert_that does, and thus their tests have been silently broken the entire time? It would seem to me that a testing library ought to err on the side of caution in such matters, since false positives (i.e., tests that should fail but pass) are extremely problematic.

[brunns]

Some of these tests may be broken, but most of them will not be. We should only be issuing warnings where things need fixing, not where they might need fixing. People need to be able to silence all warnings by fixing their code.

As I say, I think the entire non-matcher version of the function is broken, and should be deprecated then removed.

@offbyone - thoughts?

[rittneje]

I actually don't agree that the non-matcher version is intrinsically broken. For example, imagine I have a function is_valid that returns a bool. assert_that(is_valid(foo)) seems much clearer than having to deal with a matcher.

However, I cannot think of a case in which one would want to know that a non-bool was truthy but not check its actual value, which is the only case negatively impacted by this warning. At worst, users could either cast to bool themselves, or use some sort of isTruthy matcher, or this library could offer some means of disabling the warning if it is particularly irksome.

[swails]

Every Python unit test framework I've used before that has provided comparison functions (nose and unittest) has used the pattern comparator(actual, expected). assert_that seems absolutely crazy in its silence. If matcher is not a Matcher instance, it's simply ignored altogether.

I would be stunned if there was a single instance where someone wrote a statement like assert_that(arg1, arg2) knowing and expecting that arg2 was completely ignored, unless it was a matcher. This is especially true when someone who is used to writing Python unit tests in a different framework has to work on a project that uses hamcrest.

As written, hamcrest.assert_that makes it incredibly easy to write broken tests. I personally think that assert_that should throw an exception if matcher is not None, but is also not a Matcher instance, rather than simply ignore it altogether. But even if you don't want to go that drastic, a visible warning (i.e., not a DeprecationWarning) seems to me to be the least that could be done.

[swails]

It wouldn't be very pythonic to require a boolean type,

It's also not pythonic to do isinstance checks when duck typing is more aligned with the "Zen of Python". I'd argue that

def assert_that(actual, matcher=None, reason=""):
    if isinstance(matcher, Matcher):
        _assert_match(actual=actual, matcher=matcher, reason=reason)
    else:
        if isinstance(actual, Matcher):
            warnings.warn("arg1 should be boolean, but was {}".format(type(actual)))
        _assert_bool(assertion=cast(bool, actual), reason=cast(str, matcher))

is less pythonic (and less safe) than

def assert_that(actual, matcher=None, reason=""):
    if matcher is not None:
        _assert_match(actual=actual, matcher=matcher, reason=reason)
    else:
        if isinstance(actual, Matcher):
            warnings.warn("arg1 should be boolean, but was {}".format(type(actual)))
        _assert_bool(assertion=cast(bool, actual), reason=cast(str, matcher))

[swails]

Ah, you let matcher be reason when not an instance of Matcher.

I think this function could be made substantially safer without adding a ton of complexity. I'll post a possible implementation when I have more time to think about it

[brunns]

It's also not pythonic to do isinstance checks when duck typing

Oh, I couldn't agree more. The whole overloading, one function with multiple signatures thing is left over from when PyHamcrest was basically a transliteration of the Java version. It's nasty, but to get rid of it would break many people's tests, so I suppose we are stuck with it.

I never make use of the non-matcher version myself. If I'm not using a matcher, I don't see the advantage over an assert statement, especially when using pytest.

[brunns]

If only singledispatch allowed dispatching on the 2nd argument, that would be much nicer.

[swails]

If only singledispatch allowed dispatching on the 2nd argument, that would be much nicer.

Well you can, kind of:

def assert_that(arg, matcher=None, reason=None):
    return _assert_that(matcher, arg, reason)

@singledispatch
def _assert_that(matcher, arg, reason):
    # do stuff

But that's cool -- I didn't know about singledispatch before, thanks!

[rittneje]

I don't see how singledispatch resolves the problem though. The two signatures are assert_that(object, Matcher) and assert_that(bool, string). If you pick an implementation based solely on the second argument, then it means that if someone calls assert_that(str1, str2), it will still do the wrong thing (just check the truthiness of str1 instead of doing an equality check).

I agree that the second form of assert_that is of minimal utility over directly using assert.

[swails]

It lets you break up the logic and write cleaner functions for the individual situations. In my opinion, there are 5 different scenarios that should be handled independently:

1. arg is anything and matcher is a Matcher
2. arg is a bool and matcher is a string
3. arg is a string and matcher is a string
4. arg is neither a bool or a string and matcher is a string
5. arg is neither a bool or a string and matcher is neither a bool or a string

The first is unambiguous. The second is pretty clear to me, too, where matcher should be interpreted as the reason. The third is unclear and deserving of a warning. I imagine that there are just as many use cases where people assume that the function is comparing the two strings as there are where people are trying to evaluate the truthyness of a string and supplying a reason if it fails. The fourth strikes me as a situation where the programmer was relying on arg to be something truthy and providing a reason if the test failed. More details on this below. The last seems pretty clear to me that the majority of users intended them to be compared to each other and should result in a thrown exception.

singledispatch would allow us to split most of those scenarios into separate functions. It doesn't intrinsically solve the problem, it just makes the solution a little clearer/more elegant.

More details on the fourth scenario - I would not expect many people to really understand how binary logic operators work in Python:

>>> 0 or []
[]
>>> 0 and []
0
>>> 0 or object()
<object object at 0x7f02c3874dd0>
>>> 0 and object()
0

or and and don't return either boolean type -- it returns the last value encountered while short-circuiting the logic statement. That's why you'll see some people write (lazy) code like this:

def some_list_operation(arg=None):
    arg = arg or []

You don't want to assign mutable arguments as defaults because of early binding in Python (hence why you should never see def some_list_operation(arg=[]):).

So basically, if the first argument is not a string (say it's a list), it may be that way because someone wrote something like:

assert_that(list1 or list2, 'At least one of the lists should have something inside!')

So if you do a type-check on the first argument and warn/throw if the type is not boolean, this test will warn/throw because the first argument is actually a list, which would confuse the dickens out of plenty of people. In my experience, generic "pythonic-style" advice is to rely on variable truthyness rather than explicit boolean casts. By default, pylint actually complains when you do something like if len(items) == 0: and requests that you spell it like if not items:

[rittneje]

arg is neither a bool or a string and matcher is neither a bool or a string

I assume you mean "matcher is neither a matcher nor a string". However, there is another case to be considered - matcher is None. This could happen either because the caller didn't provide one - assert_that(some_var) - or because the caller explicitly provided None - assert_that(some_var, None). In the first case, presumably they are trying to assert the truthiness of some_var without an explicit reason string. But the second case could be an attempted equality check. Unfortunately, these two cases have complete opposite expectations.

[swails]

However, there is another case to be considered - matcher is None.

Yea, I'd thought about that. I did just now come up with a workaround:

_singleton = object()

def assert_that(arg1, matcher=_singleton, reason=''):
    if matcher is None:
        warnings.warn('The matcher argument should be a Matcher instance, like is(None)')
    ...
    if matcher is _singleton: # matcher wasn't specified
        ...

The reason I didn't include that in my list of cases to handle was because I had thought it was going to require pretty complex processing of all *args and **kwargs combinations of previously-legal incantations. The above approach would make handling an explicit None pretty straightforward.