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

mock 3.9 bug: Wrapped objects without __bool__ raise exception #84147

Closed
avylove mannequin opened this issue Mar 15, 2020 · 19 comments
Closed

mock 3.9 bug: Wrapped objects without __bool__ raise exception #84147

avylove mannequin opened this issue Mar 15, 2020 · 19 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@avylove
Copy link
Mannequin

avylove mannequin commented Mar 15, 2020

BPO 39966
Nosy @rbtcollins, @cjw296, @bitdancer, @voidspace, @serhiy-storchaka, @phmc, @lisroach, @tonybaloney, @mariocj89, @avylove, @tirkarthi, @amiremohamadi
PRs
  • bpo-39966: Fix wrapped objects exception mockunittest #19102
  • bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock (#16029)" #19734
  • 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 2020-04-29.04:18:01.259>
    created_at = <Date 2020-03-15.06:39:44.237>
    labels = ['type-bug', 'library', '3.9']
    title = 'mock 3.9 bug: Wrapped objects without __bool__ raise exception'
    updated_at = <Date 2020-04-29.04:18:01.258>
    user = 'https://github.com/avylove'

    bugs.python.org fields:

    activity = <Date 2020-04-29.04:18:01.258>
    actor = 'xtreak'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-04-29.04:18:01.259>
    closer = 'xtreak'
    components = ['Library (Lib)']
    creation = <Date 2020-03-15.06:39:44.237>
    creator = 'aviso'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39966
    keywords = ['patch', '3.9regression']
    message_count = 19.0
    messages = ['364222', '364223', '364663', '364728', '365144', '365198', '365199', '365200', '365201', '365202', '365203', '365204', '365211', '366999', '367402', '367423', '367426', '367558', '367624']
    nosy_count = 13.0
    nosy_names = ['rbcollins', 'cjw296', 'r.david.murray', 'michael.foord', 'serhiy.storchaka', 'pconnell', 'Darragh Bailey', 'lisroach', 'anthonypjshaw', 'mariocj89', 'aviso', 'xtreak', 'Amir']
    pr_nums = ['19102', '19734']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue39966'
    versions = ['Python 3.9']

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Mar 15, 2020

    This bug was introduced with bpo-25597

    Here's some code that demonstrates the error:

        import sys
        from unittest.mock import patch
    
        with patch.object(sys, 'stdout', wraps=sys.stdout) as mockstdout:
            bool(sys.stdout)

    This works fine in 3.8 and earlier, but fails in 3.9

    It seems the goal was to be able to access dunder methods for wrapped objects. Before this change __bool__ wasn't actually being checked, but was forced to True, which works for basic existence tests. The new code method._mock_wraps = getattr(mock._mock_wraps, name) has no fallthrough in case the attribute isn't there such as the case with __bool__ on sys.stdout.

    @avylove avylove mannequin added 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 15, 2020
    @tirkarthi
    Copy link
    Member

    Thanks for the report. There should be a check to ensure that the attribute is present to handle the attribute error while attaching the wrapped object's value. Something like below could be done so that we check for wrapped value or fallback to the old behaviour. This needs a test too where the report can be added as a unittest. The news entry can also ensure that the wrapped value will be used when present or falls back to the default value. Marking it as a 3.9 regression.

    diff --git Lib/unittest/mock.py Lib/unittest/mock.py
    index 20daf724c1..86e832cc51 100644
    --- Lib/unittest/mock.py
    +++ Lib/unittest/mock.py
    @@ -2036,7 +2036,7 @@ _side_effect_methods = {
     def _set_return_value(mock, method, name):
         # If _mock_wraps is present then attach it so that wrapped object
         # is used for return value is used when called.
    -    if mock._mock_wraps is not None:
    +    if mock._mock_wraps is not None and hasattr(mock._mock_wraps, name):
             method._mock_wraps = getattr(mock._mock_wraps, name)
             return

    @amiremohamadi
    Copy link
    Mannequin

    amiremohamadi mannequin commented Mar 20, 2020

    I'd like to work on it.
    Should I add tests to 'Lib/unittest/test/testmock/testmock.py'??

    @tirkarthi
    Copy link
    Member

    Amir, this needs a test like the initial report without preferably using sys.stdout and patch for cases below :

    1. Where the attribute is present on the wrapped object and uses it.
    2. Where the attribute is not present on the wrapped object and uses the default value of the magic method.

    @aviso Feel free to add in if I have missed any scenarios.

    @tirkarthi
    Copy link
    Member

    On thinking more about this the wraps argument provides a way to wrap the calls for the mock to a given object. So when an attribute not present in the wrapped object is being accessed then it should act like the attribute is not present. Falling back to the default value like previous case would help but that will pose the question over whether the value is from the wrapped object's attribute or from the default value for the magic method that the user could never know. This could be better clarified in the docs I suppose which I missed in the previous issue.

    Thoughts on this fallback behavior?

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Mar 28, 2020

    They are documented. That said, the subsection section could use a header.

    https://docs.python.org/3/library/unittest.mock.html#magic-mock

    Patch looks good! Can't think of any other test scenarios right now.

    @tirkarthi
    Copy link
    Member

    It's supposed to raise an AttributeError if the attribute is not present. The previous case was that magicmethods were not wrapped returning configured values. So this conflicts with the documentation returning a default value when not present instead of raising an AttributeError.

    wraps: Item for the mock object to wrap. If wraps is not None then calling the Mock will pass the call through to the wrapped object (returning the real result). Attribute access on the mock will return a Mock object that wraps the corresponding attribute of the wrapped object (so attempting to access an attribute that doesn’t exist will raise an AttributeError).

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Mar 28, 2020

    Ah, I see, yes, the documentation is a bit inconsistent. What may make more sense for some magic methods is to call the underlying object and use the return value or raise any exception that occurs.
    For example:
    __bool__ return value is bool(mock._mock_wraps)
    __int__ return value is int(mock._mock_wraps)
    __lt__ return value is operator.lt(mock._mock_wraps, other)

    This would take care of several of them and provide more consistent behavior with the wrapped object.

    @tirkarthi
    Copy link
    Member

    What may make more sense for some magic methods is to call the underlying object and use the return value or raise any exception that occurs.

    Sorry, isn't that what the previous issue did? It tries to return the wrapped object attribute and raises exception if attribute is not present. Using wrapped object attribute and falling back to default value means that it will always return a value but the user can't figure out if it's from wrapped object or the default value for the magic method.

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Mar 28, 2020

    That is not necessarily the same thing, hence why there was a bug.

    >>> hasattr(object, '__bool__')
    False
    >>> bool(object)
    True

    I think you may be right that magic methods shouldn't magically appear for wrapped objects, except those that do magically appear, like __bool__.

    @tirkarthi
    Copy link
    Member

    Thanks for the example I didn't know hasattr can return False to return a value when the magicmethod was accessed. I will wait for others opinion on this then.

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Mar 28, 2020

    Honestly, I'm not sure if any of the other magic methods behave this way. It would take a little research or someone more familiar with it.

    Here's more info about how __bool__ behaves.
    https://docs.python.org/3/reference/datamodel.html#object.\_\_bool__

    @bitdancer
    Copy link
    Member

    My guess is that it isn't so much that __bool__ is special, as that the evaluation of values in a boolean context is special. What you have to do to make a mock behave "correctly" in the face that I'm not sure (I haven't investigated). And I might be wrong.

    @tirkarthi
    Copy link
    Member

    The patch assumed using the magic method attribute as the way to evaluate an object in a context which I got to know is wrong since evaluations in context like boolean are not only dependent on one magic method but has a precedence over several others. In the event of 3.9 alpha 6 being the next candidate I guess the change can be reverted to be revisited later about the desired behaviour with backwards compatibility. The docs could perhaps be clarified that calling magic method on a wrap gives default set of values.

    Thoughts on reversion or other possible approaches?

    @cjw296
    Copy link
    Contributor

    cjw296 commented Apr 27, 2020

    I'd go with your instincts on what to do next, I'd have a slight preference to keeping behaviour the same as it was in 3.8 if the changes for 3.9 cause more problems. That leaves us no worse off than we were before, and with the opportunity to improve from the current position in a future release.

    @avylove
    Copy link
    Mannequin Author

    avylove mannequin commented Apr 27, 2020

    Checking for presence and then falling through to the <=3.8 behavior as in the patch, seems reasonable. The default magic method behavior presumably comes out of research, trial, and error. If the docs need to be clearer, I think that's different that scrapping the whole approach.

    @tirkarthi
    Copy link
    Member

    I opened a PR to revert the change. bpo-25597 was open for sometime and the implications as reported here seem to be greater than the original report to just call the magicmethod. So we can revert the change to ensure there are no regressions in Python 3.9 for first beta and discuss the behavior for a later release.

    @cjw296
    Copy link
    Contributor

    cjw296 commented Apr 28, 2020

    New changeset 521c8d6 by Karthikeyan Singaravelan in branch 'master':
    bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock" (GH-19734)
    521c8d6

    @tirkarthi
    Copy link
    Member

    Thanks Avram for the report. I have reopened bpo-25597. Closing this as the regression change has been reverted for 3.9.

    @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.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants