Deprecate + Remove 2-arg form of T.assert_true

[asottile]

I've seen code like this in three separate code reviews over the last two weeks:

T.assert_true(
    some_var,
    some_other_var,
)

From the cases I've seen, the programmer actually wanted T.assert_equal(...) instead. But turned their test into a noop (aside: a good test fails when you write it and then you make it succeed so admittedly there's a test-theory-education-problem component to this)

Which does something along the lines of:

assert some_var, some_other_var

My proposal (breaks API):

Existing code

def assert_true(lval, message=None):
    """Assert that lval evaluates to True, not identity."""
    if message:
        assert bool(lval) == True, message
    else:
        assert bool(lval) == True, \
            "assertion failed: l == r\nl: %r\nr: %r\n\n%s" % \
                (lval, True, _diff_message(lval, True))

New code:

def assert_true(lval, *args, **kwargs):
    """Assert that lval evaluates to True, not identity."""
    # Complain about probably-bad code
    if args:
        raise AssertionError('`message` is kwargs only.  Perhaps you actually meant `assert_equal`?')
    # Force message to be kwarg-only
    message = kwargs.pop('message', None)
    # equality with singletons is a weird choice, I assume this to be compatibility code with unittest.TestCase.assertTrue
    if message:
        assert bool(lval) == True, message
    else:
        assert bool(lval) == True, \
            "assertion failed: l == r\nl: %r\nr: %r\n\n%s" % \
                (lval, True, _diff_message(lval, True))

The same applies for assert_false.

Tagging @wting who added these (why?)

[bukzor]

+1

[asottile]

@wting also what does "not identity" mean?

[wting]

Hmm, from what I understand people are using assert_true() on multiple variables but instead the second one is being assigned to message and thus silently passing? This sounds like a good change to me.

Identity refers to truthiness vs is True. In retrospect these should have been named assert_truthy() and assert_falsy() and assert_true() would compare is True.

If you're making a backwards incompatible change, is it OK if we change the function names to communicate truthiness better?

[asottile]

Via #293