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

Conversation

swails
Copy link

@swails swails commented Aug 25, 2020

Alternative for #148 and #147 that more specifically targets usage patterns that are highly likely to be mistakes.

# 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.

warnings.warn("arg1 should be boolean, but was {}".format(type(actual)))
# Or if actual and matcher are both truthy strings
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>)

if isinstance(actual, str) and isinstance(matcher, str) and actual:
warnings.warn("assert_that(<str>, <str>) only evaluates whether the "
"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.

# 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.

@@ -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.

Base automatically changed from master to main March 6, 2021 08:00
@offbyone
Copy link
Member

offbyone commented Mar 7, 2021

The CI setup has been significantly revamped since this was created (and I have not had time to think through it in the interim). Please rebase this PR and let's see where it shakes out.

I'm cautiously okay with the basic idea of warning in the limited case where there is a single-arg call to assert_that that tests truth against a matcher. Anything stricter is not something I want to entertain at this time. That warning should also be trivially suppressable using standard warning suppression tools.

@rittneje
Copy link

I'm cautiously okay with the basic idea of warning in the limited case where there is a single-arg call to assert_that that tests truth against a matcher.

@offbyone I don't understand what you mean by this. Single arg calls are rare (in my experience) and aren't the problem. The root problem is that our developers (and reviewers) continue to erroneously expect assert_that(a, b) to do an equality check, whereas in reality it has the unintended behavior of merely asserting the truthiness of a, and thereby causes a faulty test to pass. I still contend that the simplest and best way of resolving this issue is to unconditionally emit a warning when the second parameter (i.e., matcher) is not a Matcher. Anyone who truly just wants a truthiness check should use a named parameter to make it unambiguous - assert_that(a, reason="..."). I don't believe that any other solution (including more clever or "Pythonic" warning conditions) can truly make this library safe for use, without introducing a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants