[issue24651] Mock.assert* API is in user namespace

[Gregory P. Smith]

Change by Gregory P. Smith :


--
versions: +Python 3.10 -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Gregory P. Smith]

Change by Gregory P. Smith :


--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Václav Brožek]

Václav Brožek  added the comment:

As for the assert misspellings raising AttributeError -- that's not true of all 
of them, only those starting with "assret" or "assert" [1].

In my company, we recently cleaned up a bunch of other assert-misspellings, 
including cases where a real bug was masked in the code by that. That's why I 
filed https://bugs.python.org/issue41877.

IMO, the fact that the assert-misspellings issue is not completely fixed seems 
to increase the motivation to add a module-level assert.

While it's easy to add further misspellings to the check in [1], the general 
solution proposed in this ticket avoids a length whack-a-mole for code owners 
who would be able / willing to switch to a module-level assert.

[1] https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L634

--
nosy: +vabr2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Brett Cannon]

Change by Brett Cannon :


--
nosy:  -brett.cannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Robert Collins]

Robert Collins added the comment:

I don't mind if this is open or closed: we've now got a clear set of 
requirements *should* someone want to work on it::

 - it would feel as convenient to use as the current API does

 - it would be possible to be both forward and backwards compatible with the 
existing API indefinitely: There are lots of users of mock, and offering a 
[presumably] improved API is not worth turning all that code into an instant 
sea-anchor: if we're going to change this, we need to do so in a -very- 
graceful fashion, because unlike e.g. the mock_open things that only affect 
some users of mock, this would (if done poorly) break every user ever.


I think its possible to deliver that systematically across the API (not just 
the asserts - the whole thing) - but its nontrivial.

--
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Kevin Benton]

Kevin Benton added the comment:

I like the idea of having the new module functions without deprecating the old 
ones. Then projects can encourage use of the API on their own terms. Maybe 
there could even be a global that projects could set that would disable the 
object methods...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Erik Bray]

Erik Bray added the comment:

Would it be possible to add the functions and *not* deprecate assert_ methods?

The existing solution is still acknowledged to be an incomplete solution.  It 
is still possible to make other typos that result in unintentional 
non-assertions (it's also not entirely clear from the docs when one would want 
to set unsafe=True, though at least the default unsafe=False is better).  I'm 
not against the existing solution--I think it's reasonable and that calls of 
"special cases yadda yadda" are unfair. Sometimes practicality beats purity.

So, although adding assert_ functions would be a bit redundant, it's also 
guaranteed safer, and so probably worth making an exception for.  

There's a tradeoff here between convenience and robustness and I think we can 
have it both ways.  If I were to write new tests using Mock I would probably 
define such functions myself, but that would get repetitive quickly.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Michael Foord]

Michael Foord added the comment:

I had a chance to talk to Guido about this at EuroPython. His feeling was that 
a separate set of functions *is* a better API - or at least would have been if 
mock had been designed this way from the start. 

However, the most serious problem with the current API is solved (as assert* 
attributes that don't exist now raise an AttributeError) and it isn't worth 
breaking people's code by deprecating then removing the assert methods just for 
a slightly cleaner API.

As I agree about breaking code I'm inclined to close this with WontFix unless 
someone has strong feelings that it should be left open.

(Thanks Robert for the proposal - it's hard for me not to feel emotionally 
attached to the current API, which makes it correspondingly hard to be 
objective about it, but I appreciate people's desires to improve mock.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Erik Bray]

Erik Bray added the comment:

As I wrote (late) in the thread that shall not be named, I don't think it makes 
any sense to have magic assert_ methods on the Mock object.  Not only does the 
"magic" clearly lead to too many ambiguities and other problems--I think they 
make less sense from an API standpoint in the first place.  Typically asserting 
something in a test is not something an object *does*--a method.  More often we 
as a test writers assert something *about* an object.  The assertion is an 
external tool meant to measure and introspect things about the system under 
observation.  In this case, although Mock is itself a testing tool, we're 
introspecting something about the Mock object as external observers.

Regarding the points raised about accessing other attributes like .calls and 
.returnvalue, and Robert's idea about set_returnvalue make me think that this 
could be softened somewhat.  Michael is right that the assert_ problem is 
mostly solved now.  Though I still think special casing one way of mistyping 
assert is the wrong way to go, and I feel like the "unsafe" option is more of 
kludge than anything, though it mostly gets the job done.

So rather than go whole hog and say that all introspection on and interaction 
with the Mock object should be via functions (which I agree would weaken much 
of its convenience), only assertions should be via functions.

As a recent user of the mock package I've found it strange, even before this 
controversy, that I called assert_ as *methods* on the mock object itself. 
 In all past cases when I've wanted to write special assertions for my classes 
I've written those as functions. In the case of Mock, using assert functions 
would have none of the spelling issues, and would be quite easy to drop into 
any test.  It might also open up possibilities with duck-typing, though I'm not 
sure to what end yet...

TL;DR, I think keep the existing assert_ methods for backward compat, including 
the unsafe flag and assret check, but add assert_ functions for those who would 
prefer such an API, and promote them in the docs as safer w.r.t. misspellings, 
and ensuring that your assertions are called.
If it would help sweeten the deal, the assert_ functions could even come as 
staticmethods on the Mock class, so as to reduce the effort in writing import 
statements.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Robert Collins]

Changes by Robert Collins :


--
priority: normal -> low

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Robert Collins]

Robert Collins added the comment:

Some great points raised here.

Yes, we'd need .calls and .returnvalue and so on all made accessible via 
another route, and that would be a less direct spelling.

e.g. a_mock.returnvalue = 3
-> 
# function
set_returnvalue(a_mock, 3)
or
# separate object to get at the mock housekeeping vs the mocked interface
mocki(a_mock).returnvalue = 3

