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 :)

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.

Needless to say the name is open: patch, replace, settempvalue, what have you.

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.

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.

A similar function already exists: test.support.patch

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).

There’s also test.support.swap_attr...

mock is being added to Python 3.3 as unittest.mock - so a helper TestCase.patch should delegate to unittest.mock.patch.

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.

Why would mock.patch.object be the appropriate one to add to TestCase? patch.object is used orders of magnitude less than patch.

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.

Well, people vote with their code and find mock.patch vastly more useful than patch.object...

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 :)

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.

What about patch_object()?

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()?

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.

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.

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...

So, what's the status of this? Move it forward or close this?

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.

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 ?

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/

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. :)

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.)

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.

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.

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.

[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.

+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.

-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.

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.

Given the two recent -1 responses let's close this.