bpo-21478: Record calls to parent when autospecced objects are used as child with attach_mock by tirkarthi · Pull Request #14688

bpo-21478: Record calls to parent when autospecced objects are used as child with attach_mock by tirkarthi · Pull Request #14688 · python/cpython

Conversation

When autospecced object is passed to attach_mock the mock object is stored in the mock attribute that has _mock_name and _mock_parent set. For function types passed check for the mock attribute and clear name and parent of it so that attach_mock updates the child with correct name and parent in _check_and_set_parent.

https://bugs.python.org/issue21478

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

Choose a reason for hiding this comment

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

	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.

Choose a reason for hiding this comment

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

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 create_autospec has returns the original object with mock stored in the mock attribute that needs to be used.

		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.

Choose a reason for hiding this comment

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

Not sure we need this comment, but the text could do with some tweaking.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This feels like it should be a separate test.

Choose a reason for hiding this comment

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

Moved it to a separate test. Thanks.


		parent = Mock()
		parent.attach_mock(create_autospec(foo, name='bar'), 'child')
		parent.child(1, 2)

Choose a reason for hiding this comment

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

Why are we calling child direct here?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

I moved it to _extract_mock to avoid code duplication and added a code comment related to autospecced functions.

Choose a reason for hiding this comment

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

Nice fix, looks quite nicer with the function extracted indeed.

		return obj.mock
		else:
		return obj

Choose a reason for hiding this comment

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

Nice!

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.
🐍🍒⛏🤖