classification
Title: Add means to mark unittest.TestCases as "do not load".
Type: enhancement Stage: needs patch
Components: Library (Lib) Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: akira, eric.araujo, ezio.melotti, kristjan.jonsson, martin.panter, michael.foord, pitrou, r.david.murray, rbcollins, remi.lapeyre, stutzbach, vzhong, zach.ware
Priority: normal Keywords: easy, patch

Created on 2012-04-09 13:10 by r.david.murray, last changed 2019-02-24 22:49 by remi.lapeyre.

Files
File name Uploaded Description Edit
issue14534.diff zach.ware, 2014-06-27 13:11 Patch by Victor Zhong and Lisha Ruan review
unittest_do_not_run.diff vzhong, 2014-06-28 16:27 Patch for marking unittest as do not run
issue14534.v2.diff zach.ware, 2014-07-24 14:42 Combined patch review
issue14534.v3.diff vzhong, 2014-07-25 15:45 combined patch against default tip review
Repositories containing patches
https://bitbucket.org/hllowrld/cpython/
https://bitbucket.org/hllowrld/cpython
https://bitbucket.org/hllowrld/cpython
Messages (41)
msg157842 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-09 13:10
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.)
msg157867 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 17:24
> 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.
msg157868 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2012-04-09 17:45
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.
msg157869 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-09 17:50
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.
msg157870 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 17:52
> 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.
msg157894 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2012-04-09 20:42
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.
msg157896 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2012-04-09 20:45
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.
msg157898 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 20:47
> 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.
msg157910 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-04-09 21:57
Note that I did just document the mixin idiom in the Lib/test docs.  Which core developers probably don't read :)
msg158222 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-04-13 17:54
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.
msg158252 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2012-04-14 10:34
+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.
msg220643 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-15 14:52
Do we have a consensus as to whether or not David's proposed solution is acceptable?
msg220648 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-15 15:39
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.
msg220844 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-06-17 16:56
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.
msg220845 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-06-17 17:03
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.
msg220854 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-06-17 18:40
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.
msg220864 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-06-17 19:19
Michael's suggestion looks too sophisticated and confusing to me.
msg220911 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-06-17 23:05
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?
msg220926 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-06-18 08:39
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).
msg220928 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-06-18 10:06
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).
msg220930 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-06-18 10:23
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).
msg220931 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-06-18 11:19
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.
msg220933 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-06-18 11:45
Note that we have to use mixin approach in the Python test suite to avoid 
discrepancies and merge conflicts between different versions.
msg220938 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-18 13:00
Issue 19645 is an alternative way to solve the problem of calling methods on the mixin that "don't exist", and has other benefits.
msg221019 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-06-19 20:41
+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.
msg221311 - (view) Author: Victor Zhong (vzhong) Date: 2014-06-22 21:02
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.
msg221686 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-06-27 13:11
Attaching Victor and Lisha's patch in reviewable form.
msg221687 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-06-27 14:16
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.
msg221800 - (view) Author: Victor Zhong (vzhong) Date: 2014-06-28 16:27
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.
msg223841 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-07-24 14:42
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).
msg223959 - (view) Author: Victor Zhong (vzhong) Date: 2014-07-25 15:45
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
msg224793 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-08-05 01:33
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.
msg224798 - (view) Author: Akira Li (akira) * Date: 2014-08-05 03:14
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.
msg224972 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-08-06 23:18
"abstract_test_class" anyone?
(or "abstractTestClass" if that looks more consistent)

(not that I would need it myself!)
msg224977 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-08-06 23:59
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.
msg225000 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-08-07 09:25
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.
msg225004 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-08-07 10:40
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.
msg248924 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-20 23:39
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.
msg248925 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-20 23:40
Removed the bogus huge diff.
msg248926 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-20 23:42
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.
msg249112 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-08-25 07:35
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.
History
Date User Action Args
2019-02-24 22:49:24remi.lapeyresetnosy: + remi.lapeyre
2019-02-24 22:43:39BreamoreBoysetnosy: - BreamoreBoy
2015-08-25 07:37:04martin.panterlinkissue22680 dependencies
2015-08-25 07:35:07martin.pantersetstage: patch review -> needs patch
messages: + msg249112
versions: + Python 3.6, - Python 3.5
2015-08-21 00:02:04rbcollinssettitle: Add means to mark unittest.TestCases as "do not run". -> Add means to mark unittest.TestCases as "do not load".
2015-08-20 23:42:02rbcollinssetmessages: + msg248926
2015-08-20 23:40:00rbcollinssetmessages: + msg248925
2015-08-20 23:39:51rbcollinssetfiles: - 01438f18ee18.diff
2015-08-20 23:39:00rbcollinssetnosy: + rbcollins

messages: + msg248924
title: Add method to mark unittest.TestCases as "do not run". -> Add means to mark unittest.TestCases as "do not run".
2014-08-08 17:20:38kilowusetfiles: + 01438f18ee18.diff
2014-08-07 12:50:27serhiy.storchakasetnosy: - serhiy.storchaka
2014-08-07 10:40:36ezio.melottisetmessages: + msg225004
2014-08-07 09:25:43michael.foordsetmessages: + msg225000
2014-08-06 23:59:06ezio.melottisetmessages: + msg224977
2014-08-06 23:18:58pitrousetmessages: + msg224972
2014-08-06 04:26:41martin.pantersetnosy: + martin.panter
2014-08-05 03:14:59akirasetnosy: + akira
messages: + msg224798
2014-08-05 01:33:52ezio.melottisetmessages: + msg224793
2014-07-25 15:45:21vzhongsetfiles: + issue14534.v3.diff
hgrepos: + hgrepo266
messages: + msg223959
2014-07-24 14:42:06zach.waresetfiles: + issue14534.v2.diff

messages: + msg223841
2014-06-28 16:27:13vzhongsetfiles: + unittest_do_not_run.diff
hgrepos: + hgrepo263
messages: + msg221800
2014-06-27 14:16:38zach.waresetmessages: + msg221687
2014-06-27 13:11:51zach.waresetstage: needs patch -> patch review
2014-06-27 13:11:27zach.waresetfiles: + issue14534.diff
keywords: + patch
messages: + msg221686
2014-06-22 21:02:04vzhongsethgrepos: + hgrepo262

messages: + msg221311
nosy: + vzhong
2014-06-19 20:41:13ezio.melottisetnosy: + ezio.melotti
messages: + msg221019
2014-06-18 13:00:32r.david.murraysetmessages: + msg220938
2014-06-18 11:45:28serhiy.storchakasetmessages: + msg220933
2014-06-18 11:19:44kristjan.jonssonsetmessages: + msg220931
2014-06-18 10:23:09michael.foordsetmessages: + msg220930
2014-06-18 10:06:38serhiy.storchakasetmessages: + msg220928
2014-06-18 08:39:37michael.foordsetmessages: + msg220926
2014-06-17 23:05:04michael.foordsetmessages: + msg220911
2014-06-17 19:19:32serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg220864
2014-06-17 18:40:06zach.waresetmessages: + msg220854
2014-06-17 17:03:55michael.foordsetmessages: + msg220845
2014-06-17 16:56:53zach.waresetnosy: + zach.ware
messages: + msg220844
2014-06-15 15:39:11r.david.murraysetmessages: + msg220648
2014-06-15 14:52:09BreamoreBoysetnosy: + BreamoreBoy

messages: + msg220643
versions: + Python 3.5, - Python 3.3
2012-04-14 10:34:01kristjan.jonssonsetnosy: + kristjan.jonsson
messages: + msg158252
2012-04-13 17:54:09eric.araujosetnosy: + eric.araujo
messages: + msg158222
2012-04-09 21:57:55r.david.murraysetmessages: + msg157910
2012-04-09 20:47:56pitrousetmessages: + msg157898
2012-04-09 20:45:54michael.foordsetmessages: + msg157896
2012-04-09 20:42:09michael.foordsetmessages: + msg157894
2012-04-09 17:52:11pitrousetmessages: + msg157870
2012-04-09 17:50:38r.david.murraysetmessages: + msg157869
2012-04-09 17:45:09stutzbachsetnosy: + stutzbach
messages: + msg157868
2012-04-09 17:24:09pitrousetnosy: + pitrou
messages: + msg157867
2012-04-09 13:10:52r.david.murraycreate