bpo-21478: Record calls to parent when autospecced objects are used as child with attach_mock#14688
Conversation
|
/cc @voidspace |
| with mock.patch(f'{__name__}.something', autospec=True) as mock_func: | ||
| self.assertEqual(mock_func.mock._extract_mock_name(), 'something') | ||
| parent.attach_mock(mock_func, 'child') | ||
| parent.child() |
There was a problem hiding this comment.
| parent.child() | |
| something() |
In my use case for attaching the child mock to the parent, I call the child mock directly instead of through the parent mock. The only reason I attach the child to a parent is so that I can check the order of calls to different functions, à la https://stackoverflow.com/a/27306920/2407644.
There was a problem hiding this comment.
Thanks @iforapsy for the review. I have changed the test to ensure call as a child, standalone function call and mock object call are all recorded to both parent and the respective mock object so that all cases are covered.
|
@cjw296 @mariocj89 Would be helpful to have a review of this. This is similar to #11273 where |
| def test_attach_mock_patch_autospec(self): | ||
| # bpo-21478: attach_mock used with autospecced object should have | ||
| # should use child's name and record calls to parent when being called | ||
| # as child and also as standalone functions. |
There was a problem hiding this comment.
Not sure we need this comment, but the text could do with some tweaking.
There was a problem hiding this comment.
I removed it since I feel bug report gives the relevant context. Feel free to suggest wording if I need to incorporate a code comment. Since I am not a native speaker sometimes my language might be little difficult to grasp.
| self.assertEqual(mock.mock_calls, [call.child(1, 2)]) | ||
| self.assertIn('mock.child', repr(mock.child.mock)) | ||
|
|
||
| parent = Mock() |
There was a problem hiding this comment.
This feels like it should be a separate test.
There was a problem hiding this comment.
Moved it to a separate test. Thanks.
|
|
||
| parent = Mock() | ||
| parent.attach_mock(create_autospec(foo, name='bar'), 'child') | ||
| parent.child(1, 2) |
There was a problem hiding this comment.
Why are we calling child direct here?
There was a problem hiding this comment.
Sorry, I don't get the comment here. The foo function is attached as a child attribute so I am calling parent.child to ensure parent records the call. I am not getting the direct call part here.
There was a problem hiding this comment.
I guess I don't really understand the use case for attach_mock so happy to go with what you say.
| # When autospecced object is passed to attach_mock then clear | ||
| # name and parent of the mock attribute which holds the actual | ||
| # mock object. | ||
| if isinstance(mock, FunctionTypes) and hasattr(mock, 'mock'): |
There was a problem hiding this comment.
If this is the way that we know an autospecced object is passed in, it feels like it should be abstracted out into a helper function with a sensible name, and used wherever else this pattern has cropped up.
There was a problem hiding this comment.
I moved it to _extract_mock to avoid code duplication and added a code comment related to autospecced functions.
mariocj89
left a comment
mariocj89
left a comment
There was a problem hiding this comment.
Nice fix, looks quite nicer with the function extracted indeed.
| return obj.mock | ||
| else: | ||
| return obj | ||
|
|
|
Thanks @tirkarthi for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Thanks @tirkarthi for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
8 participants
When autospecced object is passed to
attach_mockthe mock object is stored in themockattribute that has_mock_nameand_mock_parentset. For function types passed check for the mock attribute and clear name and parent of it so thatattach_mockupdates the child with correct name and parent in_check_and_set_parent.https://bugs.python.org/issue21478