Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for issue #464 (@attr in subclasses of unittest.TestCase misbehaves) #500

Open
wants to merge 2 commits into from

2 participants

@bobbyi

When determing what tests to run for a class, loader.py previously
would walk the class's base classes and include any test that had
the same name as a test being run for a base class.

There is no reason to do this and it leads to the wrong behavior
in cases where the parent should run the test but not the child,
for example when the child has a test with the same name but
different attributes and the attrib plugin is being used.

Fixes #464.

bobbyi added some commits
@bobbyi bobbyi Don't force subclass to include all parent's tests
When determing what tests to run for a class, loader.py previously
would walk the class's base classes and include any test that had
the same name as a test being run for a base class.

There is no reason to do this and it leads to the wrong behavior
in cases where the parent should run the test but not the child,
for example when the child has a test with the same name but
different attributes and the attrib plugin is being used.

Fixes #464.
41ab054
@bobbyi bobbyi Add test case for Issue #464 6364882
@bobbyi

This is obviously a duplicate of #464. Not sure why my pull request showed up as an issue?

@bobbyi bobbyi closed this
@bobbyi bobbyi reopened this
@jpellerin jpellerin commented on the diff
nose/loader.py
@@ -110,10 +110,6 @@ def wanted(attr, cls=testCaseClass, sel=self.selector):
return False
return sel.wantMethod(item)
cases = filter(wanted, dir(testCaseClass))
- for base in testCaseClass.__bases__:
@jpellerin Owner

This seems like a pretty heavy-weight solution for this issue, with lots of backwards-compatibility issues. I know it would break quite a few test suites of mine. So I'd like to see another solution, if possible.

@bobbyi
bobbyi added a note

All the tests pass and as far as I can tell this doesn't break anything. I don't think this should change the behavior of anything other than fixing the cited bug. Can you give an example of code that will see a back-compat impact?

@jpellerin Owner

Any time you have a set of test cases like:

class A(TestCase):
def test(self):
pass

class B(A):
pass

