Skip to content
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.

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

Patcher stop method should be idempotent #80547

Closed
sfreilich mannequin opened this issue Mar 19, 2019 · 7 comments
Closed

Patcher stop method should be idempotent #80547

sfreilich mannequin opened this issue Mar 19, 2019 · 7 comments
Labels
3.8 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@sfreilich
Copy link
Mannequin

sfreilich mannequin commented Mar 19, 2019

BPO 36366
Nosy @brettcannon, @cjw296, @voidspace, @matrixise, @mariocj89, @miss-islington, @tirkarthi, @sfreilich
PRs
  • bpo-36366: Return None on stopping unstarted patch object #12472
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-03-28.21:10:27.538>
    created_at = <Date 2019-03-19.15:57:41.523>
    labels = ['type-bug', '3.8']
    title = 'Patcher stop method should be idempotent'
    updated_at = <Date 2019-03-28.21:10:27.535>
    user = 'https://github.com/sfreilich'

    bugs.python.org fields:

    activity = <Date 2019-03-28.21:10:27.535>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-03-28.21:10:27.538>
    closer = 'brett.cannon'
    components = []
    creation = <Date 2019-03-19.15:57:41.523>
    creator = 'sfreilich'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36366
    keywords = ['patch']
    message_count = 7.0
    messages = ['338371', '338372', '338379', '338395', '338436', '339075', '339076']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'cjw296', 'michael.foord', 'matrixise', 'mariocj89', 'miss-islington', 'xtreak', 'sfreilich']
    pr_nums = ['12472']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue36366'
    versions = ['Python 3.8']

    @sfreilich
    Copy link
    Mannequin Author

    sfreilich mannequin commented Mar 19, 2019

    Currently, it's an error to call the stop() method of a patcher object created by mock.patch repeatedly:

    >>> patch = mock.patch.object(Foo, 'BAR', 'x')
    >>> patch.start()
    'x'
    >>> patch.stop()
    >>> patch.stop()
    RuntimeError: stop called on unstarted patcher

    This causes unnecessary problems when test classes using mock.patch.stopall for cleanup are mixed with those cleaning up patches individually:

    class TestA(unittest.TestCase):
    
      def setUp():
        patcher = mock.patch.object(...)
        self.mock_a = patcher.start()
        self.addCleanup(patcher.stop)

    ...

    class TestB(TestA):
    
      def setUp():
        super().setUp()
        self.addCleanup(mock.patch.stopall)
        self.mock_b = mock.patch.object(...).start()

    ...

    This fails because mock.patch.stopall stops the patch set up in TestA.setUp(), then that raises an exception when it's stopped again.

    But why does patcher.stop() enforce that precondition? Wouldn't it be sufficient for it to just enforce the postcondition, that after stop() is called the patch is stopped()? That would make it easier to write test code which makes use of mock.patch.stopall, which allows the proper cleanup of patches to be configured much more concisely.

    @matrixise
    Copy link
    Member

    @samuel,

    1. What's the version of Python?
    2. Could you provide a script with an example?

    Thank you

    @tirkarthi
    Copy link
    Member

    When mock.patch is creates a patch object and patch.start calls __enter__ that sets is_local. On stop __exit__ is called where a check is done is to make sure is_local attribute is present and then cleanup is done along with deleting calling del self.is_local so calling stop second time causes the attribute check to fail. There is no specific reason I could find with git history.

    It seems that calling patch.stop without patch.start makes cleanup to happen on unpatched objects and raises errors which I hope is avoided by always setting is_local on start and removing it on stop like a flag. That being said I am not sure why a early return couldn't be made when is_local is absent instead of proceeding with cleanup logic or raising a runtime error. I see no tests failing on early return except a test where RuntimeError is intentionally tested by calling stop on unstarted patch. I have added mock module devs for some context.

    A sample script would be as below :

    from unittest import mock
    
    class Foo:
        bar = None
    
    patch = mock.patch.object(Foo, 'bar', 'x')
    patch.start()
    patch.stop()
    patch.stop()

    @tirkarthi tirkarthi added 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Mar 19, 2019
    @voidspace
    Copy link
    Contributor

    It's almost certainly an oversight rather than a design decision. I'd be happy with the change you suggest Karthikeyan.

    @tirkarthi
    Copy link
    Member

    Thanks for the confirmation. I will raise a PR for this.

    @miss-islington
    Copy link
    Contributor

    New changeset 02b84cb by Miss Islington (bot) (Xtreak) in branch 'master':
    bpo-36366: Return None on stopping unstarted patch object (GH-12472)
    02b84cb

    @brettcannon
    Copy link
    Member

    Thanks, everyone!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants