bpo-30541: Add new method to seal mocks by mariocj89 · Pull Request #1923 · python/cpython

mariocj89 · 2017-06-02T19:49:18Z

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

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 <[email protected]>

mention-bot · 2017-06-02T19:49:20Z

@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.

lkollar · 2017-06-03T09:51:02Z

👍

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

vstinner · 2017-06-06T14:58:25Z

        __dict__['_mock_name'] = name
        __dict__['_mock_new_name'] = _new_name
        __dict__['_mock_new_parent'] = _new_parent
+        __dict__['_is_sealed'] = False


I suggest '_mock_sealed' key instead to remain consistent and reduce the risk of name collision.

vstinner · 2017-06-06T14:59:39Z

+
+        if self._is_sealed:
+            mock_name = self._extract_mock_name() + name
+            raise AttributeError("Cannot set " + mock_name)


Why not just using repr() here?

"Cannot set %r" % self

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 = 2

To 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

vstinner · 2017-06-06T15:04:45Z

        self(val)
+
+
+def seal(in_mock):


Use "mock" parameter name, see mock_open().

vstinner · 2017-06-06T15:05:53Z

+        if not isinstance(m, NonCallableMock):
+            continue
+        if m._mock_new_parent is in_mock:
+            seal(m)


Would it work to use _mock_methods and _mock_children attributes instead of dir()?

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.

vstinner · 2017-06-06T15:06:32Z

@@ -0,0 +1,164 @@
+import unittest


Please add "_" in the filename. For example, rename it to test_seal.py.

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.

vstinner · 2017-06-06T15:07:04Z

+        assert isinstance(m.test, mock.Mock)
+        assert isinstance(m.test(), mock.Mock)
+        assert isinstance(m.test().test2(), mock.Mock)
+


PEP8: only one empty line between methods.

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

vstinner · 2017-06-06T15:08:05Z

+    >>> seal(mock)
+    >>> mock.submock.attribute2  # This will raise
+    >>> mock.not_submock.attribute2  # This won't raise
+


Please move this doc to Doc/library/unittest.mock.rst. Don't forget the ".. versionadded:: 3.7" marker.

mariocj89

Updated! Plus some answers.

Thanks for having a look @Haypo

mariocj89 · 2017-06-06T18:19:51Z

+        if not isinstance(m, NonCallableMock):
+            continue
+        if m._mock_new_parent is in_mock:
+            seal(m)


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.

mariocj89 · 2017-06-06T18:45:11Z

+
+        if self._is_sealed:
+            mock_name = self._extract_mock_name() + name
+            raise AttributeError("Cannot set " + mock_name)


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 = 2

To 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

mariocj89 · 2017-06-06T18:49:53Z

@@ -0,0 +1,164 @@
+import unittest


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.

mariocj89 · 2017-06-06T18:58:39Z

+        assert isinstance(m.test, mock.Mock)
+        assert isinstance(m.test(), mock.Mock)
+        assert isinstance(m.test().test2(), mock.Mock)
+


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

vstinner · 2017-06-15T21:39:39Z

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? :-)

berkerpeksag · 2017-06-16T04:43:13Z

+        >>> mock.submock.attribute2  # This will raise
+        >>> mock.not_submock.attribute2  # This won't raise
+
+.. versionadded:: 3.7


Indentation is off here. Please indent it like the following:

.. function:: seal(mock) ... .. versionadded:: 3.7

berkerpeksag · 2017-06-16T04:44:32Z

+        >>> mock.submock.attribute1 = 2
+        >>> mock.not_submock = mock.Mock()
+        >>> seal(mock)
+        >>> mock.submock.attribute2  # This will raise


I'd change it to # This will raise AttributeError..

berkerpeksag · 2017-06-16T04:45:35Z

+
+        if self._mock_sealed and not getattr(self, name):
+            mock_name = self._extract_mock_name() + "." + name
+            raise AttributeError("Cannot set " + mock_name)


You could use an f-string here.

berkerpeksag · 2017-06-16T04:46:05Z

                self._mock_children[name] = value
+
+        if self._mock_sealed and not getattr(self, name):
+            mock_name = self._extract_mock_name() + "." + name


Most of the module use single quotes. It would be nice to follow the existing style.

berkerpeksag · 2017-06-16T04:50:27Z

            if _check_and_set_parent(self, value, name, name):
                self._mock_children[name] = value
+
+        if self._mock_sealed and not getattr(self, name):


Did you want to use hasattr here? getattr(self, name) may raise an exception.

