Skip to content

Getting error "setUpClass should be a classmethod" #597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kleptog opened this issue May 24, 2018 · 3 comments
Closed

Getting error "setUpClass should be a classmethod" #597

kleptog opened this issue May 24, 2018 · 3 comments
Labels

Comments

@kleptog
Copy link

kleptog commented May 24, 2018

Recently some magic was added that tries wrap the setUpClass methods for some reason. Not sure why. In any case, it breaks our testsuite. I've attached a test file to reproduce.

The basic issue is that the the setUpClass raises a skip exception sometimes to cause the methods defined in that class to be skipped. This causes the wrapping code to skip restoring the classmethod, leading to errors later on.

The error is in this code:

_restore_class_methods(cls)
cls.setUpClass()
_disable_class_methods(cls)

If the setUpClass() throws an exception (in this case Skip), the restore is skipped and chaos ensues.

This is a test file that reproduces it:

import pytest
from django.test import SimpleTestCase


class foo(SimpleTestCase):
    @classmethod
    def setUpClass(cls):
        if cls is foo:
            raise pytest.skip("Skip base class")
        super(foo, cls).setUpClass()

    def test_shared_foo(self):
        pass


class bar(foo):
    def test_bar1(self):
        pass


class bar2(foo):
    def test_bar21(self):
        pass
@blueyed
Copy link
Contributor

blueyed commented May 24, 2018

Thanks for the report.

I guess it should be wrapped with a try: … finally maybe to call _disable_class_methods always?

The commit is quite old, but likely was only released recently then? (96f5fb1)

It's good to have a test case.
Can you please create a PR from it?

@blueyed blueyed added the bug label May 24, 2018
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 25, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
@kleptog
Copy link
Author

kleptog commented May 25, 2018

Actually, after sleeping on it I think just adding a try/except isn't enough: at the point the exception is thrown the method has been set back, so this can't be the issue.

More digging revealed the actual problem: the classmethod test was incorrect. The testcase can be made even simpler: replacing the setUpClass with just pass still break.

Making a pull request now.

kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 25, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 25, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 25, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 25, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 28, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 28, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 28, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 28, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 28, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Fixes pytest-dev#597
kleptog pushed a commit to kleptog/pytest-django that referenced this issue May 29, 2018
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Additionally, there was a problem where the restored methods were bound to
the wrong class.  Added test case and fixed that also.

Fixes pytest-dev#597
@kleptog
Copy link
Author

kleptog commented May 29, 2018

Ok, buildbot passed my patch. Actually two bugs in the end, but now it all works :)

beyondgeeks pushed a commit to beyondgeeks/django-pytest that referenced this issue Sep 6, 2022
This test was incorrect in the case where the method was inherited from a
subclass.  Which ironically is precisely what the function was supposed to
test.

Additionally, there was a problem where the restored methods were bound to
the wrong class.  Added test case and fixed that also.

Fixes pytest-dev/pytest-django#597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants