Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for improved safety of assert_that #149

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 37 additions & 7 deletions src/hamcrest/core/assert_that.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
__tracebackhide__ = True

T = TypeVar("T")
_unassigned = object()


@overload
Expand All @@ -25,7 +26,7 @@ def assert_that(assertion: bool, reason: str = "") -> None:
...


def assert_that(actual, matcher=None, reason=""):
def assert_that(actual, matcher=_unassigned, reason=""):
Copy link

@rittneje rittneje Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this perhaps be simplified? (Forgive my somewhat pseudocode.)

# second parameter was a Matcher
if isinstance(matcher, Matcher):
    return _assert_match(actual, matcher, reason)

# first parameter was a Matcher, the caller maybe passed them in the wrong order? (existing warning)
if isinstance(actual, Matcher):
    warnings.warn('...')

# second parameter was not provided, third parameter may have been provided
if matcher is _unassigned:
    return _assert_bool(actual, reason)

if reason:
    warnings.warn('reason string will be ignored...')

# If second parameter was not a string, the caller likely thought they were doing an equality check.
# If both parameters are strings, it is ambiguous whether the caller thought they were doing an equality check,
# or a truthiness check with a reason string. Err on the side of caution and log a warning.
# If a truthiness check was desired, they can resolve the warning by passing the reason as a named parameter.
if not isinstance(matcher, str) or isinstance(actual, str):
    warnings.warn('possible misuse...')

return _assert_bool(actual, matcher)

I think we should avoid introducing a TypeError at this moment, because that is a breaking change. It would be nicer to have an intermediate release that warns about suspicious usages, allowing people to fix these issues without having to fix everything at once.

"""Asserts that actual value satisfies matcher. (Can also assert plain
boolean condition.)

Expand All @@ -52,14 +53,43 @@ def assert_that(actual, matcher=None, reason=""):
This is equivalent to the :py:meth:`~unittest.TestCase.assertTrue` method
of :py:class:`unittest.TestCase`, but offers greater flexibility in test
writing by being a standalone function.

"""
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))
return _assert_match(actual=actual, matcher=matcher, reason=reason)

# Warn if we got a Matcher first
if isinstance(actual, Matcher):
warnings.warn("arg1 should be boolean, but was {}".format(type(actual)))
# Or if actual and matcher are both truthy strings

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You aren't checking if matcher is truthy.

if isinstance(actual, str) and isinstance(matcher, str) and actual:
warnings.warn("assert_that(<str>, <str>) only evaluates whether the "

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could suggest resolving the ambiguity by passing the reason as a named parameter, in the event that they truly intended to do a truthiness assertion? assert_that(<str>, reason=<str>)

"first argument is True, so the second string is always "
"ignored. Make sure to use an explicit matcher for the "

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't always ignored. It is used as the reason string in the event that the first string is falsey.

"second term")
# If matcher is not a string (and so not a 'reason'), and is the same
# type as the actual, it's almost certain that assert_that was invoked
# with the intention that actual == matcher be evaluated. Since this will
# always appear to "pass" if "actual" itself evaluates to boolean True
# (which happens far more often than not), make this a hard error. This
# situation will almost always (if not always) be a buggy test.
if not isinstance(matcher, str) and type(matcher) is type(actual):
raise TypeError('matcher should be a Matcher instance, or simply '
'provide a reason if you wish to evaluate the actual '
'value as a boolean.')
# It really only makes sense for matcher to be a Matcher instance or a
# string (actually the 'reason'). Anything besides that is more likely to
# be a mistake than anything else, so properly warn.
if not isinstance(matcher, str):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't warn here if matcher is _unassigned, since it means they are just asserting the truthiness of actual.

warnings.warn('matcher was not an explicit Matcher instance, so the '
'assert_that check will pass if the first argument '
'simply evaluates to boolean True. This is unusual.')
# If someone provided an explicit 'reason', and passed a matcher that was
# not a Matcher instance, this is almost certainly a mistake.
if reason and matcher is not _unassigned:
raise TypeError('matcher should be a Matcher instance, not {}'.format(
type(matcher)))

return _assert_bool(assertion=cast(bool, actual), reason=cast(str, matcher))


def _assert_match(actual: T, matcher: Matcher[T], reason: str) -> None:
Expand Down
6 changes: 3 additions & 3 deletions src/hamcrest/core/string_description.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ class StringDescription(BaseDescription):
"""

def __init__(self) -> None:
self.out = ""
self.out = []

def __str__(self) -> str:
"""Returns the description."""
return self.out
return ''.join(self.out)

def append(self, string: str) -> None:
self.out += str(string)
self.out.append(str(string))