berkerpeksag · 2017-06-16T04:55:02Z

+from unittest import mock
+
+
+class SampleObject(object):


Nitpick: class SampleObject:

berkerpeksag · 2017-06-16T04:55:30Z

+
+
+class TestSealable(unittest.TestCase):
+    """Validates the ability to seal a mock which freezes its spec"""


No need to add a docstring here.

berkerpeksag · 2017-06-16T04:56:11Z

+    def test_attributes_return_more_mocks_by_default(self):
+        m = mock.Mock()
+
+        assert isinstance(m.test, mock.Mock)


Please use standard unittest assert methods, assertIsInstance in this case.

berkerpeksag · 2017-06-16T04:58:49Z

+
+        mock.seal(m)
+        try:
+            m.SECRETE_name


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))

berkerpeksag · 2017-06-16T05:00:50Z

@@ -0,0 +1,185 @@
+import unittest


Please rename file name to testsealable.py to follow the mock module standard.

mariocj89 · 2017-06-30T23:21:08Z

@berkerpeksag updated! Happy to change anything else! ヽ(´▽`)/

vstinner · 2017-07-25T15:38:49Z

@berkerpeksag: Would you mind to review the latest version of this seal PR?

mariocj89 · 2017-08-16T08:49:49Z

Hi @berkerpeksag ! Just a reminder in case this got outside of your radar!

mariocj89 · 2017-09-01T14:41:37Z

@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.

mariocj89 · 2017-09-01T15:03:57Z

Note though that if there is any way I can move it forward I rather prefer it 😄

@mariocj89

@mariocj89 took all berker comments in account.

vstinner

LGTM.

mariocj89 · 2017-10-16T23:07:18Z

@Haypo Should I add an entry in Misc/NEWS.d/next/ ?

vstinner · 2017-10-17T10:18:53Z

@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.

mariocj89 · 2017-10-17T11:24:32Z

@Haypo updated using blurb and following previous PRs structure. Thanks!

vstinner · 2017-10-17T11:36:20Z

@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.

mariocj89 · 2017-10-17T11:39:14Z

Thank you!

mariocj89 added 2 commits June 1, 2017 21:48

Add new method to seal mocks

9ba039e

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 <[email protected]>

Ensure attributes cannot be set on a sealed mock

ba16493

the-knights-who-say-ni added the CLA signed label Jun 2, 2017

Mariatta added the type-feature A feature request or enhancement label Jun 3, 2017

Dont seal mocks which are asigned manually

69be962

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

mariocj89 force-pushed the sealablemock_method branch from ebb5959 to 69be962 Compare June 5, 2017 19:43

vstinner requested changes Jun 6, 2017

View reviewed changes

mariocj89 added 7 commits June 6, 2017 19:09

Rename _is_sealed to _mock_sealed

ed2a670

Rename input parameter in_mock to mock in seal

ef61569

Add test for magic mocks

3039bcf

Allow to set existing attributes on sealed mocks

4044ffa

Rename testsealable to test_sealable

0a9bf3f

Fix pep8 violations in test_sealed

e23d151

Move seal example to docs

d94dbf5

mariocj89 commented Jun 6, 2017

View reviewed changes

vstinner previously approved these changes Jun 15, 2017

View reviewed changes

berkerpeksag previously requested changes Jun 16, 2017

View reviewed changes

mariocj89 added 2 commits June 16, 2017 08:57

Minor fixes in the docs

0b908ae

Address comments from last review

b8fc6b7

mariocj89 mentioned this pull request Jun 18, 2017

Add a seal method to the api to allow mocking existing objects mariocj89/sealedmock#1

Closed

vstinner approved these changes Sep 4, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Sep 4, 2017

Add entry in News

2b9a271

vstinner merged commit 552be9d into python:master Oct 17, 2017

bedevere-bot removed the awaiting merge label Oct 17, 2017

mariocj89 deleted the sealablemock_method branch October 17, 2017 11:38

pganssle mentioned this pull request Jan 5, 2018

[bpo-30541] Cleanup grammar in unittest.mock.seal documentation #5107

Merged



		class TestSealable(unittest.TestCase):
		"""Validates the ability to seal a mock which freezes its spec"""

Uh oh!

Conversation

mariocj89 commented Jun 2, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jun 2, 2017

Uh oh!

lkollar commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 commented Jun 2, 2017 •

edited by bedevere-bot

Loading

mariocj89 Jun 6, 2017 •

edited

Loading

mariocj89 Jun 6, 2017 •

edited

Loading