A common pattern in testing is to have a base test class that implements test methods, and subclasses that provide various data that drives the tests to be run in different ways.  It is convenient for the base class to inherit from TestCase, but this causes the normal test loader to load it as a test to be run, when most times it can't run by itself.

My proposed solution is to make test case loading depend on an attribute of the class rather than the fact that it subclasses TestCase.  TestCase would then have the attribute set to True by default.  A decorator would be provided that sets the attribute to False, since that would make it visually obvious which TestCases are base classes and not to be loaded.

(Note: once this is available the 'best practices' description of test file construction in the documentation for the stdlib 'test' module should be updated to use it.)

> A decorator would be provided that sets the attribute to False, since
> that would make it visually obvious which TestCases are base classes
> and not to be loaded.

What's the point? Just derive from TestCase in the derived classes, not the base classes.
-1 on useless complication.

Wouldn't the subclass inherit the False value?  Then the user would need to remember to override the value again in the subclass, which is error prone.

Antoine: I've used the pattern you describe on a couple of occasions, and it routinely confuses my code reviewers.

Antoine: I don't have any problem with that personally, but Michael did, and he's the maintainer :)

But there is a small advantage: it means you don't have to keep repeating the 'unittest.TestCase' boilerplate in each subclass declaration, you only have to decorate the base class once.

Daniel:  Oops, you are right. Michael seemed to have some idea on how to implement the decorator.  The class attribute was my contribution, and obviously that isn't going to work.  I wanted something more transparent than a magic decorator, but it looks like magic will be required.

Which, IMO, is a point in Antoine's favor.  Let's see what Michael has to say.

> Antoine: I've used the pattern you describe on a couple of occasions,
> and it routinely confuses my code reviewers.

Really? What is confusing about it?
Perhaps we should simply document it.

So the technique I suggested is that the TestLoader checks classes for the "testbase" (or whatever we call it) *in the class dict*. So inheritance doesn't matter - a class is only excluded from test loading if it has the attribute directly.

Here are my objections to the standard (but not widely used outside our own tests) mixin pattern for base testcases (copied and pasted from issue 14408):

Because you then have classes that inherit from object calling methods that clearly don't exist (until you subclass them *and* TestCase). It looks weird and also means the classes can't be tested in isolation.

With a class decorator the code would *look* straightforward, and the hidden attribute is just an implementation detail.

It still looks weird to see code calling methods that obviously don't exist, and with no indication *at the call site* where they come from. Making it clearer with naming would help: "TestThingMixin" or similar.

There are classes like this in the unittest test suite, and I was very confused by them initially until I found where and how they were used. It is obviously *not* a pattern that is widely known for test base classes, as we have this problem of it not being done even in the standard library tests.

In contrast I think code similar to the following would be clear and readable without knowing about multiple inheritance and the mixin trick:

    @test_base_class
    class SomeTestBase(TestCase):
        ...

Besides which, the mixin pattern won't *stop* working if we provide this extra functionality - it would just be an alternative for those (like myself) who think it impedes code readability. :-)

At this point we're off topic for the *specific issue*, and I'm fine with our own standard library tests moving to use mixins to support standard unittest invocation. I would suggest the base test cases include Mixin in their name to make it clear how they should be used.

> So the technique I suggested is that the TestLoader checks classes for
> the "testbase" (or whatever we call it) *in the class dict*. So
> inheritance doesn't matter - a class is only excluded from test
> loading if it has the attribute directly.

Why not document the official recommendation in the docs?
Adding another gimmick isn't very useful. It doesn't add a
functionality. It doesn't even make it significantly easier to write
tests (unless you have more test classes than test methods). And it
means that people have to remember two idioms instead of one.

Note that I did just document the mixin idiom in the Lib/test docs.  Which core developers probably don't read :)

FWIW I use the mixin approach too and find it simple and clean.  I don’t have a problem with a method in the mixin class calling methods from TestCase.

