Monkey-patching support code

[jml]

Unit tests often end up doing a lot of monkey-patching. We could make it a little easier by abstracting out the process. I think teratorn has something in a sandbox which would make a good starting point.

[exarkun]

Note that this has been under discussion for some time. I have avoided making a ticket because it's not really clear what functionality should be supported.

I would prefer for some agreement to be reached before anything is added to Twisted.

[jml]

Comments attached to spec.

[jml]

OK. I have a branch up in monkey-patch-2598 that has some working code which may serve to ground discussion. I guess this makes this ticket up for review.

[therve]

That looks great. There are some classic review comments (pyflakes errors, copyright) but it may be too soon for that.

My only complain would be to have an example. I needed something like this in twisted.test.test_log.PythonLoggingIntegrationTestCase, but as you can see, it's something I would like to set for one test only. So something like what's proposed in the wiki page might be more suitable:

    def test_mything(self):
        assert...
    test_mything = monkeyPatch(test_mything, [(obj, 'attr', mock)]
or
    test_mything = monkeyPatch([(obj, 'attr', mock)])(test_mything)

[therve]

After think at this more, I think the concept used by addCleanup is the good one. What I really want to do is at the beginning the test declare the monkeyPatch and expect it to be clean:

    def test_mything(self):
        self.addPatch(obj, 'attr', mock)
        self.addPatch(obj2, 'attr2', mock2)
        assert ...

The decorator syntax is not so flexible.

[jml]

It's been over a month since I put this branch up for review. I'll need to look at it again before I can figure out whether you are agreeing with me or whether you are making a suggestion for change.

[jml]

To be clear, this branch is waiting for review.

[therve]

That would be nice to get at least one answer to my questions...

[jml]

I noticed that you didn't ask any specific questions. Did you mean to?

Either way, I'll be glad to respond to your API suggestions. I'll get round to it as soon as I can.

FWIW, I'd be able to respond more swiftly if you had mentioned what the code is currently doing, in addition to suggesting improvements. If I'm not actively working on code, it flies out of my head.

[therve]

OK, let's try to be more explicit. If I understand correctly, the way to use current branch is this:

class MyTestCase(MonkeyPatchedTestCase):
    def getPatches(self):
        return [(object, 'attr', val), (object2', 'attr2', val2)]
    def test_foo(self):
        # blah
    test_foo = monkeyPatch(test_foo)
    def test_bar(self):
        # blah
    test_bar = monkeyPatch(test_bar)

I see different problems with this. First, I don't see why you have to decorate your functions whereas you've already declared your test to be a MonkeyPatchedTestCase. I think all methods could be consider to be monkeypatched.

Then, when I needed to monkeypatch something, I generally needed one set of patches for each method. I agree that class grouping is artificial, but it's useful. So my recommended API would be this:

class MyTestCase(TestCase):
    def test_foo(self):
        self.patch(object, 'attr', val)
        self.patch(object2, 'attr2', val2)
        # blah
    def test_bar(self):
        self.patch(object, 'attr', val)
        self.patch(object2, 'attr2', val2)
        # blah

And you could always have a grouping this way:

class MyTestCase(TestCase):
    def _patches(self):
        self.patch(object, 'attr', val)
        self.patch(object2, 'attr2', val2)
    def test_foo(self):
        self._patches()
        # blah
    def test_bar(self):
        self._patches()
        # blah

I think this more clear and more flexible at the same time.

[therve]

So my questions are: what do you think of the API? what are your arguments for/against this one and yours?

[jml]

The current implementation already allows for something similar:

class FooTestCase(TestCase):
    def setUp(self):
        self._patches = MonkeyPatch((object, 'attr', val), (object2, 'attr2', val2))
    def test_foo(self):
        self._patches.patch()
        self.addCleanup(self._patches.restore)
        # do stuff

I'm +0 on adding a convenience method akin to the TestCase.patch you describe. The reason I'm not +1 is that it makes it unclear as to when the patches are removed. Perhaps if it returned the MonkeyPatch object so the caller could manually restore patches.

I'm not a particular fan of the decorator anyway. I may have in fact added it because of your recommendation (I honestly can't remember). In any case, here's the defense:

MonkeyPatchedTestCase merely adds support for monkey-patching with a decorator using decorator syntax. Perhaps the name is misleading. It doesn't automatically monkey-patch stuff because I think it should be possible to have an unpatched test or two in a predominantly monkey-patched TestCase subclass.

[therve]

We agree at least that the decorator syntax is not great. It existed before my comment, I think it comes from the original work from teratorn.

I don't think that's making it unclear when the patch is removed. It's clear to me that it's removed at the end of the test method, and I don't have an usecase where I want to remove the patch at the middle of the test. Furthermore, patches must be removed before the end of the test for not clutering other tests method. Returning the object seems sane nonetheless.

Your example is OK, but I think it's a burden to write all of this, that's why I am for a convenience method. Another voice for or against would close the debate :).

[jml]

Nah, teratorn's version didn't have a decorator. I guess I did that all by myself.

I'm happy to add a convenience method, as long as it returns the MonkeyPatch? object and has good API docs.

[therve]

I just wanted to point at http://labix.org/mocker where we can get some inspiration.

[jml]

Added patch() method to TestCase.

Ready for review.

[therve]

in test_monkey.py:

• it needs a copyright line and a docstring
• this is unfortunate that TestObj attributes name and value are the same: if you invert it in monkey, no test fail
• test_patchExisting, test_patchNonExisting and test_patchAlreadyPatched docstrings don't explain what is expected in the tests
• The C{MonkeyPatcher} in the docstring of MonkeyPatcherTest could be L{MonkeyPatcher}

in test_tests.py:

• the methods in TestMonkeyPatchSupport need docstrings
• the way the addCleanup is tested is not very conventional: testing in tearDown isn't clear about the intents. Maybe that could be tested with creating another TestCase object ?

in monkey.py:

• MonkeyPatcher should document its attributes, even the private ones
• _alreadyPatched should have a docstring
• restore docstring doesn't have the good format
• I'm not sure addPatches and runWithPatches are useful, because we don't use currently. Do you think we should really keep it?

in unittest.py:

• the fact patch() returns the MonkeyPatcher object isn't tested.

That's it!

[jml]

(In [21782]) Changes for most of the review comments. Refs #2598.

[jml]

test_monkey.py:

• License and module docstring added.
• Updated the docstrings you mentioned.
• Changed to use variables instead of magic values for 'foo', 'bar', 'baz' and then changed the values to 'foo value' etc.

test_tests.py:

• Docstrings added.
• I've changed the way it's tested to be more conventional. Personally I think it's less clear, but YMMV.

monkey.py:

• Added comments for internal variables in the constructor. That's the most documentation I'm going to do for internal variables.
• Added docstring for _alreadyPatched
• Corrected format of restore's docstring.
• I've removed addPatches. I'm keeping runWithPatches because I do think it's genuinely useful.

unittest.py doesn't need tests for the type of the return value of patch(). It already tests the behaviour of the object -- that's enough.

[therve]

in monkey.py:

• thanks for the comments, that's a perfect solution
• in restore, I would prefer while self._originals instead of while len(self._originals) > 0

in unittest.py:

• sorry for the return value of patch(), I definitely missed something

There is another thing I wanted to point out. See this test to be added in TestMonkeyPatchSupport:

    def test_successivePatches(self):
        self.test.patch(self, 'objectToPatch', 'first patch')
        self.test.patch(self, 'objectToPatch', 'second patch')
        self.test.run(reporter.Reporter())
        self.assertEqual(self.objectToPatch, self.originalValue)

At first, I thought it would fail because of the implementation of patch(), but it succeed because the cleanups are running reversed (LIFO). I don't think that's a thing we could rely on, so if think a TestCase object should only have one patcher, initialized at the first call at patch(). What do you think?

[jml]

(In [21799]) Add a test that looks like it's testing patch() but is really testing addCleanup()

Refs #2598

[jml]

Cleanups running in LIFO order is exactly something that we can rely on. It's a documented, tested behaviour of addCleanup.

Removed the len() and added your suggested test.

[therve]

Alright, please merge.

[jml]

(In [21800]) Add support for monkey patching, both in code and in tests.

• Author: jml
• Reviewer: therve
• Fixes #2598

Monkey patching is a necessary evil, particularly useful for testing legacy code. This branch provides support for monkey patching in tests in a way that significantly reduces the chance of forgetting to unapply your patch.