Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Django 4.0 compatibility#8178
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Django 4.0 compatibility#8178
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pytz will not automatically be installed with Django from v4.0.
Refsdjango/django@faba5b7.addClassCleanup() is available from Python 3.8, which is the minimum supportedPython from Django 4.0.
9822649 toa7e017dCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alrighty! Thanks so much for cracking on with this Carlton. 😀
I've left a few review comments for cases where it wasn't obvious to me why things have changed. More out of the sake of completeness of review, rather than anything that's necessarily a blocker. I'd actually be happy for it to go in either with or without resolution to these. (But who knows perhaps it'll help tease out some 4.0 release notes or something?)
| if django.VERSION > (4, 0): | ||
| cls.addClassCleanup(cls._override.disable) | ||
| cls.addClassCleanup(cleanup_url_patterns, cls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IsaddClassCleanup a new feature in Django 4.0?
I'm not seeing any differences in the docs re. the existingtearDownClass...https://docs.djangoproject.com/en/4.0/topics/testing/tools/#simpletestcase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
addClassCleanup is fromunittest.TestCase, added in Python 3.8https://docs.python.org/3.9/library/unittest.html#unittest.TestCase.addClassCleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Django just happens to be using it from Django 4.0 — Seedjango/django@faba5b7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah fab. How about we switch based on the Python version, rather than the Django version?
carltongibsonSep 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we should comment on the release notes for this? 🤔 (cc@felixxm )
I don't suppose there are many projects doing anything as subtle asURLPatternsTestCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we should comment on the release notes for this? thinking (cc@felixxm )
Sure, if you think it might be helpful. Is it not enough to callsuper().setUpClass() at the beginning? 🤔
@classmethoddefsetUpClass(cls):super().setUpClass()# Get the module of the TestCase subclasscls._module=import_module(cls.__module__) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It might be... Let me try that after lunch 🥪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
... or usecls._overridden_settings and remove redundantcls._override.enable()/disable() calls 🤔
cls._overridden_settings= {'ROOT_URLCONF':cls.__module__,**cls._overridden_settings,}super().setUpClass()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Movingsuper().setUpClass() from thebottom of thesetUpClass method to the top resolves this without any4.0 branching.
Do I properly understand why? Nope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In other words, this works...
@classmethoddefsetUpClass(cls):super().setUpClass()# <--- This line has moved.# Get the module of the TestCase subclasscls._module=import_module(cls.__module__)cls._override=override_settings(ROOT_URLCONF=cls.__module__)ifhasattr(cls._module,'urlpatterns'):cls._module_urlpatterns=cls._module.urlpatternscls._module.urlpatterns=cls.urlpatternscls._override.enable()@classmethoddeftearDownClass(cls):super().tearDownClass()cls._override.disable()ifhasattr(cls,'_module_urlpatterns'):cls._module.urlpatterns=cls._module_urlpatternselse:delcls._module.urlpatterns
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| def tearDownClass(cls): | ||
| assert urlpatterns is cls.urlpatterns | ||
| super().tearDownClass() | ||
| assert urlpatterns is not cls.urlpatterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SametearDownClass vsaddClassCleanup question as above.
Nothing about it in the release notes...https://docs.djangoproject.com/en/4.0/releases/4.0/ so I'm curious what prompts this to need changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See the commit I linked — we no longer calltearDown ourselves sosuper() isn't doing what it used to. (I presume the base implementation is empty.)
I didn't trace the exact call flow... — timings on those enable/disable or__enter__/__exit__ s is tricky, but there were failures without following the change in Django.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hrm that's interesting yeah. Looking through the commit indjango/django@faba5b7 I can'tsee any reason why subclasses would need to stop usingtearDownClass, even if Django 4.0 isn't using it directly for it's own implementation. But there's possibly some change in interaction I've not figured out there.
carltongibsonSep 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, no 😀 — it's not obvious.
My suspicion it's the timing ontheoverride_settings usage — which is alwaysdelicate, as we know from e.g.#5668 &friends.
😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oncesuper().setUpClass() is moved inrest_framework/test.py this class doesn't need to change at all.
(Or at least, that's how it seems from my testing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My suspicion it's the timing on the override_settings usage — which is always delicate, as we know from e.g.#5668 &friends.
Yes that seemsvery likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, if you want to adjust please do! (Otherwise I'll have another look later on)
Hey@tomchristie Thanks — I've put initial answers there. Just follow up if you want to dig further. (np my end :) |
USE_L10N defaults to True from Django 4.0, and will be removed in Django 5.0.
Private _get_new_csrf_token() was removed indjango/django@231de68.
…ions.zoneinfo was made the default time zone implementation indjango/django@306607d.
c62e3ca to3f67343CompareGrrr.
So added that in8eefb21. Itfails on Python 3.7 (and then the rest of the runs fast-fail and cancel) Trying different envs locally it looks like the 3.2 to 4.0 change. Just moving the I need to do something else now, but I'll look at@felixxm's suggestions later on. |
3f67343 toc62e3caCompareOK, likely it's resolvable with enough staring at it, but I can't get it lined up for all versions immediately, so I've reset the PR to its earlier state, which has the branching, but does in fact work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okey dokes. I don't think the implementation details onURLPatternsTestCase really matter here, so happy for this to go in whenever. (Important thing is that we probably don't think there's anything that needs adding to the Django release notes, right?)
Thanks Tom — In that case I think we should get this in, so we'reclean against 4.0a1 from our POV. Folks then testing can then turn up any other issues. |
I'm not exactly sure what I'd say. :) — But I will chat with@felixxm about it next week. |
Uh oh!
There was an error while loading.Please reload this page.
Adds compatibility for Django 4.0.
From Django 4.0
zoneinfois the default time zone implementation. This PR makes the minimum changes to work with that, plus other necessary adjustments.DRF now requires
pytzininstall_requires. There's a small amount of work to be done infields.pyandtest_fields.pyto remove thepytzdependency. (Django will remove support forpytzin Django 5.0.)