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.
|
msg228798 - (view) |
Author: Julien Pagès (parkouss) |
Date: 2014-10-08 15:59 |
Hi all,
I would like to contribute to Python and I'm interested in working on this. I have few questions (I hope you don't mind that I ask here):
- is this issue still open and needed ?
- if yes, do I have to work from 3.3 branch, as stated in the issue "Versions" field, or in the default one ?
|
msg228801 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2014-10-08 17:48 |
Hi Julien and welcome,
> - is this issue still open and needed ?
Yes and "perhaps". I have no opinion on whether it is necessary, but other people seem to think it's useful.
> - if yes, do I have to work from 3.3 branch, as stated in the issue "Versions" field, or in the default one ?
No, you should work with the default branch. Other branches only receive bug fixes, not improvements such as this.
If you haven't yet done so, you may also take a look at the devguide:
https://docs.python.org/devguide/
|
msg228804 - (view) |
Author: Julien Pagès (parkouss) |
Date: 2014-10-08 18:08 |
Thanks Antoine for the link, and the quick answer;
It seems that it is a sensible subject, adding or not this method, and what it should do. I wrote the patch anyway, but I must confess that somewhere it feels strange to me to add such a method in TestCase class.
Nevertheless, it could be useful, and I will let other people decide this. :)
|
msg229140 - (view) |
Author: Julian Berman (Julian) * |
Date: 2014-10-12 11:26 |
My opinion is already here re: patch vs patch.object, so I won't repeat it, but @Michael, if you really want .patch, are you open to adding .patch_object as well?
(Regardless, thanks for working on this Julien.)
|
msg229202 - (view) |
Author: Michael Foord (michael.foord) *  |
Date: 2014-10-12 22:47 |
The patch (including lazy import) looks good, and the test looks ok too. I still think that patch should be the default instead of patch.object - although I wouldn't object to a second method (name?) if there was significant demand.
|
msg229205 - (view) |
Author: Robert Collins (rbcollins) *  |
Date: 2014-10-13 01:21 |
FWIW I'd really like to be reducing the TestCase API not extending it - particularly since there are lots of good convenient ways of doing this already (not least mock.patch/mock.patch.object).
So I'm -0.5 on adding this, as I don't see it adding value. That said, I'll happily review for correctness if there is consensus that we want it.
Relatedly I'd like to find some way to let regular functions tie into cleanups automatically, so that we don't need helpers like this *at all*. That probably needs a PEP though.
|
msg229207 - (view) |
Author: Ezio Melotti (ezio.melotti) *  |
Date: 2014-10-13 01:34 |
I'm -0.5 on this as well, and agree that we should try to keep the TestCase API small.
On one hand, a patch method available without extra imports would be handy, and having this as a generic function/method in unittest seems more natural to me than having it in unittest.mock. On the other hand, adding it to unittest has downsides as well: it increases API complexity, adds duplication and possibly confusion (people might wonder if they should use TestCase.patch or unittest.mock.patch, and if there are any differences). Adding both .patch and .patch_object makes things even worse.
|
msg229213 - (view) |
Author: Martin Panter (martin.panter) *  |
Date: 2014-10-13 02:31 |
[padding to avoid UTF-8 error with bug tracker]
See also Issue 22374, where an equivalent of “patch.object” is suggested as an example context manager for the “contextlib” documentation.
If we added a plain function or context manager rather than a new TestCase method, it might avoid the worries about bloating the API. Then it could be a generic thing for any kind of testing, and not coupled with the “unittest” framework.
About cleanup functions more generally, I think they already tie in well with the TestCase.addCleanup() API. Perhaps it could handle general context managers as well though, by inheriting an ExitStack.enter_context() method or providing an ExitStack attribute.
|
msg230814 - (view) |
Author: Robert Collins (rbcollins) *  |
Date: 2014-11-07 15:31 |
+1 on a plain function or context manager.
w.r.t. addCleanUp taking a context manager, that could be interesting - perhaps we'd want a thing where you pass it the context manager, it __enter__'s the manager and then calls addCleanUp for you.
|
msg351745 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2019-09-10 22:23 |
-1
I think mocking should be kept orthogonal to the unittest module. A person is free to use mocking with different testing tools like py.test or nose. Likewise, they are free to use a different mocking/patching tool than our standard library mock.
In my mind, they are separate tools that should remain loosely coupled and should not cross-import one another.
|
msg351769 - (view) |
Author: Karthikeyan Singaravelan (xtreak) *  |
Date: 2019-09-11 08:12 |
I concur with Raymond here. Since mock is also part of stdlib and this issue predates mock being merged to stdlib. I think adding it to unittest sort of expands the unittest API being just an alias to mock.patch. mock.patch is used in lot of places and has a good adoption as plain function, decorator and context manager. This would also mean explaining people why there is mock.patch and TestCase.patch in docs too.
|
msg361736 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2020-02-10 22:27 |
Given the two recent -1 responses let's close this.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:15 | admin | set | github: 55873 |
2020-02-10 22:27:47 | gvanrossum | set | status: open -> closed
nosy:
+ gvanrossum messages:
+ msg361736
resolution: wont fix stage: resolved |
2019-11-27 18:45:14 | brett.cannon | set | nosy:
- brett.cannon
|
2019-09-11 08:12:03 | xtreak | set | nosy:
+ xtreak messages:
+ msg351769
|
2019-09-10 22:23:22 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg351745
|
2019-09-10 21:00:58 | ta1hia | set | nosy:
+ ta1hia
|
2016-10-04 11:07:20 | Mariatta | set | nosy:
+ Mariatta
|
2016-09-19 06:24:20 | christian.heimes | set | versions:
+ Python 3.7, - Python 3.5 |
2014-11-07 15:31:02 | rbcollins | set | messages:
+ msg230814 |
2014-10-13 02:31:42 | martin.panter | set | messages:
+ msg229213 |
2014-10-13 01:34:43 | ezio.melotti | set | messages:
+ msg229207 |
2014-10-13 01:21:11 | rbcollins | set | messages:
+ msg229205 |
2014-10-12 22:47:52 | michael.foord | set | messages:
+ msg229202 |
2014-10-12 11:26:21 | Julian | set | messages:
+ msg229140 |
2014-10-08 18:08:34 | parkouss | set | files:
+ issue-11664.patch
messages:
+ msg228804 |
2014-10-08 17:48:33 | pitrou | set | assignee: michael.foord -> versions:
+ Python 3.5, - Python 3.3 |
2014-10-08 17:48:22 | pitrou | set | nosy:
+ pitrou, rbcollins messages:
+ msg228801
|
2014-10-08 15:59:46 | parkouss | set | nosy:
+ parkouss messages:
+ msg228798
|
2014-08-06 04:55:35 | martin.panter | set | nosy:
+ martin.panter
|
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 | |