Opened 13 years ago

Last modified 11 years ago

#2645 defect new

trial runner does not recognize class based decorators

Reported by: synapsis Owned by: kelly
Priority: normal Milestone:
Component: trial Keywords:
Cc: synapsis, Jean-Paul Calderone Branch: branches/class-based-decorators-2645
branch-diff, diff-cov, branch-cov, buildbot
Author: jml

Description

I'm using Twisted 2.4 with Python 2.4.4 on Debian Etch.

If I decorate a test case with a class based decorator, trial does not recognize it.

I have attached the test script.

Here is the problem:

$trial trial_decorator.TestCase.test_Answer
Traceback (most recent call last):
  File "/usr/local/bin/trial", line 24, in ?
    run()
  File "/usr/lib/python2.4/site-packages/twisted/scripts/trial.py", line 373, in run
    suite = _getSuite(config)
  File "/usr/lib/python2.4/site-packages/twisted/scripts/trial.py", line 332, in _getSuite
    suite.addTest(loader.loadByName(test, recurse))
  File "/usr/lib/python2.4/site-packages/twisted/trial/runner.py", line 346, in loadByName
    return self.loadAnything(thing, recurse)
  File "/usr/lib/python2.4/site-packages/twisted/trial/runner.py", line 339, in loadAnything
    raise TypeError("No loader for %r. Unrecognized type" % (thing,))
TypeError: No loader for <trial_decorator.logger object at 0xb77933cc>. Unrecognized type

Attachments (6)

trial_decorator.py (701 bytes) - added by synapsis 13 years ago.
trial_decorator.2.py (1.2 KB) - added by synapsis 13 years ago.
correct implementation of the decorator
AddMethodNamesToDictTests.patch (4.9 KB) - added by kelly 11 years ago.
tests for twisted.python.reflect.addMethodNamesToDict
TestMethodDescriptors.patch (1.5 KB) - added by kelly 11 years ago.
changes twisted.python.reflect.addMethodNamesToDict to look for methods using getattr
max-test-discovery.patch (3.0 KB) - added by kelly 11 years ago.
test method objects are no longer required to have an im_class property
review-1.patch (19.0 KB) - added by kelly 11 years ago.
Fixes outstanding review issues (I hope)

Download all attachments as: .zip

Change History (25)

Changed 13 years ago by synapsis

Attachment: trial_decorator.py added

comment:1 Changed 13 years ago by synapsis

Cc: synapsis added

comment:2 Changed 13 years ago by synapsis

Summary: trial runned does not recognize class based decoratorstrial runner does not recognize class based decorators

comment:3 Changed 13 years ago by Jean-Paul Calderone

This doesn't work with Python, either. Methods can only be implemented with functions, not arbitrary callable objects.

However, current Twisted trunk@HEAD skips the test entirely. Instead, it should try to call the method, fail with a TypeError (not the one in the description above, but one resulting from not calling the function with enough arguments), and report this error in the usual way.

If you want logger to actually be a usable decorator, you should give it a __get__ method which binds it to the instance with an instancemethod.

comment:4 Changed 13 years ago by Jean-Paul Calderone

Err, first I said you can only implement methods with functions, then I explained that you can use descriptors to implement methods. The first of these two statements is, clearly, incorrect. Please disregard it. :)

comment:5 Changed 13 years ago by jknight

In particular, you need to add:

    def __get__(self, obj, objtype=None):
        return types.MethodType(self, obj, objtype)

to your class definition.

