|
msg132026 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-03-24 20:43 |
A common thing to do in setUp or test* methods is to replace some module attribute with something else, either to mock an object calling an external resource or to test platform-specific behavior (for example, changing os.name before calling some function). Care has to be taken to restore the initial object with addCleanup, tearDown or in a finally block.
I propose that a new method TestCase.patch (inspired by mock.patch, but more limited in scope) be added, to allow such usages (each example is standalone):
def setUp(self):
self.patch(socket, 'socket', MockSocket)
def test_default_format(self):
self.patch(os, 'name', 'posix')
self.assertEqual(get_default_format(), '.tar.gz')
self.path(os, 'name', 'nt')
self.assertEqual(get_default_format(), '.zip')
def setUp(self):
self.patch(sys, 'path', sys.path.copy())
In each example, patch(object, attribute, value) does this: save object.attribute, set object.attribute to value, register a cleanup function to restore object.attribute.
I assigned to Michael so that he can kill this idea early if he has reason to do so. If not, please move stage to “patch needed” (no pun). I am willing to work on a patch for 3.3 and unittest2 (not sure which is first :)
|
|
msg132027 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-03-24 20:49 |
Typo s/self.path/self.patch/
I forgot to mention the rationale for this method: factor out common code to make sure the cleanup is not forgotten. Also kill debates about addCleanup vs. tearDown vs. try/finally.
|
|
msg132028 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-03-24 20:50 |
Needless to say the name is open: patch, replace, settempvalue, what have you.
|
|
msg132259 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2011-03-26 18:01 |
I'm attaching a draft patch for patch. Éric, is this patch implementing patch as you expected?
This patch is not finished because there are many cases where patch can leave patched objects if it fails to unpatch.
|
|
msg132265 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2011-03-26 19:02 |
I'd like to ponder this a bit. Note that the patch is incorrect - fetching the attribute should not be done with getattr (this will trigger descriptors instead of fetching the underlying member) and should not be reset unconditionally (if the original was fetched from a base class the just deleting the patched member will restore the original).
If we decide to do this I can provide a patch.
|
|
msg132493 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-03-29 18:08 |
A similar function already exists: test.support.patch
|
|
msg132494 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2011-03-29 18:18 |
Right, I helped with the writing of that at PyCon. The patch method would look very similar. test.support.patch is not something we want to make public (in that location).
|
|
msg136980 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2011-05-26 16:44 |
There’s also test.support.swap_attr...
|
|
msg155462 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2012-03-12 19:03 |
mock is being added to Python 3.3 as unittest.mock - so a helper TestCase.patch should delegate to unittest.mock.patch.
|
|
msg170542 - (view) |
Author: Julian Berman (Julian) |
Date: 2012-09-16 00:49 |
It's kind of unfortunate that `mock.patch` is called `mock.patch`. I was thinking about this a bit more yesterday, and `mock.patch.object` is the one that I think would be most appropriate to put on `TestCase`, and the best name for it is probably `patch`, but doing that would be deathly confusing, so I don't think that's a real choice we can make.
|
|
msg170606 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2012-09-17 13:48 |
Why would mock.patch.object be the appropriate one to add to TestCase? patch.object is used orders of magnitude less than patch.
|
|
msg170688 - (view) |
Author: Julian Berman (Julian) |
Date: 2012-09-19 00:22 |
It's slightly less confusing -- "Where do I patch" is the question that will never go away, and the fact that you don't have the `sys` module imported is a small hint that you should be doing patch(mymodule.sys, "path") not patch("sys.path"). Also, the fact that patch is more common doesn't reflect the fact that most of those times, patch.object would have worked as well, but it's longer to type (or people aren't as aware of it), since most of the time you're patching things in a module you've imported already (at least this is true of me, and I've started using patch.object whenever it works and only falling back on patch).
Also, Twisted's TestCase (which already has a method to implement patch) is functionally equivalent to patch.object, not patch, in case you wanted a precedent.
|
|
msg170689 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2012-09-19 00:24 |
Well, people vote with their code and find mock.patch vastly more useful than patch.object...
|
|
msg170690 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2012-09-19 00:33 |
I actually agree with Julian here. I much prefer patch.object and do my best to avoid mock.patch. support.patch is also equivalent to patch.object and not patch. That doesn't change the fact that other people prefer mock.patch, of course.
I think mock.patch is too "magical" for my taste. There is something I don't like about the dynamic import, even though I can't really tell you what it is :)
|
|
msg170691 - (view) |
Author: Julian Berman (Julian) |
Date: 2012-09-19 00:35 |
With all due respect, your response pretty much ignored mine completely. That's OK, I've agreed with you that patch seems more common.
I'll point you additionally though to the fact that Éric's original post also used patch.object's semantics, as does test.test_support.swap_attr and patch.
I don't know how hard I can push here though, since again, this would be really confusing to have it have the same name.
|
|
msg170692 - (view) |
Author: Chris Jerdonek (chris.jerdonek) *  |
Date: 2012-09-19 00:36 |
What about patch_object()?
|
|
msg170694 - (view) |
Author: Ezio Melotti (ezio.melotti) *  |
Date: 2012-09-19 01:33 |
IMHO a setattr-like API seems the obvious choice here, so that's what I would expect. I haven't used mock, so I wasn't familiar with mock.patch, but after skimming through the mock docs a bit I think I have to agree with Julian and RDM.
In addition, I'm not sure we need TestCase.patch now that we have already have mock.patch(.object) in the stdlib. If we still add it, it would probably make more sense as a "vanilla" patch that doesn't depend on mock.
> a helper TestCase.patch should delegate to unittest.mock.patch
Does it mean it will return MagicMocks?
> patch.object would have worked as well, but it's longer to type
If mock.patch requires a FQN to work the call might even be longer:
patch('package.subpackage.module.function') vs
patch.object(module, 'function', newfunc)
(assuming "from package.subpackage import module", which is not uncommon if we are testing that specific module)
> What about patch_object()?
patchobj()?
|
|
msg170716 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2012-09-19 09:32 |
It maybe that patch.object is a more natural interface to the small sample of people commenting here, in which case great - that's what it's there for.
However in common usage patch is used around two orders of magnitude more. I've seen large codebases with hundreds of uses of patch and only a handful of uses of patch.object.
To support the *minor* use case and not the major use case in TestCase would be an inanity.
|
|
msg170724 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2012-09-19 14:46 |
A data point: at work I follow Pyramid testing guidelines which tell you not to import code under test at module level, but in your test functions, so that if you have an error your tests do start and you see the error under the test method. This means that I use mock.patch and not mock.patch.object, as my modules are not imported.
|
|
msg170725 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2012-09-19 14:49 |
I think those guidelines are horrible and I've told the pyramid folks that.
There is a related issue for unittest that failing to import a test module (due to a failed import in the test module for example) should not kill the test run but should create a "failing test" that shows the problem.
This is all wandering off topic however...
|
|
msg185287 - (view) |
Author: Brett Cannon (brett.cannon) *  |
Date: 2013-03-26 18:10 |
So, what's the status of this? Move it forward or close this?
|
|
msg185326 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2013-03-27 12:25 |
Yes this is still relevant and needs doing (and is easy).
The implementation should be similar to:
def patch(self, *args, **kwargs):
# lazy import
from unittest.mock import patch
p = patch(*args, **kwargs)
result = p.start()
self.addCleanup(p.stop)
return result
Plus tests and documentation.
|
|
| Date |
User |
Action |
Args |
| 2013-03-27 12:25:27 | michael.foord | set | status: pending -> open
messages:
+ msg185326 |
| 2013-03-26 18:10:10 | brett.cannon | set | status: open -> pending
messages:
+ msg185287 |
| 2012-09-19 14:49:00 | michael.foord | set | messages:
+ msg170725 |
| 2012-09-19 14:46:54 | eric.araujo | set | messages:
+ msg170724 |
| 2012-09-19 09:32:06 | michael.foord | set | messages:
+ msg170716 |
| 2012-09-19 01:33:16 | ezio.melotti | set | messages:
+ msg170694 |
| 2012-09-19 00:36:43 | chris.jerdonek | set | messages:
+ msg170692 |
| 2012-09-19 00:35:40 | Julian | set | messages:
+ msg170691 |
| 2012-09-19 00:33:11 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg170690
|
| 2012-09-19 00:24:35 | michael.foord | set | messages:
+ msg170689 |
| 2012-09-19 00:22:57 | Julian | set | messages:
+ msg170688 |
| 2012-09-17 13:48:57 | michael.foord | set | messages:
+ msg170606 |
| 2012-09-16 00:49:19 | Julian | set | messages:
+ msg170542 |
| 2012-09-04 12:48:31 | Julian | set | nosy:
+ Julian
|
| 2012-08-23 19:55:43 | chris.jerdonek | set | nosy:
+ chris.jerdonek
|
| 2012-08-23 14:36:58 | moijes12 | set | nosy:
- moijes12
|
| 2012-07-03 04:34:36 | moijes12 | set | nosy:
+ moijes12
|
| 2012-03-12 19:03:51 | michael.foord | set | messages:
+ msg155462 |
| 2011-05-26 16:44:54 | eric.araujo | set | messages:
+ msg136980 |
| 2011-03-29 18:18:02 | michael.foord | set | messages:
+ msg132494 |
| 2011-03-29 18:08:26 | eric.araujo | set | messages:
+ msg132493 |
| 2011-03-26 19:02:55 | michael.foord | set | messages:
+ msg132265 |
| 2011-03-26 18:01:30 | pablomouzo | set | files:
+ patch.diff
nosy:
+ pablomouzo messages:
+ msg132259
keywords:
+ patch |
| 2011-03-25 19:00:54 | brett.cannon | set | nosy:
+ brett.cannon
|
| 2011-03-25 17:29:48 | daniel.urban | set | nosy:
+ daniel.urban
|
| 2011-03-24 20:50:31 | eric.araujo | set | messages:
+ msg132028 |
| 2011-03-24 20:49:35 | eric.araujo | set | messages:
+ msg132027 |
| 2011-03-24 20:43:59 | eric.araujo | create | |