Pytest: give hints when an assertion value is None instead of a boolean

[RonnyPfannschmidt]

sometimes beginners invoke code like assert something_that_asserts_and_returns_none and end up puzzled,

for reference see https://stackoverflow.com/questions/48152560/pytest-assert-has-calls-usage/48206730?noredirect=1#comment83395353_48206730

i believe it would be a nice and simple addition to give a hint in the case that a assertion value was None and this might be a misstake,

for clarity None should be tested with x is None or x is not None

[jrbenny35]

I would like to help with this, but would need some mentorship. Mainly with navigating the code base heh.

[nicoddemus]

Hi @jrbenny35, thanks for reaching out!

I don't know the intricacies of the assertion rewriting myself, but you can start digging around the _pytest/assertion directory.

For example currently a warning is issued when an assert is used with a tuple here.

While it is possible to know that an assert statement is receiving a tuple at parsing time, to realize if an arbitrary expression is returning None we need to check this at runtime.

I would start by adding a breakpoint in AssertionRewriter.visit_Assert while debugging a test like this:

def x(): return None

def test():
    assert x()

To try to figure out how pytest generates this message:

test_asserts.py:5 (test)
def test():
>       assert x()
E       assert None
E        +  where None = x()

Hope this helps! 👍

[jrbenny35]

@nicoddemus thanks for the tips.

I found this kinda weird print for the explanation that contains the error message.

%(py2)s
{%(py2)s = %(py0)s()
}

It's a list as well. Any tips?

[nicoddemus]

@jrbenny35 where did you find that code? Was that output from a debug or is that code?

I have to go to bed, but meanwhile you might want to read how assertion rewriting works behind the scene: http://pybites.blogspot.com.br/2011/07/behind-scenes-of-pytests-new-assertion.html

[jrbenny35]

It was found here.

This explanation variable is what printed the lines above.

[cheaterok]

Won't it be enough to test if an assert expression is a function\method and if it has return statement using AST? Just like it currently is implemented with tuple assertion.
This way we avoid any runtime checks and cover the cases where beginners tend to make mistakes.

[nicoddemus]

Hi @cheaterok ,

We can't know just by looking at the AST if an expression returns None:

assert f()

f() can return True, a list, etc, all cases in which we don't want to issue a warning.

This is why I think we need to add some check during runtime.

[nicoddemus]

@jrbenny35 thanks. That point in the code is where the AST translation is being made, basically changing:

assert expr

Into:

if not expr:
    # some code to explain why 'expr' failed

You can see that in the code below adding a unary operation not (ast.UnaryOp(ast.Not(), top_condition):

https://github.com/pytest-dev/pytest/blob/506c9c91c0127e66944c4e357f43fa2a46916fa7/_pytest/assertion/rewrite.py#L743-L754

One needs to look deeper into how the explanation is being generated.

Also I'm not sure if the end result will be good, I have the feeling we will be giving a lot of false positives which might be worse in the end. Well, just a thought.

[jrbenny35]

Looking over this again I came up with this.

If I do these lines when evaluating the explanation I can call this object ast.Raise(ast_Call(err_name, [fmt], []), None) and get the property cause to see if it is None.

If so, then I could change the explanation message, and then let pytest continue. It seems changing the explanation message works, but checking that message is tricky.

So I could put those lines mentioned above into a function and just return the value of cause.

[SharonNorah]

hello @RonnyPfannschmidt we would like to take on this issue. We are a team called PyBees from Pyladies Kampala. We are also inquiring if the issue is still open.

[RonnyPfannschmidt]

@SharonNorah thanks for reaching out, i am unaware of the current state of the issue/implementation - i beleive @nicoddemus and @jrbenny35 have a better handle on it

my current assumption is that @jrbenny35 has investigated quite a lot, but then unfortunately couldn't afford the time for a public POC on top

[nicoddemus]

I'm under the same impression as @RonnyPfannschmidt (but @jrbenny35 please correct me if I'm wrong). Otherwise @SharonNorah a PR would be most welcome!

[avirlrma]

@nicoddemus @RonnyPfannschmidt I understand the issue well and I think I can handle this one. Can I make a PR for this?

[nicoddemus]

@avirlrma sure please go ahead, not even need to ask! 👍

[SharonNorah]

It's fyn go ahead
On May 16, 2018 3:29 PM, "Bruno Oliveira" [e-mail] wrote:

@avirlrma https://github.com/avirlrma sure please go ahead! 👍

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/pytest-dev/pytest/issues/3191#issuecomment-389501061,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AdvwyJX2BpXVaqkRk_fh7h01offqf4ssks5tzBvHgaJpZM4R6w3x
.

[avirlrma]

Just to update,I'm going through pytests documentation(and examples) and the codebase.Expect a PR soon.

[avirlrma]

@nicoddemus @RonnyPfannschmidt ee! After numerous effort, I can't seem to crack this one. Can one of you please implement this so that I can learn? Thanks!