bpo-30541: Add new method to seal mocks #1923
Conversation
The new method allows the developer to control when to stop the feature of mocks that automagically creates new mocks when accessing an attribute that was not declared before Signed-off-by: Mario Corchero <mariocj89@gmail.com>
|
@mariocj89, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mfoord, @kushaldas and @berkerpeksag to be potential reviewers. |
|
👍 |
If an user assigns a mock to the attribute of another mock, dont recurse on those when sealing, providing a way to the users to ignore parts when performing a seal
ebb5959 to
69be962
Compare
Lib/unittest/mock.py
Outdated
| __dict__['_mock_name'] = name | ||
| __dict__['_mock_new_name'] = _new_name | ||
| __dict__['_mock_new_parent'] = _new_parent | ||
| __dict__['_is_sealed'] = False |
There was a problem hiding this comment.
I suggest '_mock_sealed' key instead to remain consistent and reduce the risk of name collision.
Lib/unittest/mock.py
Outdated
|
|
||
| if self._is_sealed: | ||
| mock_name = self._extract_mock_name() + name | ||
| raise AttributeError("Cannot set " + mock_name) |
There was a problem hiding this comment.
Why not just using repr() here?
"Cannot set %r" % self
There was a problem hiding this comment.
Just realized that this wont allow to set even existing attributes, as in the following will fail:
m = mock.Mock()
m.test.test2 = 1
mock.seal(m)
m.test.test2 = 2To be consistent with the logic on gettr, it is probably better to dont allow to set new attributes. I've changed it to do so in 4044ffa
With that change, repr does not contain the attribute that caused the issue.
mock.test1.test3.test4 = 1 will give:
Using repr:
AttributeError(Cannot set <MagicMock name='mock.test1.test3' id='4337541584'>)
Using mock_name:
AttributeError(Cannot set mock.test1.test3.test4)
Note the former lacks test4
Lib/unittest/mock.py
Outdated
| self(val) | ||
|
|
||
|
|
||
| def seal(in_mock): |
There was a problem hiding this comment.
Use "mock" parameter name, see mock_open().
| if not isinstance(m, NonCallableMock): | ||
| continue | ||
| if m._mock_new_parent is in_mock: | ||
| seal(m) |
There was a problem hiding this comment.
Would it work to use _mock_methods and _mock_children attributes instead of dir()?
There was a problem hiding this comment.
It wont recurse on magic methods, and not only for magic mocks but also for __call__ for example.
The following won't work:
x = Mock()
x.return_value.a = 1
seal(x)
x().a # this will raise
I can try to make it work if you prefer it that way, use those two attributes and manually add return_value and magic methods.
| @@ -0,0 +1,164 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Please add "_" in the filename. For example, rename it to test_seal.py.
There was a problem hiding this comment.
Done, didn't add it to be consistent with the rest of the tests in testmock folder.
I can change the others as well in a separate commit if you want to.
| assert isinstance(m.test, mock.Mock) | ||
| assert isinstance(m.test(), mock.Mock) | ||
| assert isinstance(m.test().test2(), mock.Mock) | ||
|
|
There was a problem hiding this comment.
PEP8: only one empty line between methods.
There was a problem hiding this comment.
wops, indeed, blindly converted them from plain functions in pytest and missed it, should have lint it before submitting it again sorry.
Confirmed the score for mock.py hasn't go down (prev run = master):
pylint: Your code has been rated at 7.99/10 (previous run: 7.98/10, +0.01)
New code follows pep8:
$ pep8 unittest/test/testmock/test_sealable.py && echo OK
OK
Lib/unittest/mock.py
Outdated
| >>> seal(mock) | ||
| >>> mock.submock.attribute2 # This will raise | ||
| >>> mock.not_submock.attribute2 # This won't raise | ||
There was a problem hiding this comment.
Please move this doc to Doc/library/unittest.mock.rst. Don't forget the ".. versionadded:: 3.7" marker.
| if not isinstance(m, NonCallableMock): | ||
| continue | ||
| if m._mock_new_parent is in_mock: | ||
| seal(m) |
There was a problem hiding this comment.
It wont recurse on magic methods, and not only for magic mocks but also for __call__ for example.
The following won't work:
x = Mock()
x.return_value.a = 1
seal(x)
x().a # this will raise
I can try to make it work if you prefer it that way, use those two attributes and manually add return_value and magic methods.
Lib/unittest/mock.py
Outdated
|
|
||
| if self._is_sealed: | ||
| mock_name = self._extract_mock_name() + name | ||
| raise AttributeError("Cannot set " + mock_name) |
There was a problem hiding this comment.
Just realized that this wont allow to set even existing attributes, as in the following will fail:
m = mock.Mock()
m.test.test2 = 1
mock.seal(m)
m.test.test2 = 2To be consistent with the logic on gettr, it is probably better to dont allow to set new attributes. I've changed it to do so in 4044ffa
With that change, repr does not contain the attribute that caused the issue.
mock.test1.test3.test4 = 1 will give:
Using repr:
AttributeError(Cannot set <MagicMock name='mock.test1.test3' id='4337541584'>)
Using mock_name:
AttributeError(Cannot set mock.test1.test3.test4)
Note the former lacks test4
| @@ -0,0 +1,164 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Done, didn't add it to be consistent with the rest of the tests in testmock folder.
I can change the others as well in a separate commit if you want to.
| assert isinstance(m.test, mock.Mock) | ||
| assert isinstance(m.test(), mock.Mock) | ||
| assert isinstance(m.test().test2(), mock.Mock) | ||
|
|
There was a problem hiding this comment.
wops, indeed, blindly converted them from plain functions in pytest and missed it, should have lint it before submitting it again sorry.
Confirmed the score for mock.py hasn't go down (prev run = master):
pylint: Your code has been rated at 7.99/10 (previous run: 7.98/10, +0.01)
New code follows pep8:
$ pep8 unittest/test/testmock/test_sealable.py && echo OK
OK
|
While the change LGTM, I would prefer to see a review of someone who knows the unittest module better than me :-( @vadmium or @berkerpeksag maybe? I don't know. @voidspace: are you still around? :-) |
Doc/library/unittest.mock.rst
Outdated
| >>> mock.submock.attribute2 # This will raise | ||
| >>> mock.not_submock.attribute2 # This won't raise | ||
|
|
||
| .. versionadded:: 3.7 |
There was a problem hiding this comment.
Indentation is off here. Please indent it like the following:
.. function:: seal(mock)
...
.. versionadded:: 3.7
Doc/library/unittest.mock.rst
Outdated
| >>> mock.submock.attribute1 = 2 | ||
| >>> mock.not_submock = mock.Mock() | ||
| >>> seal(mock) | ||
| >>> mock.submock.attribute2 # This will raise |
There was a problem hiding this comment.
I'd change it to # This will raise AttributeError..
Lib/unittest/mock.py
Outdated
|
|
||
| if self._mock_sealed and not getattr(self, name): | ||
| mock_name = self._extract_mock_name() + "." + name | ||
| raise AttributeError("Cannot set " + mock_name) |
There was a problem hiding this comment.
You could use an f-string here.
Lib/unittest/mock.py
Outdated
| self._mock_children[name] = value | ||
|
|
||
| if self._mock_sealed and not getattr(self, name): | ||
| mock_name = self._extract_mock_name() + "." + name |
There was a problem hiding this comment.
Most of the module use single quotes. It would be nice to follow the existing style.
Lib/unittest/mock.py
Outdated
| if _check_and_set_parent(self, value, name, name): | ||
| self._mock_children[name] = value | ||
|
|
||
| if self._mock_sealed and not getattr(self, name): |
There was a problem hiding this comment.
Did you want to use hasattr here? getattr(self, name) may raise an exception.
| from unittest import mock | ||
|
|
||
|
|
||
| class SampleObject(object): |
There was a problem hiding this comment.
Nitpick: class SampleObject:
|
|
||
|
|
||
| class TestSealable(unittest.TestCase): | ||
| """Validates the ability to seal a mock which freezes its spec""" |
There was a problem hiding this comment.
No need to add a docstring here.
| def test_attributes_return_more_mocks_by_default(self): | ||
| m = mock.Mock() | ||
|
|
||
| assert isinstance(m.test, mock.Mock) |
There was a problem hiding this comment.
Please use standard unittest assert methods, assertIsInstance in this case.
|
|
||
| mock.seal(m) | ||
| try: | ||
| m.SECRETE_name |
There was a problem hiding this comment.
I think the following pattern is more Pythonic and we use it in the CPython test suite:
with self.assertRaises(AttributeError) as cm:
m.SECRETE_name
self.assertIn('SECRET_name', str(cm.exception))| @@ -0,0 +1,185 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
Please rename file name to testsealable.py to follow the mock module standard.
|
@berkerpeksag updated! Happy to change anything else! ヽ(´▽`)/ |
|
@berkerpeksag: Would you mind to review the latest version of this seal PR? |
|
Hi @berkerpeksag ! Just a reminder in case this got outside of your radar! |
|
@Haypo @berkerpeksag it is not happening, right? 😞 I am OK closing it if you think it is not worth it. Just thought it was a good idea from the bpo comments. |
|
Note though that if there is any way I can move it forward I rather prefer it 😄 |
@mariocj89 took all berker comments in account.
|
@Haypo Should I add an entry in |
|
@mariocj89: Yes, please use the blurb tool to add a NEWS entry. Please mention also the new feature in Doc/whatsnew/3.7.rst document in the "unittest" section. In the lack of feedback from @berkerpeksag, I will merge this PR once you add the NEWS entry. |
|
@Haypo updated using blurb and following previous PRs structure. Thanks! |
|
@mariocj89: I merged your PR, thanks for this nice function! Sorry for the delay, unittest.mock is a popular module and it takes time to decide if an addition is worth it, since we will have to maintain it forever. |
|
Thank you! |
The new method allows the developer to control when to stop the feature of mocks that automagically creates new mocks when accessing an attribute that was not declared before.
There is an alternative implementation proposed in issue30541 using classes, but this seems much nicer.
Happy to rename seal to freeze, lock or any other similar name.
https://bugs.python.org/issue30541