Related to issue #16817 and to my efforts on a C OrderedDict, I think it would be nice to have a class decorator that handles most of the PEP 399 requirements for you.  The attached patch adds such a decorator to test.support.

One lingering doubt I have is about how I throw the two new test case classes into the globals.  They're already getting bound, as a pair, to the original test class's name...

One step I left out is handling the whole pickle/copyreg case outlined by #16817.

The decorator also mitigates the problem described in issue #16835.

Nice work, although I worry this is starting to get into "too much magic" territory.  Do you have a real use case for the 'names' argument?

I'm not sure what the use case for this is. It looks more obfuscating than revealing to me.

Antoine - I think I can see the use case. ATM, to conform to PEP399, every test _class_ has to be subclassed twice with appropriate assignment to the relevant tested class. This leads to a lot of repetition. As an example, see test_decimal.py, which does this a lot.

It would be nice to save programmers from this repetition; however, as I said before, this solution seems very complex. Maybe simpler solutions can be conceived?

I'm not sure how the "lots of repetition" is a problem. The following:

class CCoverage(Coverage):
    decimal = C
class PyCoverage(Coverage):
    decimal = P

is quite trivial compared to the actual base test case (the Coverage class). Not only it is quite trivial to *write*, but it is also very easy to *read*, and quite explicit.

Python is not Lisp, and we do not like meta-programming that much, when it tends to obscure the code in the name of not repeating yourself.

In my case I've been doing PEP 399 for collections.OrderedDict.  It struck me that the boilerplate could be stowed away in a decorator.  It's more than just adding a couple subclasses.  Here's what it covers:

