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

Created on 2011-03-24 20:43 by eric.araujo, last changed 2014-08-06 04:55 by vadmium.

Files
File name Uploaded Description Edit
patch.diff pablomouzo, 2011-03-26 18:01 patch for patch review
Messages (22)
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.
History
Date User Action Args
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