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

Incorrect error handling in unittest.mock #84307

Open
bpmcl mannequin opened this issue Mar 31, 2020 · 8 comments
Open

Incorrect error handling in unittest.mock #84307

bpmcl mannequin opened this issue Mar 31, 2020 · 8 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@bpmcl
Copy link
Mannequin

bpmcl mannequin commented Mar 31, 2020

BPO 40126
Nosy @ncoghlan, @cjw296, @voidspace, @serhiy-storchaka, @tirkarthi, @bpmcl
PRs
  • [3.7] bpo-40126: Fix attribute out of scope during error handling in mock #19338
  • bpo-40126: Fix reverting multiple patches in unittest.mock. #19351
  • [3.8] bpo-40126: Fix reverting multiple patches in unittest.mock. (GH-19351) #19483
  • [3.7] bpo-40126: Fix reverting multiple patches in unittest.mock. (GH-19351) #19484
  • 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 = None
    created_at = <Date 2020-03-31.16:07:24.415>
    labels = ['3.7', '3.8', 'type-bug', 'library', '3.9']
    title = 'Incorrect error handling in unittest.mock'
    updated_at = <Date 2020-04-12.11:57:36.107>
    user = 'https://github.com/bpmcl'

    bugs.python.org fields:

    activity = <Date 2020-04-12.11:57:36.107>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2020-03-31.16:07:24.415>
    creator = 'bmclarnon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40126
    keywords = ['patch']
    message_count = 8.0
    messages = ['365395', '365707', '365728', '366195', '366222', '366238', '366239', '366241']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'cjw296', 'michael.foord', 'serhiy.storchaka', 'xtreak', 'bmclarnon']
    pr_nums = ['19338', '19351', '19483', '19484']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue40126'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @bpmcl
    Copy link
    Mannequin Author

    bpmcl mannequin commented Mar 31, 2020

    The error handling in mock.decorate_callable (3.5-3.7) and mock.decoration_helper (3.8-3.9) is incorrectly implemented. If the error handler is triggered in the loop, the patching variable is out of scope and raises an unhandled UnboundLocalError instead.

    This happened as a result of a 3rd-party library that attempts to clear the patchings list of a decorated function. The below code shows a recreation of the incorrect error handling:

    import functools
    from unittest import mock
    
    
    def is_valid():
        return True
    
    
    def mock_is_valid():
        return False
    
    
    def decorate(f):
        @functools.wraps(f)
        def decorate_wrapper(*args, **kwargs):
            # This happens in a 3rd-party library
            f.patchings = []
            return f(*args, **kwargs)
    
        return decorate_wrapper
    
    
    @decorate
    @mock.patch('test.is_valid', new=mock_is_valid)
    def test_patch():
        raise Exception()

    @bpmcl bpmcl mannequin added 3.8 only security fixes 3.9 only security fixes tests Tests in the Lib/test dir 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Mar 31, 2020
    @bpmcl
    Copy link
    Mannequin Author

    bpmcl mannequin commented Apr 3, 2020

    After further investigation, it seems this was fixed in 436c2b0

    However, this fix was as part of an unrelated changeset and in a different function in 3.8+, and was never rolled back to 3.7 and below. PR opened to add the missing attribute instantiation to 3.7.

    @bpmcl bpmcl mannequin added stdlib Python modules in the Lib dir and removed 3.8 only security fixes 3.9 only security fixes tests Tests in the Lib/test dir labels Apr 3, 2020
    @serhiy-storchaka
    Copy link
    Member

    I think that the current code is not correct. __exit__ should not be called if __enter__ is failed. If some __enter__ implementation calls other __enter__s it should manually call corresponding __exit__s.

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes 3.9 only security fixes labels Apr 3, 2020
    @serhiy-storchaka
    Copy link
    Member

    New changeset 4b222c9 by Serhiy Storchaka in branch 'master':
    bpo-40126: Fix reverting multiple patches in unittest.mock. (GH-19351)
    4b222c9

    @bpmcl
    Copy link
    Mannequin Author

    bpmcl mannequin commented Apr 11, 2020

    Issue still exists in 3.7 and below, as it was part of a different function before. Current PR doesn't resolve the original issue that was raised.

    @serhiy-storchaka
    Copy link
    Member

    New changeset ee249d7 by Serhiy Storchaka in branch '3.8':
    [3.8] bpo-40126: Fix reverting multiple patches in unittest.mock. (GH-19351) (GH-19483)
    ee249d7

    @serhiy-storchaka
    Copy link
    Member

    New changeset 4057e8f by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-40126: Fix reverting multiple patches in unittest.mock. (GH-19351) (GH-19484)
    4057e8f

    @serhiy-storchaka
    Copy link
    Member

    3.5 and 3.6 are in security fixes only mode, and this issue is not a security issue. After merging PR 19484 I cannot reproduce the original issue anymore.

    I left this issue open for the case if I find a way to write tests for the merged changes.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    1 participant