[Python-Dev] How far to go with user-friendliness

[Florian Bruhin]

* Ron Adam <ron3200 at gmail.com> [2015-07-19 18:06:22 -0400]:
> 
> 
> On 07/19/2015 02:33 PM, Florian Bruhin wrote:
> >* Ron Adam<ron3200 at gmail.com>  [2015-07-19 11:17:10 -0400]:
> >>>I had to look at the source to figure out what this thread was really all
> >>>about.
> 
> And it seems I don't quite get it still, but I am trying.

No worries - I'll try to clarify until things are clear :)

> >>>Basically it looks to me the purpose of adding "assret" is to add an "alias
> >>>check" for "unsafe" methods.  It doesn't actually add an "alias".  It allows
> >>>a developer to use a valid alias to avoid conflicting with methods starting
> >>>with assert that will still work with the mock module.
> >>>
> >>>The mock module says that when "unsafe" flag is set to True, it will not
> >>>raise AssertionError for methods beginning with "assert" and "assret".  It
> >>>doesn't specify what "unsafe" means, and why you would want to do that.
> >>>
> >>>So first do this...
> >>>
> >>>     * Document "unsafe" in mock module.
> 
> I still think documenting the purpose of "unsafe", rather than just the
> effect it has is important.
> 
> From the confusion in this thread, (including my own), it's clear the
> documentation does need some attention.
> 
> 
> There are only two places where it mentions "unsafe" in the docs...
> 
> The class signature...
> 
> """
> class unittest.mock.Mock(spec=None, side_effect=None, return_value=DEFAULT,
> wraps=None, name=None, spec_set=None, unsafe=False, **kwargs)
> """
> 
> 
> And in the section below that.
> 
> """
> unsafe: By default if any attribute starts with assert or assret will raise
> an AttributeError. Passing unsafe=True will allow access to these
> attributes.
> """
> 
> But that doesn't explain the purpose or why these are unsafe or why it
> should throw an AttributeError.   Or what to do with that AttributeError.

It's "unsafe" because tests which:

1) are using the assert_* methods of a mock, and
2) where the programmer did a typo (assert_called() instead of
   assert_called_with() for example)

do silently pass.

> >But if you do a typo, the test silently doesn't fail (because it's returning a
> >new mock instead of checking if it has been called):
> 
> Do you mean silently passes?

Yes - it passes without checking the thing the test was intended to
check.

> 
> >     >>> m.assert_called()
> >     <Mock name='mock.assert_called()' id='140277467876152'>
> >
> >With the patch, everything starting with "assert" or "assret" (e.g. my
> >example above) raises an AttributeError so these kind of issues can't
> >happen.
> 
> I get that part.  It's checking for a naming convention for some purpose.
> 
> What purpose?
> 
>    "To raise an AttributeError"  ... but why?

So you notice you have done a typo and your test will not actually
verify what you want it to verify.

Compare it with the behavior of a normal object - if you call a method
which doesn't exist, it raises AttributeError.

This isn't possible with Mock objects, as they are designed to support
*any* call, and you can check the calls have happened after the fact.

With the patch, if you call any method starting with "assert" which
does not exist (assert_caled_with, assert_foobar, assert_called, etc.)
this is assumed to be a mistake and you get an AttributeError so you
notice there was a mistake.

If you pass unsafe=True, you can still call mock.assert_foo() methods
as usual and they will return a new Mock, just as calling, say,
mock.spam() would.

> >The thing people are discussing about is whether this should also
> >happen if you do "m.assret_called_with()" (i.e. if this is a common
> >enough typo to warrant a special exception), or if*only*  things
> >starting with assert_* are checked.
> >
> >The merged patch also treats "assret_*" as a typo, and some people
> >think this is a wart/unnecessary/harmful and only "assert_*" should
> >raise an AttributionError, i.e. the patch should be amended/reverted.
> 
> I think it would be easy enough to revert, It' just a few line in the source
> and docs.  No big deal there.
> 
> It's not clear why getting an AttributeError for methods beginning with
> assert is needed, and how that exception is to be used.   (Should it Bubble
> up, or should it be caught and handled?)

Note the discussion *isn't* about the fact that assert-methods should
raise AttributeError! The patch also does the same with "assret".

At least if I understand things correctly, the discussion is whether
*only* "assert*" should be handled as a typo, or "assret*" too.

The exception should bubble up, as the whole point of it is to tell
you you did a typo and your test is broken.

Florian

-- 
http://www.the-compiler.org | me at the-compiler.org (Mail/XMPP)
   GPG: 916E B0C8 FD55 A072 | http://the-compiler.org/pubkey.asc
         I love long mails! | http://email.is-not-s.ms/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://mail.python.org/pipermail/python-dev/attachments/20150720/47427170/attachment-0001.sig>