Conversation
eviljeff
force-pushed
the
15585-django52
branch
5 times, most recently
from
7362a2f to
926b079
Compare
| assert set(inputs) == set(data.items()) | ||
|
|
||
| # Same for the 'search' form | ||
| form = doc('#changelist-filter form') |
There was a problem hiding this comment.
this part of the test never worked as expected - we were using the same selector as the previous chunk of code. I couldn't figure out what it was supposed to be testing exactly so I removed it.
| target_versions = ( | ||
| self.target_versions.no_transforms() | ||
| .only('pk', 'version') | ||
| .defer('approval_notes') |
There was a problem hiding this comment.
This isn't really the same as the only it replaces, but I figured only approval_notes was potentially a huge chunk of text. But we could tighten it?
| self.grant_permission(user, 'Admin:Advanced') | ||
| self.client.force_login(user) | ||
| with self.assertNumQueries(20): | ||
| with self.assertNumQueries(20 if self.is_django42 else 18): |
There was a problem hiding this comment.
From looking into one of the other tests, the savepoints got optimised - previoulsy in some of these cases there were 4 savepoint related queries; now 2 in django52
| data_types = { | ||
| **mysql_base.DatabaseWrapper.data_types, | ||
| # in django52 the class property is _data_types instead. | ||
| # TODO: this overwrite's django52's special handling for mariadb! Make it work. |
There was a problem hiding this comment.
Once we drop django42 support we can rewrite this in a way that doesn't break mariadb. Seems a fair tradeoff (given it's only a problem if we want to switch away from mysql to mariadb) in the meantime.
| if not self.has_listed: | ||
| # add empty label back | ||
| listed.empty_label = listed_empty_label | ||
| listed.choices = [('', listed_empty_label)] |
There was a problem hiding this comment.
couldn't quite get to the bottom of what changed that made our current hack break, but switching it around so we add the empty label when there is no applicable listed version, rather than remove it when there is, seemed to fix it 🤷
| # If running in a Windows environment this must be set to the same as your | ||
| # system time zone. | ||
| TIME_ZONE = 'UTC' | ||
| USE_TZ = False |
There was a problem hiding this comment.
default changed in django52
| USE_I18N = True | ||
|
|
||
| # This enables localized formatting of numbers and dates/times. Deprecated in Django4.1 | ||
| USE_L10N = True |
There was a problem hiding this comment.
setting dropped in django52, and was already defaulted to True in django42
|
Given the changes, I'm not sure we should bother with the added CI complexity. Could we split this and run most of the changes in |
eviljeff
force-pushed
the
15585-django52
branch
from
8830e8b to
8311987
Compare
|
(rebased to get the mysql upgrade, and added jslex replacement). Once CI passes on 4.2 and 5.2 I'll strip out the CI, etc, changes so it all applies to 4.2 only. |
eviljeff
force-pushed
the
15585-django52
branch
from
dee5cf7 to
95d488e
Compare
eviljeff
force-pushed
the
15585-django52
branch
from
95d488e to
86d4d3b
Compare
2 participants
Fixes: mozilla/addons#15585
Description
All the django52 fixes~, plus CI changes to run tests on both django52 and django42.~
Context
The complex pipeline we have from ci.yml, through: sub-test yml, actions, dockerfile, makefile, install_deps.py, -and in some cases back again- makes for a nightmarish environment for adding switching between django versions. (For the django42 and django32 upgrades it was considerably easier). Some of the passing down of django_version/DJANGO_VERSION may not actually be necessary.
It would be cleaner to not include any of this in the eventual merge, and to just directly upgrade in prod.txt, at the cost of losing the ability to switch backwards and forwards, and test both django versions in the same CI.
^ that's what we're doing, except without the upgrade in prod.txt - we're sticking on django4.2 in this pr
Testing
Test all the things!
DJANGO_VERSION=django52 make upDJANGO_VERSION=django52 make update_depswith docker already up.- quicker because it doesn't have to rebuild everything in the docker image, but less authentic./__version__(or amo-info webextension)Checklist
#ISSUENUMat the top of your PR to an existing open issue in the mozilla/addons repository.