8000 gh-104745: Limit starting a patcher more than once without stopping it by Red4Ru · Pull Request #126649 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-104745: Limit starting a patcher more than once without stopping it #126649

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
merged 11 commits into from
Nov 13, 2024
Merged

gh-104745: Limit starting a patcher more than once without stopping it #126649

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f78011f
Limit starting a patcher more than once without stopping it
Red4Ru Nov 10, 2024
1b9c6d4
Update Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.z…
Red4Ru Nov 10, 2024
399bb66
Refactor patch context
Red4Ru Nov 10, 2024
e28c3fb
Add property setters (untested)
Red4Ru Nov 10, 2024
a07d3b5
Avoid re-call of self.getter
Red4Ru Nov 10, 2024
3c4edf2
Preserve only _patch.temp_original instead of _patch.original
Red4Ru Nov 10, 2024
916ac69
Change NEWS file
Red4Ru Nov 10, 2024
862d5a0
Fix of property setters along with a test for them
Red4Ru Nov 10, 2024
7719894
Refactor _PatchContext from namedtuple to plain class with __slots__
Red4Ru Nov 11, 2024
1180b63
Minimize changes in mock.py
Red4Ru Nov 12, 2024
7d73933
Leave as little changes in mock.py as possible
Red4Ru Nov 12, 2024
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
33 changes: 31 additions & 2 deletions Lib/test/test_unittest/testmock/testpatch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,35 @@ def test_stop_idempotent(self):
self.assertIsNone(patcher.stop())


def test_exit_idempotent(self):
patcher = patch(foo_name, 'bar', 3)
with patcher:
patcher.stop()


def test_second_start_failure(self):
patcher = patch(foo_name, 'bar', 3)
patcher.start()
try:
self.assertRaises(RuntimeError, patcher.start)
finally:
patcher.stop()


def test_second_enter_failure(self):
patcher = patch(foo_name, 'bar', 3)
with patcher:
self.assertRaises(RuntimeError, patcher.start)


def test_second_start_after_stop(self):
patcher = patch(foo_name, 'bar', 3)
patcher.start()
patcher.stop()
patcher.start()
patcher.stop()


def test_patchobject_start_stop(self):
original = something
patcher = patch.object(PTModule, 'something', 'foo')
Expand Down Expand Up @@ -1098,7 +1127,7 @@ def test_new_callable_patch(self):

self.assertIsNot(m1, m2)
for mock in m1, m2:
self.assertNotCallable(m1)
self.assertNotCallable(mock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice spot!



def test_new_callable_patch_object(self):
Expand All @@ -1111,7 +1140,7 @@ def test_new_callable_patch_object(self):

self.assertIsNot(m1, m2)
for mock in m1, m2:
self.assertNotCallable(m1)
self.assertNotCallable(mock)


def test_new_callable_keyword_arguments(self):
Expand Down
60 changes: 45 additions & 15 deletions Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,6 +1320,14 @@ def _check_spec_arg_typos(kwargs_to_check):
)


class _PatchStartedContext(object):
Copy link
Member
@picnixz picnixz Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To reduce the memory footprint, how about using a namedtuple? I would also call it only class _PatchContext because it does not really give more information to have the "Started" IMO.

def __init__(self, exit_stack, is_local, target, temp_original):
self.exit_stack = exit_stack
self.is_local = is_local
self.target = target
self.temp_original = temp_original


class _patch(object):

attribute_name = None
Expand Down Expand Up @@ -1360,6 +1368,7 @@ def __init__(
self.autospec = autospec
self.kwargs = kwargs
self.additional_patchers = []
self._started_context = None


def copy(self):
Expand Down Expand Up @@ -1469,13 +1478,31 @@ def get_original(self):
)
return original, local

@property
def is_started(self):
Copy link
Member

Choose a reason for hiding this comment

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

Previously, those were writable attributes and now it's no more the case. Could there be some code in the wild assuming so? (for instance pytest which makes quite hacky things, though I don't know if they do hacky things with this specific part of CPython).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so... I have committed property setters just in case. Will write tests if needed when we figure out what to do with temp_original: do we preserve it and somehow deprecate or anything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed them. A bit ugly but anyway these setters exist not for an intended usecase but for backwards compatibility only

