search

derwiki-adroll / mock

Automatically exported from code.google.com/p/mock
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

don't stop a patcher when the test function returns, if it returned a Twisted Deferred #96

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Write a test function to test some asynchronous code in Twisted, such that 
your test function returns a Deferred, and once the test is finished then that 
Deferred gets fired.
2. Use a mock.patch() to inject some code for this test.
3. Run "trial" to execute the test.

What is the expected output? What do you see instead?

I expect the patch to stay in place until the test is finished. Instead, the 
patch is stopped when the test function returns.

What version of the product are you using? On what operating system?

Mock 0.7.2 on Linux

Please provide any additional information below.

Attached is some code that I wrote which overrides some of the guts of mock.py 
to make it do the right thing for functions that return Deferreds (as well as 
continuing to do the right thing for functions that return values of another 
type). This code actually works and makes my current unit test do the right 
thing, but it is potentially a bit too weird of a thing to add to the 
Tahoe-LAFS source code tree. For one thing it could easily break when new 
versions of mock are used with Tahoe-LAFS. Would you consider adopting this 
patch into mock proper?

Here is the salient diff that would make mock leave a patch in place until a 
Deferred is finished:

--- mock-0.7.2/mock.py  2011-06-16 23:57:22.207717503 -0600
+++ mock-0.7.2-zookopatch/mock.py       2011-06-16 23:59:04.128717498 -0600
@@ -68,6 +68,13 @@
     # Python 3
     long = int

+Deferred = None
+try:
+    from twisted.internet import defer
+    Deferred = defer.Deferred
+except ImportError:
+    pass
+
 inPy3k = sys.version_info[0] == 3

 if inPy3k:
@@ -562,15 +569,27 @@
                 if patching.new is DEFAULT:
                     extra_args.append(arg)
             args += tuple(extra_args)
-            try:
-                return func(*args, **keywargs)
-            finally:
+            def cleanup(res):
                 for patching in reversed(getattr(patched, 'patchings', [])):
                     patching.__exit__()
+                return res
+            singleton = {}
+            retval = singleton
+            try:
+                retval = func(*args, **keywargs)
+            except:
+                cleanup(None)
+                raise
+            if Deferred is None or not isinstance(retval, Deferred):
+                return cleanup(retval)
+            retval.addBoth(cleanup)
+            return retval

         patched.patchings = [self]
         if hasattr(func, 'func_code'):
             # not in Python 3
-            patched.compat_co_firstlineno = getattr(func, 
"compat_co_firstlineno",
-                                                    
func.func_code.co_firstlineno)
+            patched.compat_co_firstlineno = getattr(
+                func, "compat_co_firstlineno",
+                func.func_code.co_firstlineno
+            )
         return patched

Original issue reported on code.google.com by zoo...@gmail.com on 17 Jun 2011 at 6:00

Attachments:

GoogleCodeExporter commented 9 years ago 
Sorry about the unnecessary reformatting of the getattr() line there at the end 
-- that was an accident.

Original comment by zoo...@gmail.com on 17 Jun 2011 at 6:00

GoogleCodeExporter commented 9 years ago 
Here is the code I submitted for the Tahoe-LAFS issue:

http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1395#comment:27

Original comment by zoo...@gmail.com on 17 Jun 2011 at 6:13

GoogleCodeExporter commented 9 years ago 
I'd love to better support deferreds, however the issue I have with this patch 
is that it will unconditionally import twisted (if available) even if it isn't 
being used. Can you think of an alternative approach?

An explicit enable_deferred_support function would be one way, but it would 
need calling in every test module that you want to run individually (or just 
once in the test runner framework I guess).

Another way would be recognising deferreds from the __name__ of the type and 
their __module__?

And I prefer the suggested formatting for the getattr call. :-)

Original comment by fuzzyman on 17 Jun 2011 at 12:19

GoogleCodeExporter commented 9 years ago

Original comment by fuzzyman on 17 Jun 2011 at 12:19

GoogleCodeExporter commented 9 years ago 
Also, a test would be nice...

Original comment by fuzzyman on 17 Jun 2011 at 11:10

GoogleCodeExporter commented 9 years ago 
I don't understand the problem with importing twisted. If Twisted is installed, 
and if you intend to support tests returning Deferreds, then you probably ought 
to import twisted. Why not?

Original comment by zoo...@gmail.com on 20 Jun 2011 at 5:06

GoogleCodeExporter commented 9 years ago 
On Snow Leopard, executing "from twisted.internet import defer" takes 0.4 
seconds. The number of modules in sys.modules increases from 33 to 185. 

If your code is importing twisted anyway this is no problem. If your code is 
not importing twisted then paying this extra startup / memory cost is not 
acceptable. Just because twisted is available on the system does not mean it is 
being used in any particular piece of code.

