bpo-20239: Allow repeated deletion of unittest.mock.Mock attributes #11057

pablogsal · Dec 9, 2018

https://bugs.python.org/issue20239

mariocj89 · Dec 10, 2018

Lib/unittest/test/testmock/testmock.py

+
+            del mock.foo
+            self.assertFalse(hasattr(mock, 'foo'))
+            self.assertRaises(AttributeError, getattr, mock, 'foo')


Seems there is no test in the suite attempting to delete twice an attribute. That should raise, can you add one?

Done in 0c3dde0

mariocj89 · Dec 10, 2018

Misc/NEWS.d/next/Library/2018-12-09-21-35-49.bpo-20239.V4mWBL.rst

@@ -0,0 +1,2 @@
+Allow repeated assignment deletion of ``unittest.mock.Mock`` attributes.


You might want to link to the class here.

Done in 0c3dde0

tirkarthi · Dec 10, 2018

Lib/unittest/mock.py

-        if obj is _deleted:
+        if name in self.__dict__:
+            super().__delattr__(name)
+        elif obj is _deleted:
            raise AttributeError(name)
        if obj is not _missing:


There was a comment in the patch about this line of checking _missing being superfluous and that it could be removed. Is there a test that will break on removing this?

No, no test fail when removing this line. This does not mean is superfluous without inspecting. I prefer to handle that question in a different PR :)

Thanks for the catch!

cjw296

Thanks for this! A couple of tweaks and we're there...

cjw296 · Dec 11, 2018

Lib/unittest/test/testmock/testmock.py

+            self.assertRaises(AttributeError, getattr, mock, 'foo')
+
+
+    def test_mock_raises_when_deleting_inexistent_attribute(self):


inexistent -> nonexistent, but what is this testing that isn't tested above?

It was requested by @mariocj89 because there is no test that tries to delete twice an attribute. The one above is checking that setting+deleting+setting+deleting does not raise, while this one checks that deleting twice does raise.

I will eliminate the extra checks now that we have a second test for that, so every test only test one thing.

cjw296 · Dec 11, 2018

Lib/unittest/test/testmock/testmock.py

@@ -1769,6 +1769,35 @@ def test_attribute_deletion(self):
            self.assertRaises(AttributeError, getattr, mock, 'f')


+    def test_mock_does_not_raise_on_repeated_attribute_deletion(self):
+        # bpo-20239: Assigning and deleting twice an attribute raises.


No sure we need the bpo comment, what will this mean in 10 years time when we're on a different tracker?

We need it precisely to distinguish the tracker. For example, there is a PEP proposing moving to the GitHub issue tracker but the existing issues won't be moved. Therefore, issuexxxx is ambiguous while bpo-xxxx is not.

I meant not having the comment at all, what value does the comment add? The test name and its code should be enough :-)

cjw296 · Dec 11, 2018

Lib/unittest/test/testmock/testmock.py

@@ -1769,6 +1769,35 @@ def test_attribute_deletion(self):
            self.assertRaises(AttributeError, getattr, mock, 'f')


+    def test_mock_does_not_raise_on_repeated_attribute_deletion(self):


This doesn't seem like the right name, we appear to be testing that deleting an attribute of a mock does raise an AttributeError on the second attempt. test_delete_of_deleted_raises_attribute_error?

I will eliminate the extra checks now that we have a second test for that.

cjw296 · Dec 11, 2018

Lib/unittest/test/testmock/testmock.py

+                     NonCallableMock()):
+            with self.assertRaises(AttributeError):
+                del mock.foo
+                del mock.foo


Which of these two statements is doing the raising? Only one of them should be wrapped with the assertRaises.

bedevere-bot · Dec 11, 2018

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pablogsal · Dec 11, 2018

I have made the requested changes; please review again

bedevere-bot · Dec 11, 2018

Thanks for making the requested changes!

@cjw296: please review the changes made to this pull request.

pablogsal · Jan 20, 2019

@cjw296 Gentle ping

bedevere-bot · Jan 21, 2019

@cjw296: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · Jan 21, 2019

Thanks @pablogsal for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · Jan 21, 2019

GH-11629 is a backport of this pull request to the 3.7 branch.

…ythonGH-11057) * Allow repeated deletion of unittest.mock.Mock attributes * fixup! Allow repeated deletion of unittest.mock.Mock attributes * fixup! fixup! Allow repeated deletion of unittest.mock.Mock attributes (cherry picked from commit 222d303) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

Allow repeated deletion of unittest.mock.Mock attributes

9eb6def

the-knights-who-say-ni added the CLA signed label Dec 9, 2018

bedevere-bot added the awaiting merge label Dec 9, 2018

pablogsal requested a review from cjw296 December 9, 2018 21:37

pablogsal self-assigned this Dec 9, 2018

mariocj89 reviewed Dec 10, 2018

View reviewed changes

tirkarthi reviewed Dec 10, 2018

View reviewed changes

fixup! Allow repeated deletion of unittest.mock.Mock attributes

0c3dde0

mariocj89 approved these changes Dec 11, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Dec 11, 2018

tirkarthi approved these changes Dec 11, 2018

View reviewed changes

cjw296 requested changes Dec 11, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Dec 11, 2018

fixup! fixup! Allow repeated deletion of unittest.mock.Mock attributes

e01c333

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 11, 2018

cjw296 approved these changes Jan 21, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 21, 2019

cjw296 merged commit 222d303 into python:master Jan 21, 2019

bedevere-bot removed the awaiting merge label Jan 21, 2019

cjw296 added the needs backport to 3.7 label Jan 21, 2019

bedevere-bot removed the needs backport to 3.7 label Jan 21, 2019

pablogsal deleted the bpo20239 branch May 19, 2021 18:57

		@@ -0,0 +1,2 @@
		Allow repeated assignment deletion of ``unittest.mock.Mock`` attributes.

		self.assertRaises(AttributeError, getattr, mock, 'foo')


		def test_mock_raises_when_deleting_inexistent_attribute(self):

Search code, repositories, users, issues, pull requests...

Uh oh!

bpo-20239: Allow repeated deletion of unittest.mock.Mock attributes #11057

bpo-20239: Allow repeated deletion of unittest.mock.Mock attributes #11057

Uh oh!

Conversation

pablogsal commented Dec 9, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pablogsal Dec 10, 2018 • 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!

cjw296 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 11, 2018 • 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!

bedevere-bot commented Dec 11, 2018

Uh oh!

pablogsal commented Dec 11, 2018

Uh oh!

bedevere-bot commented Dec 11, 2018

Uh oh!

pablogsal commented Jan 20, 2019

Uh oh!

bedevere-bot commented Jan 21, 2019

Uh oh!

miss-islington commented Jan 21, 2019

Uh oh!

bedevere-bot commented Jan 21, 2019

Uh oh!

Uh oh!

pablogsal commented Dec 9, 2018 •

edited by bedevere-bot

Loading

pablogsal Dec 10, 2018 •

edited

Loading

pablogsal Dec 11, 2018 •

edited

Loading