Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cjw296 merged 3 commits into from
Jan 21, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb6def
Allow repeated deletion of unittest.mock.Mock attributes
pablogsal Dec 9, 2018
0c3dde0
fixup! Allow repeated deletion of unittest.mock.Mock attributes
pablogsal Dec 10, 2018
e01c333
fixup! fixup! Allow repeated deletion of unittest.mock.Mock attributes
pablogsal Dec 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,11 +727,10 @@ def __delattr__(self, name):
# not set on the instance itself
return

if name in self.__dict__:
object.__delattr__(self, name)

obj = self._mock_children.get(name, _missing)
if obj is _deleted:
if name in self.__dict__:
super().__delattr__(name)
elif obj is _deleted:
raise AttributeError(name)
if obj is not _missing:

@tirkarthi tirkarthi Dec 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pablogsal pablogsal Dec 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

del self._mock_children[name]
Expand Down
27 changes: 27 additions & 0 deletions Lib/unittest/test/testmock/testmock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,33 @@ def test_attribute_deletion(self):
self.assertRaises(AttributeError, getattr, mock, 'f')


def test_mock_does_not_raise_on_repeated_attribute_deletion(self):

@cjw296 cjw296 Dec 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pablogsal pablogsal Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# bpo-20239: Assigning and deleting twice an attribute raises.

@cjw296 cjw296 Dec 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pablogsal pablogsal Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cjw296 cjw296 Dec 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for mock in (Mock(), MagicMock(), NonCallableMagicMock(),
NonCallableMock()):
mock.foo = 3
self.assertTrue(hasattr(mock, 'foo'))
self.assertEqual(mock.foo, 3)

del mock.foo
self.assertFalse(hasattr(mock, 'foo'))

mock.foo = 4
self.assertTrue(hasattr(mock, 'foo'))
self.assertEqual(mock.foo, 4)

del mock.foo
self.assertFalse(hasattr(mock, 'foo'))


def test_mock_raises_when_deleting_nonexistent_attribute(self):
for mock in (Mock(), MagicMock(), NonCallableMagicMock(),
NonCallableMock()):
del mock.foo
with self.assertRaises(AttributeError):
del mock.foo


def test_reset_mock_does_not_raise_on_attr_deletion(self):
# bpo-31177: reset_mock should not raise AttributeError when attributes
# were deleted in a mock instance
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2018-12-09-21-35-49.bpo-20239.V4mWBL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Allow repeated assignment deletion of :class:`unittest.mock.Mock` attributes.
Patch by Pablo Galindo.