* add the test case subclasses,
* make sure the original test case does not get used (#16835),
* use the relevant names as class attributes rather than globals (e.g. self.OrderedDict vs. OrderedDict),
* hack around modules that do their own imports (#16817),
* others(?).

When adapting existing test cases having a decorator to do this makes the job a snap without having to worry about corner cases.  You have to add the decorator and then add the new test names to the test discovery code (at least until we use unittest's test discovery).  For new test cases and splitting out test cases it likewise simplifies things.  On top of that, if you drop the decorator the test case would still run as-is.

Other benefits for both new and existing test cases:

* the decorator makes it clear what is going on (or will be made to) via the decorator name,
* the requisite boilerplate doesn't have to clutter up the code,
* when something like #16817 or #16835 comes up, it's much easier to update the decorator in test.support instead of finding each of the PEP-399-ized tests that is affected and updating them individually.

Downsides:

* "explicit is better than implicit",
* relatedly, the solution is more magic/obfuscated that the current boilerplate.

You could argue that the magnitude of the problem doesn't warrant a complex decorator, but it doesn't have to be hard to understand.  My patch is something I threw together late last night that could certainly be made more clear.  More importantly, as more modules get a dual implementation, the above benefits of the decorator become more pronounced.

> Do you have a real use case for the 'names' argument?

My use case was with the tests for OrderedDict.  The existing tests don't refer to collections.OrderedDict, but rather to OrderedDict (looked up from the globals).  The names argument facilitates the replacement of OrderedDict.

On Sat, Jan 26, 2013 at 9:10 AM, Eric Snow <report@bugs.python.org> wrote:

>
> Eric Snow added the comment:
>
> In my case I've been doing PEP 399 for collections.OrderedDict.  It struck
> me that the boilerplate could be stowed away in a decorator.  It's more
> than just adding a couple subclasses.  Here's what it covers:
>
> * add the test case subclasses,
> * make sure the original test case does not get used (#16835),
>

PEP 399 dictates that the base class does not inherit from
unittest.TestCase, so why would it be used?

> * use the relevant names as class attributes rather than globals (e.g.
> self.OrderedDict vs. OrderedDict),
>

Wouldn't it be better to suggest this in PEP 399? Otherwise, the proposed
decarator kind-of goes against it.

> * hack around modules that do their own imports (#16817),
>

Can you clarify how this helps? I though the decorator doesn't (yet?)
handle the problem with pickle.

> * "explicit is better than implicit",
> * relatedly, the solution is more magic/obfuscated that the current
> boilerplate.
>
> You could argue that the magnitude of the problem doesn't warrant a
> complex decorator, but it doesn't have to be hard to understand.  My patch
> is something I threw together late last night that could certainly be made
> more clear.  More importantly, as more modules get a dual implementation,
> the above benefits of the decorator become more pronounced.
>
>
I agree that it can be made clearer, but you are unlikely to lose the
metaprogramming magic.

> My use case was with the tests for OrderedDict.  The existing tests don't
> refer to > collections.OrderedDict, but rather to OrderedDict (looked up
from the globals).
> The names argument facilitates the replacement of OrderedDict.

Is it possible to just rewrite the tests to use collections.OrderedDict?
IMHO this extra 'names' argument adds a lot of magic to the decorator, and
if it's just there for a problem that's easy to avoid...

(changing the title to disassociate it from a particular solution)

>> * make sure the original test case does not get used (#16835),
>
> PEP 399 dictates that the base class does not inherit from
> unittest.TestCase, so why would it be used?

Sorry for any confusion.  In this list I was enumerating what you have to do to accommodate PEP 399, rather than any new behavior.  Hopefully I wasn't too far off. :)  I'd expect that any solution to reduce boilerplate here (like the decorator) would address these responsibilities.

>> * use the relevant names as class attributes rather than globals (e.g.
>> self.OrderedDict vs. OrderedDict),
>
> Wouldn't it be better to suggest this in PEP 399? Otherwise, the proposed
> decarator kind-of goes against it.

PEP 399 already espouses the approach of using a class attribute to hold the module under test, though it doesn't require it.  You're right that the decorator obviates the need to make that change when accommodating PEP 399.

>> * hack around modules that do their own imports (#16817),
> Can you clarify how this helps? I though the decorator doesn't (yet?)
> handle the problem with pickle.

If a boilerplate solution can make it so that we don't have to hack around these situations whenever they come up, I think it'd be a win.  The decorator currently doesn't do this, but I'd expect that any solution we accepted would.

That said, if there is a risk of something like the decorator causing other problems then the status quo is certainly better even if it's more work.  :)

>> * relatedly, the solution is more magic/obfuscated that the current
>> boilerplate.
>>
>> ...
>
> I agree that it can be made clearer, but you are unlikely to lose the
> metaprogramming magic.

Yeah, if the right balance can't be struck then I definitely agree that we're better of with the way things are.

> Is it possible to just rewrite the tests to use collections.OrderedDict?
> IMHO this extra 'names' argument adds a lot of magic to the decorator, and
> if it's just there for a problem that's easy to avoid...

That's a good idea.

Thanks for the feedback, Eli.

I think something similar has already been proposed and rejected on another issue.

> I think something similar has already been proposed and rejected on another issue.

Wouldn't surprise me.  I'll look for it.  I know there's been a bunch of traffic relative to PEP 399 and to test discovery, so I'm guessing I missed it in there.  Thanks for the heads up.

OK, let's take a step back here and look at what exactly we are trying to simplify (which is now the updated example in PEP 399)::

    from test.support import import_fresh_module
    import unittest

    c_heapq = import_fresh_module('heapq', fresh=['_heapq'])
    py_heapq = import_fresh_module('heapq', blocked=['_heapq'])


    class ExampleTest:

        def test_example(self):
            self.assertTrue(hasattr(self.module, 'heapify'))


    class PyExampleTest(ExampleTest, unittest.TestCase):
        module = py_heapq


    @unittest.skipUnless(c_heapq, 'requires the C _heapq module')
    class CExampleTest(ExampleTest, unittest.TestCase):
        module = c_heapq


    if __name__ == '__main__':
        unittest.main()


Ignoring the unittest.main() boilerplate, we have two import_fresh_module() calls and the creation of two subclasses of ExampleTest, both of which inherit from unittest.TestCase and one of which is skipped if the acceleration code is lacking. So there is some boilerplate. The question is whether a solution be made that isn't too magical to minimize this code.

In my head I see this becoming something more like::

    from test.support import PEP399Tests

    pep399_tests = PEP399Tests('heapq', '_heapq')

    class ExampleTest:

        def test_example(self):
            self.assertTrue(hasattr(self.heapq, 'heapify'))

    PyExampleTest, CExampleTest = pep399_tests.create_test_cases(ExampleTest)


    if __name__ == '__main__':
        unittest.main()


This would cut out the import_fresh_module() calls (which don't need to be injected globally as you should be accessing the code through the test class' attribute storing the module and if you don't care what version you get you just do a standard imoprt), remembering to subclass unittest.TestCase, and to skip the accelerated version of the tests if it isn't needed. Basically it goes from 7 lines to 2 with not repetitious lines. You could make this a decorator, but honestly it's the same number of lines without the magic of mucking with a module's globals by getting at them through sys.modules (the decorator really only saves you from typing the variable names to hold the new test classes and the test class argument to begin with).

And the implementation should be relatively straight-forward (thanks to Eric having done most of the thinking on what needs to happen)::

  class PEP399Tests:
    # Using keyword-only arguments you could go as far as allowing for
    # customizing the attribute name to set, class name prefixes, etc.
    def __init__(module_name, *accelerated_names):
      self.module_name = module_name
      self.py_module = import_fresh_module(module_name, fresh=accelerated_names)
      self.accelerated_module = import_fresh_module(module_name, block=accelerated_names)

    def create_test_cases(self, test_class):
      class PyTestCase(test_class, unittest.TestCase): pass
      PyTestCase.__name__ = 'Py' + test_class.__name__
      setattr(PyTestCase, self.module_name, self.py_module)
      if self.accelerated_module is None:
        AcceleratedTestCase = None
      else:
        class AcceleratedTestCase(test_class, unittest.TestCase): pass
        AcceleratedTestCase.__name__ = 'Accelerated' + test_class.__name__
        setattr(AcceleratedTestCase, self.module_name, self.accelerated_module)
      return PyTestCase, AcceleratedTestCase


Does this approach seem more reasonable to people in terms of cutting down boilerplate without being too magical?

Note that while this works for the simple case, it doesn't work whenever you need additional tests or attributes that are specific for one of the two classes, or when you have additional base classes.  This is also not so uncommon.  I think the skip on the accelerated version might not be necessary in some situations.

IOW we have to trade between compactness, flexibility, and readability, and trying to improve one side will likely make other aspects worse.

I don't think there would be any problem at grouping the two import_fresh_module() in a single call that returns the two modules though.  (I would also avoid the use of pep399 in the names -- it's not really readable unless you can map pep numbers with their titles.)

Brett - you approach is certainly less magical. While you're at it, how about PEP 399 recommending the import_fresh_modules calls not being in the global context, but rather class/instance attributes of the test cases? This is what both yours and Eric's decorator do, and it's better common sense.

That wouldn't work if there is a skip decorator that checks that the C module is available :)

FWIW, I like Brett's approach much better.
Also, I agree with Ezio that "pep399" shouldn't be in the name.

> That wouldn't work if there is a skip decorator that checks that the C
module is available :)

Yes, but that's easily amended with just raising SkipTest in the setUp
instead. Not polluting the global namespace has its merits, especially when
the import sequence is so funky you want to clearly separate testing of the
two modules.

That would work if we use something Brett's proposal, so that we can create a default setUp that raises SkipTest.  My comment was about using module = import_fresh_module() directly in the class body of the subclasses as they are currently documented in PEP 399.
The point is that, while Brett's proposal doesn't look too bad, I'm not sure it's flexible enough.

On Sun, Jan 27, 2013 at 3:13 PM, Ezio Melotti <report@bugs.python.org>wrote:

>
> Ezio Melotti added the comment:
>
> Note that while this works for the simple case, it doesn't work whenever
> you need additional tests or attributes that are specific for one of the
> two classes,

Sure it does. The modules are simply attributes on the PEP399Test instance,
so you can still access the modules just as easily in test classes that
don't need to be tested with both possible modules.

or when you have additional base classes.

That can easily be solved with the argument to create_test_cases(), e.g.
``def create_test_cases(*bases_classes): ... class PyTest(*(base_classes +
[unittest.TestCase])): pass``.

>  This is also not so uncommon.  I think the skip on the accelerated
> version might not be necessary in some situations.
>

Then don't use this approach. It doesn't have to apply to every single test
class in a module. It just needs to help in the simple case.

>
> IOW we have to trade between compactness, flexibility, and readability,
> and trying to improve one side will likely make other aspects worse.
>

There is no trade-off. Either you can use this approach or you simply don't
and use the slightly longer form. This doesn't mean the current approach we
use can't continued to be used, just that the simple case can be made
really simple.

> I don't think there would be any problem at grouping the two
> import_fresh_module() in a single call that returns the two modules though.

And that can still be a useful thing for the class; for test modules whose
needs are simply too complicated then the instantiated instance can be
nothing more than a streamlined way of getting at both modules.

>  (I would also avoid the use of pep399 in the names -- it's not really
> readable unless you can map pep numbers with their titles.)

I came up with the name on the fly when I could think of anything better in
5 seconds. =) I assumed a different name would be used if this approached
was decided upon and went into test.support.

>> This is also not so uncommon.  I think the skip on the accelerated
>> version might not be necessary in some situations.
>
>Then don't use this approach. It doesn't have to apply to every single >test class in a module. It just needs to help in the simple case.

What I was trying to say is that since the special cases are not so uncommon, we might not have many simple cases where this idiom could be used to save a few lines, and we will end up with two different idioms for the same thing.

To prevent speculation, the files that mention import_fresh_module are:

# Without changes to test classes
Lib/test/test_bisect.py: would work if you made the attribute configurable (e.g. self.module instead of self.bisect
Lib/test/test_heapq.py: would work
Lib/test/test_warnings.py: would work if you made the attribute configurable

# With minor changes to test classes
Lib/test/json_tests/__init__.py: would work if you took out the function accessing shortcuts (e.g. self.json.loads instead of self.loads)
Lib/test/test_functools.py: would work if you took out the function accessing shortcuts

# Won't work
Lib/test/test_datetime.py: has its own crazy version of this approach (although it sounds like part of it is because of problem with import_fresh_module and other things that might need to be fixed)
Lib/test/test_decimal.py: won't work because of fractions modules
Lib/test/test_xml_etree.py: doctest-based
Lib/test/test_xml_etree_c.py: specific to accelerated version

# Doesn't apply
Lib/test/support.py: because it defines it =) 
Lib/test/test_bz2.py: used in a single test method
Lib/test/test_importlib/test_api.py: used in a single test
Lib/test/test_support.py: obvious =)


So out of 9 test modules it will work with 5 of them (2 with either search-and-replace changes to the test code or making the proposed solution more flexible) and not w/ 4 (or 3 if you don't count the etree tests as separate, and the test_datetime seems weird as to why it's so complicated based on the comments in the code). So overall it still seems like a win to me, especially if we keep adding more test modules that need this approach.

I took a closer look at test_datetime and it probably should be using this approach, but because the tests seemed to have been copied from Zope the tests were left as-is and instead the testing approach we have been using was actually forced upon the test module instead of datetime. So it would probably work with some work to refactor the code to use it.

Some of the exceptions among the modules you listed are:

Lib/test/test_warnings.py:420 has a class specific for the C module.

Lib/test/json_tests/test_dump.py:34 has an additional test specific for the C module.
Lib/test/json_tests/test_speedups.py is specific for the C module.

Lib/test/test_functools.py:199 is specific to the Python module.
Lib/test/test_functools.py:207 different tests depending on the version.

Unless these additional methods are defined separately and somehow added to the two classes returned by pep399_tests.create_test_cases(ExampleTest), it would be necessary to create additional classes using the current idiom and then get the modules from pep399_tests.cmodule/pep399_tests.pycmodule, e.g.:

# the two "normal" tests
TestPyDump, TestCDump = pep399_tests.create_test_cases(DumpTest)

# the additional test for the C module
@unittest.skipUnless(pep399_tests.cmodule, 'requires _json')
class TestCDump2(unittest.TestCase)
    module = pep399_tests.cmodule
    @bigmemtest(size=_1G, memuse=1)
    def test_large_list(self, size):
        ...

FWIW I won't change the json tests, because I already wrote them in a way that most of the code is defined in __init__.py and shared in all the other test files.

etree's doctest tests are a painful relic of the past, which will hopefully go away one day (issue #15083), at which point conformance to PEP 399 shouldn't be an issue.

More generally, and this applies to etree's test too, a lot of trouble is being caused by pickle (see etree's tests and decimals's tests). If we're on a way to actually commit a decorator for PEP 399, perhaps this issue can somehow be addressed in a generic manner.

True, the current idiom needs to still be used in those cases, although we could introduce another method to help with this situation as well:

# Could also be named use_accelerator to be less hostile-sounding.
def requires_accelerator(self, cls):
  if self.accelerated_module is None:
    raise SkipTest  # With proper message
  else:
    setattr(cls, self.module_name, self.accelerated_module)
    return cls

Then the idiom becomes:

  @pep399_tests.requires_accelerator
  class AcceleratorSpecificTests(unittest.TestCase): pass


This then extends out to also the current idiom if you don't want to have any "magical" classes:

  @pep399_tests.requires_accelerator
  class AcceleratedExampleTests(unittest.TestCase): pass

  # Can add another decorator for this if desired.
  class PyExampleTests(unittest.TestCase):
    module = pep399_tests.py_module


This also has the benefit of extracting out the module attribute name to minimize messing even that up.

+1 to where Brett's taking this.  I really like having the PEP399Tests class encapsulating the various boilerplate parts, with some of them as methods, rather than trying to pack them all into one all-powerful decorator.

It would be worth raising an exception in ``pep399_tests.create_test_cases(ExampleTest)`` if ExampleTest inherits from unittest.TestCase.  This is a simple thing that would help people get it right.

FWIW, here's a little more explanation on my original thoughts, none of which I'm married to.

Most of the magic in my patch came messing with the globals (swap out the module and module attrs; add in the two new classes; ditch the original class).  That was a consequence of my remaining objectives.

I was also following the lead of decorators by having the the boilerplate before the test case definition rather than split up before and after.

Finally, I was trying to do it in a way that existing test cases, like TestOrderedDict, would need as little changes as possible.

This leads to one bit of boilerplate that I'm not a fan of: the whole self.<module> thing.  For TestOrderedDict, it meant changing all uses of an OrderedDict global to self.collections.OrderedDict.  In fact, that was essentially the biggest motivation for the "magic" in the decorator.  However, I can live with the boilerplate--I'll just deal with the search-and-replace (not the end of the world).

+1 on requires_accelerator().  It could also be used for individual test methods.  Similar decorators-as-methods could be added later, where appropriate, for other special cases, like handling the pickle situation described in #16817.

Any objections to what Brett proposed?  Any preferences on a name?  How about DualImplementationTests?  Here's a patch with tests.

I added a pure_python_only() decorator in addition to accelerated_only().  I also made both work as decorators for classes and methods (just like unittest.skip()).

Finally, I added on a with_module_exported() decorator to help with the scenario from issue #16817.  Eli, I know it doesn't quite fill the gap you described in msg180669, but hopefully it's an improvement.

I'm still a bit skeptic about this.

To summarize my position, I am:
+0 about adding something like this to test.support and use it for new tests;
-1 about mass-rewriting the existing tests to use this (in particular for test_json);
+1 about simplifying the API of import_fresh_module so that it returns both the modules and doesn't require the fresh/blocked args;

On Wed, Feb 6, 2013 at 4:17 AM, Ezio Melotti <report@bugs.python.org> wrote:

>
> Ezio Melotti added the comment:
>
> I'm still a bit skeptic about this.
>
> To summarize my position, I am:
> +0 about adding something like this to test.support and use it for new
> tests;
> -1 about mass-rewriting the existing tests to use this (in particular for
> test_json);
> +1 about simplifying the API of import_fresh_module so that it returns
> both the modules and doesn't require the fresh/blocked args;
>

I share Ezio's sentiment regarding the new proposal(s). As for
import_fresh_module, if it's only ever used to do what we describe here,
then I propose to rename it to something more descriptive and indeed make
it just return both modules. If it's being used for other stuff, we can add
another function and rewrite all the tests to use that.

py_module, c_module = import_python_module_and_accelerator('module',
'_module')

First of all, I agree that the API for import_fresh_module() isn't ideal and could stand some improvement.  I've created issue 17146 for pursuing that avenue.

More importantly for this issue, simplifying import_fresh_module() is not nearly enough of a win for me regarding PEP 399.  My big motivator here was to make complying with PEP 399 much easier.  That encompasses more than eliminating the extra call to import_fresh_module().

There are a number of considerations that I've already outlined, which especially relate to maintenance and writing new dual-implementation tests.  Without a comprehensive helper, existing tests don't automatically benefit from a better way of doing it (or fixes); converting existing tests to dual-implementation isn't as simple; and when someone goes to write new dual-implementation tests they have to do all the boilerplate and cover the corner cases (e.g. pickle-related tests).

I'm coming at this from the perspective of someone who hadn't already written or adjusted tests to accommodate PEP 399.  Having a more comprehensive helper would have saved me the time to figure it out, which I count as somewhat wasted since it's just boilerplate.  I'd rather be working on the tests and the code that I'm testing.  I'd rather not ask anyone else to take the time to get familiar with the boilerplate when I'm sure they'd rather be working on their problem than shaving this yak. :)

For the record, I'm glad we have PEP 399 and that we are making the effort to get more pure Python modules in the stdlib.  A big thanks to Brett for continuing to push for this!

Ezio, I'm on board with your summary, though I'm +1 on adding the helper. :)  I didn't have any plans to refactor any tests, but I would like to use any helper that might come out of this discussion in my testing of OrderedDict.

At this point let's just start with the helper class which takes the arguments as necessary to do the proper importing of both the pure Python and accelerated versions of the module and optionally the name of the attribute the test classes expect (otherwise just use the name of the module itself). Then expose two decorator methods to use on subclasses to set the proper class with the proper attribute name. Going fancier with a method that generates the subclasses can come in a separate patch. import_fresh_module() can stay as-is and this class just becomes the preferred way to get both versions of a module at the same time.

Fine with me.

The gains here aren't worth the hassle in my mind, and I've lost interest in pushing this forward any further.  If anyone else wants to pick this up, feel free to re-open the issue.