etc.

None of which make me want to say 'wow thats a great api'.

I wouldn't want to advocate for a patch for this unless:
 - it would feel as convenient to use as the current API does
 - it would be possible to be both forward and backwards compatible with the 
existing API indefinitely: There are lots of users of mock, and offering a 
[presumably] improved API is not worth turning all that code into an instant 
sea-anchor: if we're going to change this, we need to do so in a -very- 
graceful fashion, because unlike e.g. the mock_open things that only affect 
some users of mock, this would (if done poorly) break every user ever.

For clarity, I have no plans to write such a patch at this time, though if 
someone did come up with a proof of concept I'd happily review it, and dig up 
Michael to get his thoughts *if and only if* it met the two constraints above.

I'm not sure of the right bug tracker state to say 'this is a maybe, maintainer 
isn't enthused about it'.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Erik Bray]

Changes by Erik Bray :


--
nosy: +erik.bray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Carl Meyer]

Carl Meyer added the comment:

FWIW, my assumption was that the typical usage pattern would be `import mock` 
rather than separate imports of all the assertions, so I don't think there'd 
really be an increase in what users need to know about or import. (They already 
need to know about the methods on the mock object, knowing about functions in a 
module isn't practically any different.)

But the "breaking working code for insufficient benefit" argument is strong 
indeed, and personally I'm happy to defer to the module author on this one.

Thanks for writing and maintaining mock!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Michael Foord]

Michael Foord added the comment:

I'm not wild about this idea. The problem with the assert methods has 
*essentially* been solved now, so I'm not convinced of the need for this change 
(unless users really *need* to have their own mocked attributes like 
"assert_called_with" which I think is highly unlikely).

Part of the genius of mock was providing a flexible mock object that also 
encapsulated simple methods for introspecting/asserting how it has been used. 
Changing to require users to import/know about a whole host of separate 
functions doesn't feel like an improvement to me. That's aside from the whole 
"breaking people's code for no tangible benefit" issue.

I acknowledge that other people, Carl for example, have different opinions - 
but from talking to many, many mock users over the years I think that those 
with the philosophically purist approach are in a minority to those who 
appreciate the more practical approach that mock takes.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Felipe]

Felipe added the comment:

Not sure it's my place to comment here, but here are my 2 cents: I think 
Robert's proposal to have module functions is the only way to have a 
user-friendly and robust API, and it solves more than just the assert typo 
problem. (And yes, it would require moving the mock public API entirely to 
functions/classmethods).

To me, there's an underlying fragility in mock since the current API for `Mock` 
is not cleanly separated from the mocked API. This issue creates the problem of 
the  assert typos, and also creates problems with name conflicts (I've always 
thought the `.call_count` attribute was particularly likely to be clobbered).

The only bullet-proof way I can think of to ensure such a conflict does not 
take place is to separate the namespaces altogether, by moving the data out of 
the Mock object and into a global structure. E.g., `mock.Mock` could have a 
class attribute (say `mock.Mock.call_log`) tracking all of the calls to all 
mocks, and there could be a series of classmethods to query this store. 
Unfortunately, this design goes seriously against the grain of OOP, but we're 
essentially back to Robert's proposal.

A more OOP-friendly approach sacrifices the '100% clash-proof guarantee' and 
only provides a 'highly unlikely to clash' guarantee instead: Mangle the mock 
API namespace. Mock currently does this for its internal attributes (e.g., 
`._mock_parent`) but not for its public API (e.g., `.assert_called_once_with`). 
To remain user-friendly, of course we wouldn't require users to mangle names by 
hand, but would provide convenience functions to access these mangled 
attributes/methods, which puts us right back at Robert's proposal.

--
nosy: +fov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Kevin Benton]

Kevin Benton added the comment:

What about other methods/properties like called, call_count, and reset_mock? It 
seems that they should be removed as well to be consistent with the reason for 
this change.

--
nosy: +kevinbenton

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Ben Finney]

Changes by Ben Finney :


--
nosy: +bignose

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Antti Haapala]

Changes by Antti Haapala :


--
nosy: +ztane

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Patrick Westerhoff]

Changes by Patrick Westerhoff :


--
nosy: +poke

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Carl Meyer]

Carl Meyer added the comment:

Er, I meant `AttributeError`, of course...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Carl Meyer]

Carl Meyer added the comment:

As a frequent and long-time user of mock, the `assert_*` methods being on the 
mock object itself has always struck me as an unfortunate wart on an otherwise 
great library. The change to raise `AssertionError` on `assert_*` and 
`assret_*` feels like piling an ugly band-aid on top of the wart. I'd love to 
see Robert's suggested solution of moving the assertion helpers to standalone 
functions.

--
nosy: +carljm

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Brett Cannon]

Changes by Brett Cannon :


--
nosy: +brett.cannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Robert Collins]

New submission from Robert Collins:

We had a discussion on the list sparked by the assret checking, and in it I 
proposed that the API would be cleaner if the asserts were module functions.

e.g. rather than::

 a_mock.assert_called_with(Foo)


 assert_called_with(a_mock, Foo)

Michael has objected to this saying that the current structure is part of 
mock's success - but I'm filing this since a number of other core devs seemed 
to really like the idea. We can discuss here and if the consensus is that it 
wouldn't be an improvement - thats fine. OTOH if the consensus is that it is an 
improvement, this can serve as a memo to someone to implement the new API.

--
components: Library (Lib)
messages: 246846
nosy: rbcollins
priority: normal
severity: normal
status: open
title: Mock.assert* API is in user namespace
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24651] Mock.assert* API is in user namespace

[Robert Collins]

Changes by Robert Collins :


--
nosy: +berker.peksag, kushal.das, michael.foord

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com