-- that will break. Current behavior would be to run A.test and B.test, but with this change, B.test would not be run (unless I'm mistaken and this block of loader code does nothing). This is a pretty common pattern for parameterizing batches of tests by adding flags or settings to the test case that the tests then use. For instance if you have a bunch of identical tests to run against several versions of an api, you might put them all in the base test class with the api version as a class-level attribute, and have a bunch of subclasses that just override that attribute without defining any tests themselves.

@bobbyi
bobbyi added a note

Yes, that case works fine. I just tried it to verify. We get the initial list of test cases using dir() on the test class on the line immediately before the ones I removed. 'test' shows up in dir(B), so the test is run.

@jpellerin Owner

Ok. That's some comfort, but I'm still really not enthusiastic about fixing an issue with the attr plugin by removing code from the test loader. I'd really like to find some other way.

@bobbyi
bobbyi added a note

In my opinion, what the loader is doing here is clearly wrong and broken. It makes no sense to walk the list of base classes and re-add tests that the child class explicitly said it doesn't want. It is a no-op in the vast majority of cases, but makes the code more confusing through superflous use of __bases__ and recursion which is immediately scary. The only case in which the code actually has an effect is when it does something you don't want by re-adding tests that were explicitly filtered out for a subclass. In this case, that affects the attr plugin, but it could cause the same problem for any plugin that wants to filter out certain tests.

Nose's test coverage is good (especially around the attr plugin, where I added functional tests covering all the cases I could come up with when I fixed all the other issues with the plugin) and the fact that this change doesn't affect any tests makes me more confident it is the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jpellerin
Owner

Ok, I think we need a 2nd (or 3rd ;) opinion. Summoning @kumar303! Are you comfortable removing this code from the loader to fix the attr plugin issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 19, 2012
  1. @bobbyi

    Don't force subclass to include all parent's tests

    bobbyi authored
    When determing what tests to run for a class, loader.py previously
    would walk the class's base classes and include any test that had
    the same name as a test being run for a base class.
    
    There is no reason to do this and it leads to the wrong behavior
    in cases where the parent should run the test but not the child,
    for example when the child has a test with the same name but
    different attributes and the attrib plugin is being used.
    
    Fixes #464.
  2. @bobbyi

    Add test case for Issue #464

    bobbyi authored
This page is out of date. Refresh to see the latest.
View
12 functional_tests/support/att/test_attr.py
@@ -53,6 +53,16 @@ class TestSubclass(Superclass):
pass
+class TestSuperNoAttr(unittest.TestCase):
+ def test_method(self):
+ pass
+
+class TestSubclassWithAttr(TestSuperNoAttr):
+ def test_method(self):
+ pass
+ test_method.from_child = True
+
+
class Static:
def test_with_static(self):
pass
@@ -90,7 +100,7 @@ class TestAttrSubClass(TestAttrClass):
def test_sub_three(self):
pass
-def added_later_test(self):
+def added_later_test(*args):
pass
TestAttrSubClass.added_later_test = added_later_test
View
9 functional_tests/test_attribute_plugin.py
@@ -133,6 +133,15 @@ def verify(self):
assert 'test_case_three' not in self.output
+class TestMethodOverriding(AttributePluginTester):
+ # Issue #464
+ args = ["-a", "!from_child"]
+
+ def verify(self):
+ assert 'test_method (test_attr.TestSuperNoAttr) ... ok' in self.output
+ assert 'test_method (test_attr.TestSubclassWithAttr) ... ok' not in self.output
+
+
class TestStatic(AttributePluginTester):
# Issue #411
args = ["-a", "with_static"]
View
4 nose/loader.py
@@ -110,10 +110,6 @@ def wanted(attr, cls=testCaseClass, sel=self.selector):
return False
return sel.wantMethod(item)
cases = filter(wanted, dir(testCaseClass))
- for base in testCaseClass.__bases__:
@jpellerin Owner

This seems like a pretty heavy-weight solution for this issue, with lots of backwards-compatibility issues. I know it would break quite a few test suites of mine. So I'd like to see another solution, if possible.

@bobbyi
bobbyi added a note

All the tests pass and as far as I can tell this doesn't break anything. I don't think this should change the behavior of anything other than fixing the cited bug. Can you give an example of code that will see a back-compat impact?

@jpellerin Owner

Any time you have a set of test cases like:

class A(TestCase):
def test(self):
pass

class B(A):
pass

-- that will break. Current behavior would be to run A.test and B.test, but with this change, B.test would not be run (unless I'm mistaken and this block of loader code does nothing). This is a pretty common pattern for parameterizing batches of tests by adding flags or settings to the test case that the tests then use. For instance if you have a bunch of identical tests to run against several versions of an api, you might put them all in the base test class with the api version as a class-level attribute, and have a bunch of subclasses that just override that attribute without defining any tests themselves.

@bobbyi
bobbyi added a note

Yes, that case works fine. I just tried it to verify. We get the initial list of test cases using dir() on the test class on the line immediately before the ones I removed. 'test' shows up in dir(B), so the test is run.

@jpellerin Owner

Ok. That's some comfort, but I'm still really not enthusiastic about fixing an issue with the attr plugin by removing code from the test loader. I'd really like to find some other way.

@bobbyi
bobbyi added a note

In my opinion, what the loader is doing here is clearly wrong and broken. It makes no sense to walk the list of base classes and re-add tests that the child class explicitly said it doesn't want. It is a no-op in the vast majority of cases, but makes the code more confusing through superflous use of __bases__ and recursion which is immediately scary. The only case in which the code actually has an effect is when it does something you don't want by re-adding tests that were explicitly filtered out for a subclass. In this case, that affects the attr plugin, but it could cause the same problem for any plugin that wants to filter out certain tests.

Nose's test coverage is good (especially around the attr plugin, where I added functional tests covering all the cases I could come up with when I fixed all the other issues with the plugin) and the fact that this change doesn't affect any tests makes me more confident it is the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- for case in self.getTestCaseNames(base):
- if case not in cases:
- cases.append(case)
# add runTest if nothing else picked
if not cases and hasattr(testCaseClass, 'runTest'):
cases = ['runTest']
Something went wrong with that request. Please try again.