classification
Title: Add patch method to unittest.TestCase
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Julian, brett.cannon, chris.jerdonek, daniel.urban, eric.araujo, ezio.melotti, michael.foord, pablomouzo, parkouss, pitrou, r.david.murray, rbcollins, vadmium
Priority: normal Keywords: easy, patch

Created on 2011-03-24 20:43 by eric.araujo, last changed 2014-11-07 15:31 by rbcollins.

Files
File name Uploaded Description Edit
patch.diff pablomouzo, 2011-03-26 18:01 patch for patch review
issue-11664.patch parkouss, 2014-10-08 18:08 add a TestCase.patch method review
Messages (31)
msg132026 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2011-03-29 18:08
A similar function already exists: test.support.patch
msg132494 - (view) Author: Michael Foord (michael.foord) * (Python committer) 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) * (Python committer) Date: 2011-05-26 16:44
There’s also test.support.swap_attr...
msg155462 - (view) Author: Michael Foord (michael.foord) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2012-09-19 00:36
What about patch_object()?
msg170694 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (vadmium) * 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) * (Python committer) 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.
History
Date User Action Args
2014-11-07 15:31:02rbcollinssetmessages: + msg230814
2014-10-13 02:31:42vadmiumsetmessages: + msg229213
2014-10-13 01:34:43ezio.melottisetmessages: + msg229207
2014-10-13 01:21:11rbcollinssetmessages: + msg229205
2014-10-12 22:47:52michael.foordsetmessages: + msg229202
2014-10-12 11:26:21Juliansetmessages: + msg229140
2014-10-08 18:08:34parkousssetfiles: + issue-11664.patch

messages: + msg228804
2014-10-08 17:48:33pitrousetassignee: michael.foord ->
versions: + Python 3.5, - Python 3.3
2014-10-08 17:48:22pitrousetnosy: + pitrou, rbcollins
messages: + msg228801
2014-10-08 15:59:46parkousssetnosy: + parkouss
messages: + msg228798
2014-08-06 04:55:35vadmiumsetnosy: + vadmium
2013-03-27 12:25:27michael.foordsetstatus: pending -> open

messages: + msg185326
2013-03-26 18:10:10brett.cannonsetstatus: open -> pending

messages: + msg185287
2012-09-19 14:49:00michael.foordsetmessages: + msg170725
2012-09-19 14:46:54eric.araujosetmessages: + msg170724
2012-09-19 09:32:06michael.foordsetmessages: + msg170716
2012-09-19 01:33:16ezio.melottisetmessages: + msg170694
2012-09-19 00:36:43chris.jerdoneksetmessages: + msg170692
2012-09-19 00:35:40Juliansetmessages: + msg170691
2012-09-19 00:33:11r.david.murraysetnosy: + r.david.murray
messages: + msg170690
2012-09-19 00:24:35michael.foordsetmessages: + msg170689
2012-09-19 00:22:57Juliansetmessages: + msg170688
2012-09-17 13:48:57michael.foordsetmessages: + msg170606
2012-09-16 00:49:19Juliansetmessages: + msg170542
2012-09-04 12:48:31Juliansetnosy: + Julian
2012-08-23 19:55:43chris.jerdoneksetnosy: + chris.jerdonek
2012-08-23 14:36:58moijes12setnosy: - moijes12
2012-07-03 04:34:36moijes12setnosy: + moijes12
2012-03-12 19:03:51michael.foordsetmessages: + msg155462
2011-05-26 16:44:54eric.araujosetmessages: + msg136980
2011-03-29 18:18:02michael.foordsetmessages: + msg132494
2011-03-29 18:08:26eric.araujosetmessages: + msg132493
2011-03-26 19:02:55michael.foordsetmessages: + msg132265
2011-03-26 18:01:30pablomouzosetfiles: + patch.diff

nosy: + pablomouzo
messages: + msg132259

keywords: + patch
2011-03-25 19:00:54brett.cannonsetnosy: + brett.cannon
2011-03-25 17:29:48daniel.urbansetnosy: + daniel.urban
2011-03-24 20:50:31eric.araujosetmessages: + msg132028
2011-03-24 20:49:35eric.araujosetmessages: + msg132027
2011-03-24 20:43:59eric.araujocreate