Issue45215
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2021-09-16 02:13 by andrei.avk, last changed 2022-04-11 14:59 by admin.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 28378 | open | andrei.avk, 2021-09-16 02:39 |
Messages (4) | |||
---|---|---|---|
msg401913 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-09-16 02:13 | |
Currently using *name* and *parent* args in Mock and MagicMock is problematic in a few ways: *name* - any value can be passed silently but at a later time, any value except for an str will cause an exception when repr() or str() on the mock object is done. - a string name can be passed but will not be equal to the respective attr value: Mock(name='foo').name != 'foo', as users would expect. (this should be documented). *parent* - any value can be passed but, similarly to *name*, will cause an exception when str() or repr() is done on the object. - this arg is not documented so users will expect it to be set as an attr, but instead the attribute is going to be a Mock instance. [1] I propose to fix these issues by: - checking the types that are passed in and display a DeprecationWarning if types are wrong. (note that this check should be fast because at first value can be compared to None default, which is what it's going to be in vast majority of cases, and isinstance() check is only done after that.) (in 3.11) - in 3.12, convert warnings into TypeError. - Document that *name* attribute will be a Mock instance. - Document that *name* argument needs to be a string. - Document *parent* argument. - In the docs for the two args, point to `configure_mock()` method for setting them to arbitrary values. (Note that other args for Mock() have more specialized names and are much less likely to cause similar issues.) [1] https://bugs.python.org/issue39222 |
|||
msg401990 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-09-16 20:39 | |
I forgot to include example of the breakage: >>> m=Mock(name=1) >>> m Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ak/opensource/cpython2/Lib/unittest/mock.py", line 735, in __repr__ name = self._extract_mock_name() ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/ak/opensource/cpython2/Lib/unittest/mock.py", line 732, in _extract_mock_name return ''.join(_name_list) ^^^^^^^^^^^^^^^^^^^ TypeError: sequence item 0: expected str instance, int found >>> m=Mock(parent='foo') >>> m Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ak/opensource/cpython2/Lib/unittest/mock.py", line 735, in __repr__ name = self._extract_mock_name() ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/ak/opensource/cpython2/Lib/unittest/mock.py", line 719, in _extract_mock_name _name_list.append(_parent._mock_new_name + dot) ^^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'str' object has no attribute '_mock_new_name' |
|||
msg402369 - (view) | Author: Łukasz Langa (lukasz.langa) * | Date: 2021-09-21 21:39 | |
Have you encountered misuse of Mock's `name` and `parent` in the wild? As you're saying, since `str()` and `repr()` will error out anyway, it's unlikely users were doing this in the first place. I'm a little hesitant to accept the deprecation since there are very many places in the stdlib where we accept an equivalent of, say, `name=` during some object's `__init__()` and just assign it to a field. If that happens not to be printable and crash `repr()` then too bad... but we won't be adding checks for each such case, right? |
|||
msg402404 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-09-22 02:09 | |
Łukasz: For parent, there's this issue: #39222 (which is why I started looking into this). For name, I couldn't find anything here or on SO, but it's hard to search for this because keywords 'mock argument name' on SO finds over 700 results but they're referring to argument names, rather than literal argument `name`. I didn't look through all 700 though :) The scenario that I was thinking about that could cause issues is that one developer creates a bunch of tests with Mock(name=1) for example, that doesn't cause any issues if the mocks aren't printed (for example if they're attributes). Some time later another dev is expanding or debugging the tests and needs to print the mocks, which causes errors. Now the 2nd dev will need to figure out why the first dev used these names and if they can be safely changed to strings - quite annoying. It seems like the instances in the std lib of name args are of the form: init(name) self.name = name repr() return '<... %s ...>' % self.name or equivalent; I've also did a grep for `return self.name` and it was a fairly short list so i was able to check all of them, and all of the hits either internal use classes or functions, or check the type of `name` in __init__, or the return is not from __repr__ but from some other function. I didn't check all instances of where name arg is passed to init, because it's a much longer list and I figured the cases where it could be a problem would be ones with `__repr__(): return self.name`, and those are all fine. The case with Mock is also different because users are not passing it as a known argument but as a "mock accepts all arguments and sets them as attrs" type of usage. On the other hand, I don't think this is a high priority issue, users are probably not using name and parent args that often, and when they do, most of them probably try to print them as they work on the test; and if they do run into this issue later, it's not very hard to fix. So it might be worth just leaving this open and seeing if more users run into it. Let me know what you think! |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:50 | admin | set | github: 89378 |
2021-09-22 02:09:52 | andrei.avk | set | messages: + msg402404 |
2021-09-21 21:39:28 | lukasz.langa | set | nosy:
+ lukasz.langa messages: + msg402369 |
2021-09-16 20:39:22 | andrei.avk | set | messages: + msg401990 |
2021-09-16 02:39:05 | andrei.avk | set | keywords:
+ patch stage: patch review pull_requests: + pull_request26793 |
2021-09-16 02:13:10 | andrei.avk | create |