Skip to content
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

Fix classmethod test. #598

Closed
wants to merge 1 commit into from
Closed

Conversation

kleptog
Copy link

@kleptog kleptog commented 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 #597

@kleptog
Copy link
Author

kleptog commented May 25, 2018

It's not passing the tests. Test case that this fixes:

from django.test import SimpleTestCase


class foo(SimpleTestCase):
    @classmethod
    def setUpClass(cls):
        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

@kleptog kleptog force-pushed the issue_597 branch 4 times, most recently from 831bee2 to e4d4451 Compare May 25, 2018 16:41
@blueyed
Copy link
Contributor

blueyed commented May 26, 2018

Thanks for fixing this (changed in a67ab3b by myself).

Tests are currently failing, but it looks like it is related to matching the output only.
You could use * to make it less strict - OTOH messages/output ("Destroying test database") shouldn't be joined like this in the first place.

@kleptog
Copy link
Author

kleptog commented May 28, 2018

I'll see if I can make it less strict, but it would be nice indeed if we could prevent those messages appearing. This test doesn't require a database...

@kleptog kleptog force-pushed the issue_597 branch 5 times, most recently from 5261c01 to 0fc5220 Compare May 28, 2018 16:46
@kleptog
Copy link
Author

kleptog commented May 29, 2018

Ok, the tests are now fine, but a test case fails. I'm pretty sure this indicates some other bug which I'm not sure how to fix. I'm having difficulty testing on my local setup, but I'll keep trying.

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
@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #598 into master will decrease coverage by 3.97%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
- Coverage   91.92%   87.95%   -3.98%     
==========================================
  Files          33       33              
  Lines        1660     1660              
  Branches      143      143              
==========================================
- Hits         1526     1460      -66     
- Misses         95      145      +50     
- Partials       39       55      +16
Flag Coverage Δ
#dj110 83.91% <0%> (ø) ⬆️
#dj111 79.93% <0%> (-6.15%) ⬇️
#dj18 ?
#dj19 83.79% <0%> (ø) ⬆️
#dj20 84.21% <0%> (ø) ⬆️
#djmaster 84.21% <0%> (ø) ⬆️
#mysql_innodb ?
#mysql_myisam ?
#postgres 85.54% <0%> (-2.05%) ⬇️
#py27 ?
#py34 83.79% <0%> (ø) ⬆️
#py35 83.91% <0%> (ø) ⬆️
#py36 84.69% <0%> (ø) ⬆️
#sqlite 85.96% <0%> (ø) ⬆️
#sqlite_file 83.79% <0%> (ø) ⬆️
Impacted Files Coverage Δ
pytest_django/plugin.py 82.19% <0%> (-3.57%) ⬇️
pytest_django_test/settings_mysql_myisam.py 0% <0%> (-100%) ⬇️
pytest_django_test/settings_mysql_innodb.py 0% <0%> (-100%) ⬇️
pytest_django/migrations.py 80% <0%> (-20%) ⬇️
pytest_django_test/db_helpers.py 65.74% <0%> (-16.67%) ⬇️
tests/test_database.py 85.86% <0%> (-13.05%) ⬇️
pytest_django/live_server_helper.py 80.59% <0%> (-10.45%) ⬇️
tests/conftest.py 94.11% <0%> (-5.89%) ⬇️
tests/test_db_setup.py 95.6% <0%> (-4.4%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bd3d3e...10c93a9. Read the comment docs.

@blueyed
Copy link
Contributor

blueyed commented May 30, 2018

it would be nice indeed if we could prevent those messages appearing. This test doesn't require a database...

I guess that's just implied with tests based on Django's TestCase I assume.

@blueyed
Copy link
Contributor

blueyed commented May 30, 2018

Thanks, slightly amended and merged in 9b7f47b.

diff --git c/pytest_django/plugin.py i/pytest_django/plugin.py
index 2164f13..1336f90 100644
--- c/pytest_django/plugin.py
+++ i/pytest_django/plugin.py
@@ -292,8 +292,8 @@ def _disable_class_methods(cls):
         return
 
     _disabled_classmethods[cls] = (
-        # want the classmethod object, not the resulting bound method
-        # otherwise when restoring, the inheritence will be broken
+        # Get the classmethod object (not the resulting bound method),
+        # otherwise inheritence will be broken when restoring.
         cls.__dict__.get('setUpClass'),
         _classmethod_is_defined_at_leaf(cls, 'setUpClass'),
         cls.__dict__.get('tearDownClass'),

@blueyed blueyed closed this May 30, 2018
adamchainz added a commit to adamchainz/pytest-django that referenced this pull request Jun 17, 2018
I have a case of using a mixin on my test classes that started failing with 3.3.0 in upgrade PR adamchainz/django-mysql#484. It was caused by pytest-dev#598.

It looks like this was actually a latent failure in the way `_classmethod_is_defined_at_leaf` was picking the 'super method' - it would iterate further up the tree despite coming across the next method in the resolution chain. Also by not using `__mro__` the order of the base classes wasn't strictly being observed, although I don't think that affects my mixin case.

Added a test that fails before and passes after.
blueyed pushed a commit that referenced this pull request Jun 18, 2018
Fixes regression in #598.

It looks like this was actually a latent failure in the way
`_classmethod_is_defined_at_leaf` was picking the 'super method' - it
would iterate further up the tree despite coming across the next method
in the resolution chain. Also by not using `__mro__` the order of the
base classes wasn't strictly being observed, although I don't think that
affects my mixin case.

Added a test that fails before and passes after.

Closes #618.
@blueyed
Copy link
Contributor

blueyed commented Jun 21, 2018

Thanks for fixing this - just noticed that DRF is also affected by this.

Releasing 3.3.1 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants