bpo-20239: Allow repeated deletion of unittest.mock.Mock attributes by pablogsal · Pull Request #11057 · python/cpython

pablogsal · 2018-12-09T21:36:54Z

https://bugs.python.org/issue20239

mariocj89 · 2018-12-10T00:08:05Z

+
+            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 · 2018-12-10T00:08:20Z

@@ -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 · 2018-12-10T00:55:32Z

+            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 · 2018-12-11T06:04:11Z

+            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 · 2018-12-11T06:04:55Z



+    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 · 2018-12-11T06:06:17Z

            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 · 2018-12-11T06:08:23Z

+                     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 · 2018-12-11T06:08:56Z

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

pablogsal · 2018-12-11T09:44:33Z

I have made the requested changes; please review again

bedevere-bot · 2018-12-11T09:44:35Z

Thanks for making the requested changes!

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

pablogsal · 2019-01-20T18:58:38Z

@cjw296 Gentle ping

bedevere-bot · 2019-01-21T08:57:50Z

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

miss-islington · 2019-01-21T08:58:04Z

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

bedevere-bot · 2019-01-21T08:58:20Z

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



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

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pablogsal commented Dec 9, 2018 •

edited by bedevere-bot

Loading

pablogsal Dec 10, 2018 •

edited

Loading

pablogsal Dec 11, 2018 •

edited

Loading