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 override_config test decorator on Django 5.2 #609

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pfouque
Copy link
Member

@pfouque pfouque commented Jan 27, 2025

Yet another Django 5.2 fix:

Django 5.2 is the first version to ship this PR turning pre_setup into a classmethod and generating this error (already visible in tox, but ignored)

  ERROR: test_override_config_on_class_changes_config_value (tests.test_test_overrides.OverrideConfigClassDecoratorTestCase.test_override_config_on_class_changes_config_value)
  Assert that the class decorator changes config.BOOL_VALUE.
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/home/runner/work/django-constance/django-constance/constance/test/unittest.py", line 49, in _pre_setup
      original_pre_setup(inner_self)
  TypeError: TransactionTestCase._pre_setup() takes 1 positional argument but 2 were given

I found this fix, following the current solution but maybe a better (and more modern) solution exists
"""
Override the config by modifying TestCase methods.

This method follows the Django <= 1.6 method of overriding the
_pre_setup and _post_teardown hooks rather than modifying the TestCase
itself.
"""

@pfouque pfouque self-assigned this Jan 27, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.82%. Comparing base (1451943) to head (1ea167f).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   84.23%   84.82%   +0.58%     
==========================================
  Files          21       21              
  Lines         736      751      +15     
  Branches      117      122       +5     
==========================================
+ Hits          620      637      +17     
+ Misses         83       82       -1     
+ Partials       33       32       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pfouque
Copy link
Member Author

pfouque commented Jan 27, 2025

FYI: I added Django 5.2 to the test matrix (to catch other potential issues).
it's pointing to the alpha release so it will have to be updated

Screenshot 2025-01-27 at 12 16 13

@pfouque pfouque marked this pull request as ready for review January 27, 2025 11:18
original_pre_setup(inner_self)
else:

@classmethod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create a decorator for that so that we don't have to duplicate the code of the method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The decorator is not the only difference, the call to original_pre_setup also differs (as we don't have to pass the instance anymore)
Additionally, this version is probably easier to clean up when Django5.2 will be the minimum supported version

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.

3 participants