(but trial still won't work with it).

comment:6 Changed 13 years ago by Jean-Paul Calderone

Oh hey, I see there are different behaviors based on different trial command-lines too. So maybe there are two separate issues here.

Changed 13 years ago by synapsis

Attachment: trial_decorator.2.py added

correct implementation of the decorator

comment:7 in reply to:  3 Changed 13 years ago by synapsis

Replying to exarkun:

This doesn't work with Python, either. Methods can only be implemented with functions, not arbitrary callable objects.

However, current Twisted trunk@HEAD skips the test entirely. Instead, it should try to call the method, fail with a TypeError (not the one in the description above, but one resulting from not calling the function with enough arguments), and report this error in the usual way.

If you want logger to actually be a usable decorator, you should give it a __get__ method which binds it to the instance with an instancemethod.

Yes, you are right, sorry.

I just forgot to add it, however the problem is the same.

I have updated the attached test script.

comment:8 in reply to:  description Changed 11 years ago by kelly

Part of the cause for this lies in twisted.python.reflect.addMethodNamesToDict. It iterates through the class __dict__ looking for things that are FunctionType. In the example the __dict__ entry for test_Answer is a logger.

First patch adds tests for twisted.python.reflect.addMethodNamesToDict to demonstrate the problem.

Second patch changes twisted.python.reflect.addMethodNamesToDict to look for methods using getattr (to trigger descriptor __get__).

At this point descriptors which return MethodType objects from __get__ work.

Third patch modifies twisted.trial.unittest.TestCase.__init__ so that test method objects are no longer required to have an im_class property. This fixes the original problem.

Changed 11 years ago by kelly

tests for twisted.python.reflect.addMethodNamesToDict

Changed 11 years ago by kelly

Attachment: TestMethodDescriptors.patch added

changes twisted.python.reflect.addMethodNamesToDict to look for methods using getattr

Changed 11 years ago by kelly

Attachment: max-test-discovery.patch added

test method objects are no longer required to have an im_class property

comment:9 Changed 11 years ago by kelly

Keywords: review added

comment:10 Changed 11 years ago by Jonathan Lange

Author: jml
Branch: branches/class-based-decorators-2645

(In [27383]) Branching to 'class-based-decorators-2645'

comment:11 Changed 11 years ago by Jonathan Lange

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

Hi Kelly,

Thanks for contributing this patch. I have a couple of questions about it that I'd like resolved before it lands. I've only had a shallow look, so please bear with me if I ask something stupid.

I notice that your initial patch is supposed to reproduce the problem. However, when I apply the patch, I don't see any test that actually fails. How is it reproducing the problem?

Thank you so much for adding tests, btw. It makes it a lot easier to get the patch landed. I have to say that I don't understand the motivation for a lot of these tests. Could you please add docstrings to each that explain the behaviour that each of them is checking? e.g.

    def test_no_prefix_methods(self):
        """
        When there are no methods that match the given prefix, nothing is added 
        to the dict.
        """

A bit redundant for that method perhaps, but very useful for things like test_all.

Similarly, each of the example classes that you add should have docstrings explaining how they are actually used and why they are needed.

Comments should in general be sentences, e.g.

# Not a method.

rather than

# not a method

Although, better still would be a comment explaining why you've got something that's not a method. Most of the people reading this will be able to infer that 42 isn't actually a method. ;) Similar reasoning applies to "create a bunch of methods various ways".

It would also be worthwhile having an explanatory comment before the two for blocks that generate tests. They are reasonably arcane, so it would be good to explain what they are actually testing and why we're generating tests this way rather than writing them the old-fashioned way.

Using hasattr(method, '__call__') seems a bit dodgy to me. Why not use callable?

In twisted/trial/util.py, DUMMY should be named _DUMMY, since it's not intended for export.

Also in twisted/trial/util.py, could you please update the docstring of getPythonContainers to document the new parameter? While you're there, I'd appreciate it if you could also put the docstring into the correct format.

There are a few places where the branch doesn't comply with the coding standard. In particular, we deviate from standard Python practice in that there must be two blank lines between methods and three blank lines between top-level blocks. You don't have to address all of these issues yourself; I can do some cleanup before landing. However, if you're planning on submitting patches in the future, it'd be good to get familiar with the coding standard, since it also

I'm also a little nervous as to how the change to unittest.py will affect old-style classes. Using type(self) seems wrong for them, since it will always return <type 'instance'>.

I'd really appreciate it if you could add a test that would fail without the change you made to unittest.py.

Phew. I hope that's not too much. Sorry for taking so long to get back to you.

And thanks again!

jml

comment:12 Changed 11 years ago by Jonathan Lange

therve says use getattr(f, '__call__', None) is not None rather than hasattr(f, '__call__') or callable.

comment:13 in reply to:  11 Changed 11 years ago by kelly

Replying to jml:

I notice that your initial patch is supposed to reproduce the problem. However, when I apply the patch, I don't see any test that actually fails. How is it reproducing the problem?

Oops. Sorry, that patch doesn't really demonstrate the problem so much as the current behaviour. Essentially it asserts that the types of test methods that work, work and the types that don't work, don't work. The idea was that these tests could be added regardless of whether the later patches were deemed appropriate. It seemed logical at the time, less so now.

It would also be worthwhile having an explanatory comment before the two for blocks that generate tests. They are reasonably arcane, so it would be good to explain what they are actually testing and why we're generating tests this way rather than writing them the old-fashioned way.

I've since found a better way to create those tests which should be considerably less arcane.

Using hasattr(method, '__call__') seems a bit dodgy to me. Why not use callable?

callable is removed in 3.0 I figured it would be good to be forward compatible.

I'm also a little nervous as to how the change to unittest.py will affect old-style classes. Using type(self) seems wrong for them, since it will always return <type 'instance'>.

Using self.__class__ should fix that. I believe it does the same thing for both new and old classes.

I'd really appreciate it if you could add a test that would fail without the change you made to unittest.py.

Phew. I hope that's not too much. Sorry for taking so long to get back to you.

Thanks for the feedback. I'll resubmit the patch once I've fixed everything. Should it still be against trunk or against the branch? And should I break the tests out into a separate patch again?

comment:14 in reply to:  12 Changed 11 years ago by kelly

Replying to jml:

therve says use getattr(f, '__call__', None) is not None rather than hasattr(f, '__call__') or callable.

Seems a bit circuitous, what's the reason behind that?

Changed 11 years ago by kelly

Attachment: review-1.patch added

Fixes outstanding review issues (I hope)

comment:15 in reply to:  11 Changed 11 years ago by kelly

Replying to jml:

I'm also a little nervous as to how the change to unittest.py will affect old-style classes. Using type(self) seems wrong for them, since it will always return <type 'instance'>.

Added some tests under twisted.trial.test.test_tests.TestInit.

test_absent_im_class fails spectacularly before the patch.

I also added test_parents_oldstyle with the intention of demonstrating the negative effect using type(self) would've had, at which point I noticed that t.t.unittest.TestCase has been a new style class since r12088.

comment:16 Changed 11 years ago by kelly

Keywords: review added

comment:17 Changed 11 years ago by Jonathan Lange

Owner: changed from kelly to Jonathan Lange

comment:18 Changed 11 years ago by Jean-Paul Calderone

(In [27458]) Apply review-1.patch

refs #2645

comment:19 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: changed from Jonathan Lange to kelly
  1. Some coding standard issues:
    1. The patch adds some trailing whitespace in a few places. You can fix this in emacs with M-x whitespace-cleanup. And you can customize show-trailing-whitespace to t to make it obvious when this is necessary.
    2. Test methods should be named like test_fooBar. This is a consequence of two things:
      1. _ is used in Twisted to separate a prefix from a dynamically discovered suffix.
      2. Functions and methods are named with camelCase.
    3. assertEquals is preferred over assertEqual
    4. Operators should have whitespace around them, eg 1 + 1 rather than 1+1. Also foo: bar rather than foo:bar.
  2. In twisted/trial/test/test_tests.py
    1. The Test that prefix in the test_absent_im_class docstring is redundant.
    2. The test_shared docstring is missing a word or two, I think.
  3. In twisted/test/test_reflect.py
    1. In the comment explaining prefix_method_descriptor_method, it's probably worth mentioning how it differs from prefix_def_method as well (ie, that looking it up in the __dict__ directory results in an object of a different type)
    2. The docstring for test_base_match_one says ancestor where I think it means descendant.
    3. Overall, reading this code, it's hard to stop thinking about how bad a function addMethodNamesToDict is. I think it may be worth leaving it unmodified, opening a ticket for its deprecation, and adding the necessary logic to make trial happy to a new function which doesn't have all the weird baseClass behavior that addMethodNamesToDict has. prefixedMethodNames is really the API most people want, anyway.
  4. twisted/trial/util.py
    1. The getPythonContainers change could probably use a direct unit test (twisted/trial/test/test_util.py seems like the place for such things)

Thanks!

Note: See TracTickets for help on using tickets.