Opened 15 years ago

Closed 14 years ago

#2598 enhancement closed fixed (fixed)

Monkey-patching support code

Reported by: Jonathan Lange Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, Glyph, therve Branch: branches/monkey-patch-2598-4
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

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.

Change History (28)

comment:1 Changed 15 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

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.

comment:2 Changed 15 years ago by Jonathan Lange

Comments attached to spec.

comment:3 Changed 15 years ago by Jonathan Lange

Cc: Glyph added
Keywords: review added
Priority: normalhighest

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.

comment:4 Changed 15 years ago by Jonathan Lange

Owner: Jonathan Lange deleted

comment:5 Changed 15 years ago by 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)

comment:6 Changed 15 years ago by 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.

comment:7 Changed 15 years ago by Jonathan Lange

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.

comment:8 Changed 15 years ago by Jonathan Lange

To be clear, this branch is waiting for review.

comment:9 Changed 15 years ago by therve

Cc: therve added

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

comment:10 Changed 15 years ago by Jonathan Lange

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.

comment:11 Changed 15 years ago by 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.

comment:12 Changed 15 years ago by therve

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

comment:13 Changed 15 years ago by Jonathan Lange

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.

comment:14 Changed 15 years ago by 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 :).

comment:15 Changed 15 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Jonathan Lange

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.

comment:16 Changed 14 years ago by therve

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

comment:17 Changed 14 years ago by Jonathan Lange

Branch: branches/monkey-patch-2598-4
Keywords: review added
Owner: Jonathan Lange deleted

Added patch() method to TestCase.

Ready for review.

comment:18 Changed 14 years ago by therve

Owner: set to therve

comment:19 Changed 14 years ago by therve

Keywords: review removed
Owner: changed from therve to Jonathan Lange

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!

comment:20 Changed 14 years ago by Jonathan Lange

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

comment:21 Changed 14 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to therve

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.

comment:22 Changed 14 years ago by therve

Keywords: review removed
Owner: changed from therve to Jonathan Lange

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?

comment:23 Changed 14 years ago by Jonathan Lange

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

Refs #2598

comment:24 Changed 14 years ago by Jonathan Lange

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.

comment:25 Changed 14 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to therve

comment:26 Changed 14 years ago by therve

Keywords: review removed
Owner: changed from therve to Jonathan Lange

Alright, please merge.

comment:27 Changed 14 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

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

comment:28 Changed 11 years ago by <automation>

Owner: Jonathan Lange deleted
Note: See TracTickets for help on using tickets.