return self._started_context is not None

@property
def is_local(self):
return self._started_context.is_local

@property
def target(self):
return self._started_context.target

@property
def temp_original(self):
return self._started_context.temp_original

def __enter__(self):
"""Perform the patch."""
if self.is_started:
raise RuntimeError("Patch is already started")

new, spec, spec_set = self.new, self.spec, self.spec_set
autospec, kwargs = self.autospec, self.kwargs
new_callable = self.new_callable
self.target = self.getter()
target = self.getter()

# normalise False to None
if spec is False:
Expand All @@ -1491,7 +1518,7 @@ def __enter__(self):
spec_set not in (True, None)):
raise TypeError("Can't provide explicit spec_set *and* spec or autospec")

original, local = self.get_original()
original, is_local = self.get_original()

if new is DEFAULT and autospec is None:
inherit = False
Expand Down Expand Up @@ -1579,17 +1606,17 @@ def __enter__(self):
if autospec is True:
autospec = original

if _is_instance_mock(self.target):
if _is_instance_mock(target):
raise InvalidSpecError(
f'Cannot autospec attr {self.attribute!r} as the patch '
f'target has already been mocked out. '
f'[target={self.target!r}, attr={autospec!r}]')
f'[target={target!r}, attr={autospec!r}]')
if _is_instance_mock(autospec):
target_name = getattr(self.target, '__name__', self.target)
target_name = getattr(target, '__name__', target)
raise InvalidSpecError(
f'Cannot autospec attr {self.attribute!r} from target '
f'{target_name!r} as it has already been mocked out. '
f'[target={self.target!r}, attr={autospec!r}]')
f'[target={target!r}, attr={autospec!r}]')

new = create_autospec(autospec, spec_set=spec_set,
_name=self.attribute, **kwargs)
Expand All @@ -1600,17 +1627,20 @@ def __enter__(self):

new_attr = new

self.temp_original = original
self.is_local = local
self._exit_stack = contextlib.ExitStack()
self._started_context = _PatchStartedContext(
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming self._context would be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._started_context = _PatchStartedContext(
exit_stack = contextlib.ExitStack()
self._started_context = _PatchStartedContext(

exit_stack=contextlib.ExitStack(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exit_stack=contextlib.ExitStack(),
exit_stack=exit_stack,

is_local=is_local,
target=self.getter(),
temp_original=original,
Copy link
Member

Choose a reason for hiding this comment

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

This attribute can be called original since we are anyway wrapping it in a new object.

)
try:
setattr(self.target, self.attribute, new_attr)
if self.attribute_name is not None:
extra_args = {}
if self.new is DEFAULT:
extra_args[self.attribute_name] = new
for patching in self.additional_patchers:
arg = self._exit_stack.enter_context(patching)
arg = self._started_context.exit_stack.enter_context(patching)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
arg = self._started_context.exit_stack.enter_context(patching)
arg = exit_stack.enter_context(patching)

if patching.new is DEFAULT:
extra_args.update(arg)
return extra_args
Expand All @@ -1622,6 +1652,9 @@ def __enter__(self):

def __exit__(self, *exc_info):
"""Undo the patch."""
if not self.is_started:
return

if self.is_local and self.temp_original is not DEFAULT:
setattr(self.target, self.attribute, self.temp_original)
else:
Expand All @@ -1633,11 +1666,8 @@ def __exit__(self, *exc_info):
# needed for proxy objects like django settings
setattr(self.target, self.attribute, self.temp_original)

del self.temp_original
del self.is_local
del self.target
exit_stack = self._exit_stack
del self._exit_stack
exit_stack = self._started_context.exit_stack
self._started_context = None
return exit_stack.__exit__(*exc_info)


Expand Down
5 changes: 5 additions & 0 deletions Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.zAa5Ke.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Limit starting a patcher (from :func:`unittest.mock.patch`,
:func:`unittest.mock.patch.object` or :func:`unittest.mock.dict`) more than
once without stopping it. Previously, this would cause an
:exc:`AttributeError` if the patch stopped more than once after this, and
would also disrupt the original patched object.
Loading
0