Skip to content

bpo-31177: Fix reset_mock on mock with deleted children#10807

Closed
eli-b wants to merge 2 commits intopython:masterfrom
eli-b:master
Closed

bpo-31177: Fix reset_mock on mock with deleted children#10807
eli-b wants to merge 2 commits intopython:masterfrom
eli-b:master

Conversation

@eli-b
Copy link
Contributor

@eli-b eli-b commented Nov 30, 2018 •
edited by bedevere-bot
Loading

Currently, calling reset_mock() on a unittest.mock.Mock object after one of its attributes was deleted causes an exception.

This fixes the exception and introduces the behavior that the deletion of the attribute is reset, making it available again after the reset_mock() call.

https://bugs.python.org/issue31177

@tirkarthi tirkarthi mentioned this pull request Nov 30, 2018
Merged
vstinner
vstinner requested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

This change doesn't look correct to me. reset_mock() documentation says:
"The reset_mock method resets all the call attributes on a mock object"
https://docs.python.org/dev/library/unittest.mock.html#unittest.mock.Mock.reset_mock

I don't think that delete attributes are "call attributes".

cc @mariocj89

@bedevere-bot
Copy link

bedevere-bot commented Nov 30, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mariocj89
Copy link
Contributor

mariocj89 commented Nov 30, 2018

Absolutely agree with @vstinner

If the feature of resetting deleted attributes is really needed, that would be separate from fixing the current issue and I'd add it in an optional way, the same way return_value and side_effect reset was added.

Additionally, this will still break for delted attributes in childs isn't it?

@vstinner vstinner closed this Nov 30, 2018
@eli-b
Copy link
Contributor Author

eli-b commented Nov 30, 2018

Thanks for you comments. I'll make this PR just solve the immediate issue.
Since @tirkarthi raised the question of what the desired behavior is, and the current behavior was clearly a bug, I didn't think a deleted_children=False option was needed.

@eli-b
Copy link
Contributor Author

eli-b commented Nov 30, 2018

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Nov 30, 2018

Thanks for making the requested changes!

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

@eli-b
Copy link
Contributor Author

eli-b commented Nov 30, 2018

@vstinner, the changes I made are visible in my repo, but not here. I hoped using the "I've made the changes" message above would help them appear, but that doesn't seem to be the case.

@eli-b
Copy link
Contributor Author

eli-b commented Nov 30, 2018

@vstinner, I figured out my error. I have to open a new PR because I can't change the source branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

Assignees

No one assigned

Labels

awaiting change review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eli-b @bedevere-bot @mariocj89 @vstinner @the-knights-who-say-ni