+1, we already have such decorators for individual test cases. Code should be obvious, particularly testing code and mixins often aren't. Magic such as identifying classes to run by their type should be over rideable. All magic should.

Do we have a consensus as to whether or not David's proposed solution is acceptable?

It's Michael's solution that is under consideration, and I guess no one has felt strongly enough about having it to write a patch.  So this issue stays in limbo until someone does.

What if we simply rename the current unittest.TestCase class to unittest.BaseTestCase, and define unittest.TestCase as "class TestCase(BaseTestCase): pass"?  Then mixin classes can derive from BaseTestCase and have all of the TestCase methods available (for auto-completion, etc.), but won't be picked up by discovery.  Real test classes would derive from TestCase as usual (but would still have to do so explicitly when also using a mixin), and all current code should work without modification.

If you're writing a mixin you don't need to derive from TestCase, you just derive from object. The point of this feature is to allow TestCase subclasses to be "base classes" instead of being used as a mixin.

Right, but in my experience, it seems like a lot of people do inherit from TestCase in their mixin anyway, possibly just to have the TestCase methods available for auto-completion in their IDE.  I've done the same, though I've remembered after writing the tests that I could remove TestCase as a base on the mixin.

On the other hand, I may be getting "base class" and "mixin" confused in this context and completely misunderstanding some aspect of the issue :)

Anyway, my suggestion was just the simplest change I could come up with makes the situation better from my point of view, but I do actually like the decorate-the-base-class method better anyway.

Michael's suggestion looks too sophisticated and confusing to me.

My suggested solution is a class decorator you use on your base class that means "don't run tests from this class". 


@unittest.base_class
class MyTestBase(TestCase):
    pass

Not quite sure how that's sophisticated or confusing... Are you confusing the way it's used with my suggested implementation?

Zachary: Inheriting from TestCase in your mixin is actually an anti-pattern. And yes, most people seem to do it (most people seem to misunderstand how to use mixins, which is why I like this approach of having a way of explicitly marking base classes).

Class decorator approach looks less obvious to me. Looking at current standard mixing approach I understand what it do and why. But encountering the unittest.base_class decorator, I need to look in the documentation and/or sources to understand how it works.

IMHO this decorator adds a burden for readers (which needs to learn yet one thing) and adds only small benefit for whose writers which use IDE (not me).

To be honest, even though I understand it, I find the mixin pattern hard to read. You derive from object, but call methods (the assert methods for example) that don't exist in the class (or its inheritance chain).

My experience, with even experienced Python devs, is that they don't *properly* understand how to create mixins and they *much prefer* to write base classes that derive directly from TestCase. And then you have the problem that this issue is intended to resolve (and that we even have in the Python test suite).

Just want to restate my +1 for Michael's idea.  I'm hit by this all the time and it is beautiful and expressive.  It also does not preclude the annoying mix-in way of doing it.

Note that we have to use mixin approach in the Python test suite to avoid 
discrepancies and merge conflicts between different versions.

Issue 19645 is an alternative way to solve the problem of calling methods on the mixin that "don't exist", and has other benefits.

+1 on the idea.

While the mixin method works and it's not overly complex, it might not be immediately obvious to those unfamiliar with it and to those reviewing the code (i.e. there's no clear hint about the reason why the base class doesn't inherit from TestCase directly).  Using mixins also adds duplication and might results in tests not being run if one adds a new class and forgets to add TestCase in the mix.

A decorator would solve these problems nicely, and a bit of magic under the hood seems to me an acceptable price to pay.

We made a patch for this issue at Bloomberg's Python Open Source Event. Please find the diff here: 

https://bitbucket.org/hllowrld/cpython/commits/382773b2611a86326568a22dd5cef6c7f7bae18c

Zach Ware will review the patch.

Attaching Victor and Lisha's patch in reviewable form.