Original comment by fuzzyman on 20 Jun 2011 at 9:15

GoogleCodeExporter commented 9 years ago 
How about something like this:

>>> def is_deferred(obj):
...     defer = sys.modules.get('twisted.internet.defer')
...     if defer is None:
...         return False
...     return isinstance(obj, defer.Deferred)

This only uses defer.Deferred if it has already been imported. 
...

Original comment by fuzzyman on 20 Jun 2011 at 11:00

GoogleCodeExporter commented 9 years ago 
That sounds okay to me. I guess the test code can't return an instance of 
Deferred without populating sys.modules. Oh wait, can they have imported the 
Deferred class from the twisted.internet.defer module without putting the 
string "twisted.internet.defer" in the sys.modules.keys()?

I don't feel confident that I understand the import machinery well enough to 
answer that question.

(Aside: maybe this pattern should be generalized into a function that people 
can use to test for isinstance of a thing that they don't want to import if it 
isn't already imported.)

Original comment by zoo...@gmail.com on 22 Jun 2011 at 4:29

GoogleCodeExporter commented 9 years ago 
Yep, if Deffered is implemented in 'twisted.internet.defer' then importing it 
(even indirectly) will always put 'twisted.internet.defer' into sys.modules.

Could you write a test for your patch as it stands and I'll adapt it? Obviously 
the test will depend on twisted. I wouldn't know how to test that the behavior 
with deferreds works as expected.

An is_instance that takes a string instead of a class is an interesting idea. 
Not *really* sure it belongs in mock, but if it has to be implemented *anyway* 
then maybe exposing it is harmless (beyond maintenance and documentation 
burden).

Original comment by fuzzyman on 22 Jun 2011 at 10:56

GoogleCodeExporter commented 9 years ago

Original comment by fuzzyman on 22 Jun 2011 at 10:57

GoogleCodeExporter commented 9 years ago 
For your information: the Tahoe-LAFS project is going ahead and using an ad hoc 
one-off patcher for this one test that we were working on. I posted a plea on 
the Tahoe-LAFS ticket -- 
http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1395#comment:31 -- for someone to 
come over here to mock.py and contribute code-review and a unit test. :-)

Original comment by zoo...@gmail.com on 30 Jun 2011 at 3:04

GoogleCodeExporter commented 9 years ago 
Thanks Zooko.

Original comment by fuzzyman on 30 Jun 2011 at 3:52

GoogleCodeExporter commented 9 years ago 
My concern is, what happens if the deferred never fires - presumably then the 
patch is never undone (the cleanup is never executed). Is this possible - is 
there anything that can be done about it? (The patch *must* be undone before 
the next test starts to execute.)

If it *isn't* possible to prevent this within mock.patch itself, can mock 
change to make it easier to build this as a utility on top of mock?

Original comment by fuzzyman on 16 Jul 2011 at 4:22

GoogleCodeExporter commented 9 years ago 
Dealing with that is what trial does. It deals with it by delaying the 
beginning of the next test until any deferred returned by the previous test has 
fired, and also by reporting to the user about the deferred which failed to 
fire. In any case, I don't think mock.py needs any added functionality to 
handle this. Nor, as far as I can think, does trial need any added 
functionality to handle deferred undoing of patches. (I've used this patch or a 
patch like it myself in unit tests in trial and had no problems.)

Original comment by zoo...@gmail.com on 17 Jul 2011 at 11:03

GoogleCodeExporter commented 9 years ago 
Ok - thanks Zooko.

Original comment by fuzzyman on 17 Jul 2011 at 11:27

GoogleCodeExporter commented 9 years ago 
Note that even without this fix in place you can achieve the same thing using 
patch.start / patch.stop.

Instead of using patch as a decorator:

{{{
def test_thing(self):
    patcher = patch('module.klass')
    mock = patcher.start()

    ...

    deferred.addBoth(lambda res: patcher.stop())
}}}

Original comment by fuzzyman on 17 Jul 2011 at 11:33

GoogleCodeExporter commented 9 years ago 
If a test times out, then the patch will not be undone. (However, in my 
experience when a test times out then all subsequent test results are 
unreliable anyway, so perhaps this doesn't matter too much.)

Original comment by davidsar...@googlemail.com on 24 Jul 2011 at 10:29

GoogleCodeExporter commented 9 years ago

Original comment by fuzzyman on 26 Jul 2011 at 10:33

GoogleCodeExporter commented 9 years ago 
I'm assuming that the following snippet is sufficient solution to this problem, 
and further changes to mock are not needed:

    deferred.addBoth(lambda res: patcher.stop())

If I'm wrong add comments here and I'll re-open.

Original comment by fuzzyman on 14 Apr 2012 at 9:53