bpo-35512: Resolve string target to patch.dict decorator during function call by tirkarthi · Pull Request #12000 · python/cpython

bpo-35512: Resolve string target to patch.dict decorator during function call by tirkarthi · Pull Request #12000 · python/cpython

String target provided to patch.dict can be reassigned after the decorator decorates the function. So resolve the target before function call to ensure the new reference is patched instead of patching the old reference stored in the constructor.

https://bugs.python.org/issue35512

@mariocj89 I have just moved the resolution to _patch_dict since I felt storing the target in in_dict_name is little misleading since it's not always a name. Also that using a separate variable for only string target felt little verbose to me since it's essentially the same thing.

For tests I have used testmock.support to have a dictionary that I am trying to patch as a string target in testpatch.py which patches the older reference without the PR. Let me know if this could be made better and comments/NEWS need to be reworded.

Thanks

Choose a reason for hiding this comment

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

I think it is a little bit weird that self.in_dict changes when executing the function/ctx manager but given that we are using it just as a way to make it available across functions LGTM.

Thanks for the review Mario. I would wait for feedback from @jaraco and @cjw296 to see if this can be merged.

		self.assertEqual(support.target['bar'], 'BAR')

		support.target = {'foo': 'BAZ'}
		test()

Choose a reason for hiding this comment

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

I'd expect this test() call to be wrapped in a try/finally to put support.target back like it was, regardless of what happens in test.

Choose a reason for hiding this comment

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

Yes, tests might use the variable in the future. Good catch. Thanks.

		test()

		self.assertEqual(support.target['foo'], 'BAZ')
		self.assertNotIn('bar', support.target)

Choose a reason for hiding this comment

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

Why not just:
self.assertEqual(support.target, {'foo': 'BAZ'})
?
(I guess the same goes for the assertions inside test too?)


		def __init__(self, in_dict, values=(), clear=False, **kwargs):
		if isinstance(in_dict, str):
		in_dict = _importer(in_dict)

Choose a reason for hiding this comment

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

Is the setting of self.in_dict on the line below still needed?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

LGTM!


		try:
		support.target = {'foo': 'BAZ'}
		test()

Choose a reason for hiding this comment

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

You could always add the following here, so be super sure:
self.assertEqual(support.target, {'foo': 'BAZ'})

Choose a reason for hiding this comment

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

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