Victor: I've left a review on Rietveld; it should have sent you an email with it.  The basic change looks good to me, but there's some cleanup that will need to happen before it can be committed, and Michael will need to confirm that this does what he was expecting.

Hi Zach,

I've pushed a fix here according to your suggestions:
https://bitbucket.org/hllowrld/cpython/commits/fe10b98717a23fd914c91d42dcca383d53e924a8

Please also find attached the diff.

Victor: Sorry for the delay in getting back to this.  I'm attaching your full patch again; the diff you posted is a diff against your first patch, while what we need to be able to review it properly is a diff against the tip of the 'default' branch.  You may be able to make such a diff by pulling from http://hg.python.org/cpython and then doing "hg diff -r default Lib/unittest", but that may still not come up with the right changes.  I would suggest stripping your repository of any changesets on default branch that are not present on http://hg.python.org/cpython#default, and recommitting your changes on your own 'issue14534' branch, then you can make a proper patch just by doing "hg diff -r default" (assuming your branch is up to date with default).

Hi Zach,

I've pulled from the default branch. Please find attached the diff for "hg diff -r default Lib/unittest" in the attached "issue14534.v3.diff".

Victor

I left a few comments on rietveld.
The patch should also include documentation.

> Class decorator approach looks less obvious to me. [...]
> But encountering the unittest.base_class decorator, I need to look in
> the documentation and/or sources to understand how it works.

I also agree that seeing @testBaseClass (the current name in the patch) gives very little indication of what's going on, but perhaps that could be fixed with a better name.
The two importants things are that the class is ignored/skipped, and that this doesn't propagate to the subclasses (as one might expect).
Would something like @ignoredBaseClass/@skippedBaseClass be better?  Another options could be a more explicit @dontRunBaseClassTests, even though I don't particularly like the name.

About the name: abstract_tests could be used e.g.:

  @abstract_tests
  class AbcSetTests(TestCase):
      # test abc.Set
      Set = abstract_property()

      def setUp(self):
          self.set = self.Set('abc')

      def test_difference(self):
          self.assert... 
    
      def test_union(self):
          self.assert...

It is clear that AbcSetTests do not run because the class is abstract
(can't create an instance).  It is clear that to create a concrete test,
you need to subclass the abstract class (serve as a base class):

  class BuiltinSetTests(AbcSetTests):
      # test builtins.set
      Set = set

  class FrozenSetTests(AbcSetTests):
      # test frozenset
      Set = frozenset

It might not be necessary to enforce all abstract constraints in the
implementation initially. The only thing that needs to be implemented is
that tests from @abstract_tests decorated class should not be run.

"abstract_test_class" anyone?
(or "abstractTestClass" if that looks more consistent)

(not that I would need it myself!)

abstractTestClass/abstractBaseClass were up next in the list of names that I was considering, but they are not too explicit about the fact the the test case will get ignored/skipped (especially if one is not familiar iwth the concept and behavior of abstract class).
They are not too obscure either though, so those would be fine with me.

testBaseClass, abstractTestClass and abstractBaseClass are all fine with me. I wouldn't waste too much time bike-shedding it. If we need a decision let's go with abstractTestClass.

SGTM.
The patch still needs docs and there are some comments on rietveld.
The name of the decorator should be updated too, and possibly __unittest_base_class__ should be renamed to __unittest_abstract_class__ to match the name of the decorator.

So I've two more cases for this that I think we need to ensure works.

Firstly FunctionTestCase should be blacklistable, and its not abstract.

Secondly we're going to want nose, unittest2 etc to be able to also honour this. I suspect that this is easy and may not require any changes, but having a think about it would be good.

Removed the bogus huge diff.

I'm going to review on rietvald - I see a lot of changes needed - sorry - and some are a bit bikesheddy. But, if we do them I'll commit it asap and do any final fixup needed.

Issue 22680, about blacklisting FunctionTestCase, was closed as a duplicate of this. However the patch there has a useful test case for FunctionTestCase that could be merged with any future work here.