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.patch.stopall doesn't work with patch.dict #65799

Closed
kakuma mannequin opened this issue May 29, 2014 · 22 comments
Closed

mock.patch.stopall doesn't work with patch.dict #65799

kakuma mannequin opened this issue May 29, 2014 · 22 comments
Assignees
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@kakuma
Copy link
Mannequin

kakuma mannequin commented May 29, 2014

BPO 21600
Nosy @cjw296, @voidspace, @ericsnowcurrently, @kushaldas, @lisroach, @mariocj89, @tirkarthi
PRs
  • bpo-21600: Fix mock.patch.dict to be stopped with mock.patch.stopall #17606
  • Files
  • add_stopall_patch_dict.patch: a patch to support patch.dict by patch.stopall.
  • support_patch_dict_by_stopall.diff
  • support_patch_dict_by_stopall.diff
  • 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 = 'https://github.com/voidspace'
    closed_at = <Date 2020-01-24.08:41:44.508>
    created_at = <Date 2014-05-29.03:35:28.462>
    labels = ['type-bug', 'library', '3.9']
    title = "mock.patch.stopall doesn't work with patch.dict"
    updated_at = <Date 2020-01-24.08:41:44.507>
    user = 'https://bugs.python.org/kakuma'

    bugs.python.org fields:

    activity = <Date 2020-01-24.08:41:44.507>
    actor = 'cjw296'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2020-01-24.08:41:44.508>
    closer = 'cjw296'
    components = ['Library (Lib)']
    creation = <Date 2014-05-29.03:35:28.462>
    creator = 'kakuma'
    dependencies = []
    files = ['35445', '35499', '35513']
    hgrepos = []
    issue_num = 21600
    keywords = ['patch']
    message_count = 22.0
    messages = ['219331', '219567', '219697', '219885', '219887', '219951', '220030', '220082', '229291', '229292', '229294', '232411', '232435', '232440', '232458', '232467', '232480', '298203', '322016', '358323', '358324', '358374']
    nosy_count = 10.0
    nosy_names = ['cjw296', 'michael.foord', 'eric.snow', 'kushal.das', 'kakuma', 'lwcolton', 'lisroach', 'mariocj89', 'Antoni Szych', 'xtreak']
    pr_nums = ['17606']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21600'
    versions = ['Python 3.9']

    @kakuma
    Copy link
    Mannequin Author

    kakuma mannequin commented May 29, 2014

    It seems that stopall doesn't work when do start patch.dict to sys.modules.
    I show sample scripts the following.

    Using stopall test case seems to always refer the first mock foo object.
    But using stop it refers a new mock foo object.

    $ cat test_sampmod.py
    import foo
    def myfunc():
        print "myfunc foo=%s" % foo
        return foo
    $ cat test_samp.py
    import mock
    import sys
    import unittest
    
    
    class SampTestCase(unittest.TestCase):
        def setUp(self):
            self.foo_mod = mock.Mock()
            self.m = mock.patch.dict('sys.modules', {'foo': self.foo_mod})
            self.p = self.m.start()
            print "foo_mod=%s" % self.foo_mod
            __import__('test_sampmod')
            self.test_sampmod = sys.modules['test_sampmod']
    
        def tearDown(self):
            if len(sys.argv) > 1:
                self.m.stop()
                print ">>> stop patch"
            else:
                mock.patch.stopall()
                print ">>> stopall patch"
    
        def test_samp1(self):
            self.assertEqual(self.foo_mod, self.test_sampmod.myfunc())
    
        def test_samp2(self):
            self.assertEqual(self.foo_mod, self.test_sampmod.myfunc())
    
        def test_samp3(self):
            self.assertEqual(self.foo_mod, self.test_sampmod.myfunc())
    
    if __name__ == '__main__':
        suite = unittest.TestSuite()
        suite.addTest(unittest.TestLoader().loadTestsFromTestCase(SampTestCase))
        unittest.TextTestRunner(verbosity=2).run(suite)
    $ python test_samp.py stop
    test_samp1 (__main__.SampTestCase) ... foo_mod=<Mock id='41504336'>
    myfunc foo=<Mock id='41504336'>
    >>> stop patch
    ok
    test_samp2 (__main__.SampTestCase) ... foo_mod=<Mock id='41504464'>
    myfunc foo=<Mock id='41504464'>
    >>> stop patch
    ok
    test_samp3 (__main__.SampTestCase) ... foo_mod=<Mock id='41504720'>
    myfunc foo=<Mock id='41504720'>
    >>> stop patch
    ok

    Ran 3 tests in 0.004s

    OK
    $ python test_samp.py
    test_samp1 (__main__.SampTestCase) ... foo_mod=<Mock id='19152464'>
    myfunc foo=<Mock id='19152464'>
    >>> stopall patch
    ok
    test_samp2 (__main__.SampTestCase) ... foo_mod=<Mock id='19152592'>
    myfunc foo=<Mock id='19152464'>
    FAIL
    >>> stopall patch
    test_samp3 (__main__.SampTestCase) ... foo_mod=<Mock id='19182096'>
    myfunc foo=<Mock id='19152464'>
    FAIL
    >>> stopall patch

    ======================================================================
    FAIL: test_samp2 (main.SampTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "test_samp.py", line 27, in test_samp2
        self.assertEqual(self.foo_mod, self.test_sampmod.myfunc())
    AssertionError: <Mock id='19152592'> != <Mock id='19152464'>

    ======================================================================
    FAIL: test_samp3 (main.SampTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "test_samp.py", line 30, in test_samp3
        self.assertEqual(self.foo_mod, self.test_sampmod.myfunc())
    AssertionError: <Mock id='19182096'> != <Mock id='19152464'>

    Ran 3 tests in 0.003s

    FAILED (failures=2)
    $

    @kakuma kakuma mannequin added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels May 29, 2014
    @kakuma
    Copy link
    Mannequin Author

    kakuma mannequin commented Jun 2, 2014

    Checking mock.py(version 1.0.1) it seems that patch.stopall does not support patch.dict. Does it have any problem to support ptch.dict by stopall.
    I made the attached patch file for this. It seems to work well. How about this?

    But I don't know why sys.modules refers the first mock object.

    $ python test_samp.py
    test_samp1 (__main__.SampTestCase) ... foo_mod=<Mock id='140164117280080'>
    myfunc foo=<Mock id='140164117280080'>
    >>> stopall patch
    ok
    test_samp2 (__main__.SampTestCase) ... foo_mod=<Mock id='140164117280208'>
    myfunc foo=<Mock id='140164117280208'>
    >>> stopall patch
    ok
    test_samp3 (__main__.SampTestCase) ... foo_mod=<Mock id='140164117280464'>
    myfunc foo=<Mock id='140164117280464'>
    >>> stopall patch
    ok

    Ran 3 tests in 0.001s

    OK
    $

    @voidspace
    Copy link
    Contributor

    Yep, patch.dict wasn't designed with stopall in mind so it needs adding. Thanks for pointing this out and your fix.

    Your patch isn't quite right, those operations shouldn't be inside the try excepts. (And there are no tests.)

    @voidspace voidspace self-assigned this Jun 3, 2014
    @kakuma
    Copy link
    Mannequin Author

    kakuma mannequin commented Jun 6, 2014

    Thank you for your reply.
    Yes, you are right. The patch was too slapdash. I re-created it and added unit tests.

    @voidspace
    Copy link
    Contributor

    That's better - thanks. Another minor tweak needed though. stopall should only stop patches that were started with "start", not those used as context managers or decorators (or they will be stopped twice!).

    See how the main patch object only adds to the set of active patches in the start method, not in __enter__ (and removes in stop rather than __exit__).

    @kakuma
    Copy link
    Mannequin Author

    kakuma mannequin commented Jun 7, 2014

    Hi michael,
    Certainly, thank you for your many advices. I attached the new patch file.

    @voidspace
    Copy link
    Contributor

    That looks great - thanks! I'll get it committed shortly.

    @kakuma
    Copy link
    Mannequin Author

    kakuma mannequin commented Jun 9, 2014

    Thank you in advance.

    @lwcolton
    Copy link
    Mannequin

    lwcolton mannequin commented Oct 14, 2014

    What's the status on this?

    @voidspace
    Copy link
    Contributor

    It just needs committing, I believe Kushal Das has volunteered to do it.

    @kushaldas
    Copy link
    Member

    I will do that. New job is taking time.

    @ericsnowcurrently
    Copy link
    Member

    FYI, you probably don't want to be patching out sys.modules. It isn't guaranteed that doing so will have the expected effect on import semantics.

    @voidspace
    Copy link
    Contributor

    why not?

    @lwcolton
    Copy link
    Mannequin

    lwcolton mannequin commented Dec 10, 2014

    Patching sys.modules is an idea I got from the official documentation

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

    http://mock.readthedocs.org/en/latest/patch.html#patch-dict

    @ericsnowcurrently
    Copy link
    Member

    At least for CPython sys.modules is initially set to the modules dict on the interpreter struct. As of 3.4 the import system itself only cares about sys.modules (I'll have to double check on builtin___import__). However, if I recall correctly at least part of the import C-API interacts directly with the original interpreter copy of the modules dict.

    The catch is that setting sys.modules to something else does not also change the dict on the interpreter struct. So they will be out of sync. This may cause problems. In practice I don't think it's a big deal, but there is no guarantee that patching out sys.modules will give you the behavior that you expect.

    See also issue bpo-12633.

    @voidspace
    Copy link
    Contributor

    Using patch.dict manipulates the contents of sys.modules, it doesn't replace sys.modules.

    @ericsnowcurrently
    Copy link
    Member

    Ah. Never mind then. :)

    @AntoniSzych
    Copy link
    Mannequin

    AntoniSzych mannequin commented Jul 12, 2017

    Hi,

    It's been 3 years now since this issue was first raised.
    We bumped upon this issue while using code like following:

    def tearDown():
        patch.stopall()
    
    def test123():
        p=patch.dict(...)
        p.start()
        assert False
        p.stop()

    While patch.stopall() is run, it doesn't stop anything. This is because p.start() in fact executes mock._patch_dict._patch_dict(), which does not execute self._active_patches.append(self) (like ordinary p=patch(...).start() would).

    I could understand that this is just a design choice (which may seem unintuitive for me, but possibly perfectly good for others), however the official documentation [1] states:

    All the patchers have start() and stop() methods.

    And few lines below [2], we have:

    patch.stopall() Stop all active patches. Only stops patches started with start.

    The above is not true for patch.dict(), so the documentation is unfortunatelly misleading.

    Is there a possibility to fix this in some 3.5 maintenance release (preferably code, not docs :) )?

    If anyone else will have the same issue: for now a workaround would be to use patch.dict() either as a decorator, or as a context manager (with patch.dict()).

    [1] https://docs.python.org/3/library/unittest.mock.html#patch-methods-start-and-stop
    [2] https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch.stopall

    @AntoniSzych
    Copy link
    Mannequin

    AntoniSzych mannequin commented Jul 20, 2018

    Hi,

    Any chance of getting this resolved 1 year later? :)

    BR

    @pppery pppery mannequin changed the title mock.patch.stopall doesn't work with patch.dict to sys.modules mock.patch.stopall doesn't work with patch.dict Jul 21, 2018
    @tirkarthi
    Copy link
    Member

    This sounds like a good idea given the docs and the general agreement in the thread. I would like to target this for 3.9 . Differentiation between the normal patches and the ones from patch.dict is nice but given that docstring indicates LIFO for all active patches storing patch.dict items also in _patch.active_patches ensures we can do LIFO if there is a mixture of normal and dictionary patches started with start method. Adding other maintainers for thoughts.

    diff --git Lib/unittest/mock.py Lib/unittest/mock.py
    index cd5a2aeb60..96115e06ba 100644
    --- Lib/unittest/mock.py
    +++ Lib/unittest/mock.py
    @@ -1853,8 +1853,21 @@ class _patch_dict(object):
    self._unpatch_dict()
    return False

    -    start = __enter__
    -    stop = __exit__
    +    def start(self):
    +        """Activate a patch, returning any created mock."""
    +        result = self.__enter__()
    +        _patch._active_patches.append(self)
    +        return result
    +
    +    def stop(self):
    +        """Stop an active patch."""
    +        try:
    +            _patch._active_patches.remove(self)
    +        except ValueError:
    +            # If the patch hasn't been started this will fail
    +            pass
    +
    +        return self.__exit__()

    @tirkarthi tirkarthi added stdlib Python modules in the Lib dir 3.9 only security fixes and removed tests Tests in the Lib/test dir labels Dec 13, 2019
    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Dec 13, 2019

    Makes total sense, I think we should get this for 3.9.
    Not sure I'll backport this, even if a bugfix it might cause unexpected changes (Though I'd be OK with it, given that calling stop twice causes no issue).

    I'm happy to push a PR with the proposed change and some tests if you want @XTreak. This is quite a simple fix we can get through in a short time.

    @tirkarthi
    Copy link
    Member

    Thanks Mario. I think the last patch attached to the issue is complete with tests and needs to be updated master. Feel free to raise a PR.

    @cjw296 cjw296 closed this as completed Jan 24, 2020
    @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

    5 participants