pakal / django-compat-patcher

A system to improve compatibility between different Django versions, and make upgrading dependencies less painful.
MIT License
13 stars 2 forks source link

setup.py test integration #7

Closed jayvdb closed 3 years ago

jayvdb commented 4 years ago

Could we add a setup.py command for packagers to use which depends on envvar DJANGO_SETTINGS_MODULE being set, and does the patching before running the test suite (invoking the test command).

e.g. rpm spec could then do

 %check
 export DJANGO_SETTINGS_MODULE=test_settings
-python setup.py test
+python setup.py test_django_compat_patched

(setup.py test is deprecated, but they didnt go back in time and tell that to Django 1.x era developers ;-) django-setuptest serves as a proxy.)

jayvdb commented 4 years ago

This, and a pytest-django extension, might be better off as a separate project..?

jayvdb commented 4 years ago

The primary motivation is that often there is a patch for the app, or patching the app is trivial (e.g. adding on_delete), however patching the test suite is often significantly harder, especially if the distro has multiple versions of Python which can result in multiple versions of Django 2.2 and 3.0, and then the changes to the test logic gets messy, and requires knowledge of more than the current Django. And while writing that patch, the packager doesnt know whether there are extra compat issues they havent found yet. If django-compat-patcher can be employed in a single line of .spec change, it can be put into work procedures as a "try this before deleting the package" (which unfortunately happens a lot).

jayvdb commented 4 years ago

Or more generic , create a binscript which does the patching and then invokes the command line args within the same interpreter.

export DJANGO_SETTINGS_MODULE=test_settings
patched-django setup.py test

then pytest should also work with either

patched-django /usr/bin/pytest

patched-django -m pytest
pakal commented 4 years ago

When you write "patched-django -m pytest", you rather mean a patched python, don't you?

I've searched for a way to automatically load DCP at python startup, but I couldn't find anything so far :( It always requires modifying the user configs, or the python "site", or to be in interactive mode... no generic "PYTHON_PRELOAD" env var it seems. Maybe that'd be an idea to push upstream ?

I've proposed settings to choose the way Django is setup, but they didn't make it upstream.

Usually, projects relying on DCP just have to import and activate it very early (in their manage.py, or in a top level import), and it's all good.

Pytest is a specific case, since it sets up Django very early, before project code has a change to execute. I've solved it with an early-load plugin, but I'd love to have this PR included to help that (I must update it asap) : https://github.com/pytest-dev/pytest-django/pull/768

DCP usually requires at least the codebase to be editable, so a packager will probably hit showstoppers anyway. But if lots of packages use the same way to test code, maybe some solutions can be found to ease the work. For example, providing an alternative "django-admin" command, which works like the native one, but preloads DCP before doing its call_command()?

jayvdb commented 4 years ago

When you write "patched-django -m pytest", you rather mean a patched python, don't you?

No. create a binscript (i.e. in setup.py) called "patched-django", which invokes the patcher and then uses the python standard startup processing of command line args.

Similar to how coverage run ... can do anything that python ... can do, but lets coverage get into the runtime first.

jayvdb commented 4 years ago

What would also be useful is a quick way to select only a subset of fixers which are "common old test logic".

Scenario: I have patched the code of https://github.com/pmac/django-redirect-urls (https://github.com/browniebroke/django-codemod/pull/123 ) , and then while running the test suite I run into https://github.com/browniebroke/django-codemod/issues/129 . I would like to focus on fixing all of the code problems first, but I can't see them because they are hidden behind test setup logic failure.

This is a major stumbling block for distro maintainers / typical Django devs, because if the test logic fails, the test logic is usually much more complicated than Django app logic, and test logic it also often foreign to them, especially if it is using unittest as increasingly pytest has washed peoples brains of that skillset.

In reality I will have to fix the test logic next, or I will get DCP working with pytest. i.e. Expect a patch for this issue soon. ;-)

jayvdb commented 4 years ago

https://github.com/rpatterson/python-main-wrapper does the trick

django-redirect-urls> PYTHONPATH=. DJANGO_SETTINGS_MODULE=tests.settings python-main-wrapper ~/bin/django_wrapper.py ./runtests.py

where ~/bin/django_wrapper.py is

import django_compat_patcher
django_compat_patcher.patch()
pakal commented 4 years ago

Thanks for the idea, indeed I hadn't thought about going as far as providing an alternate python executable.

Coverage.py use their own toolchain for that ; python-main-wrapper would mean adding another dependency to DCP but it might tbe worth it (or as a setup.py extra?)

I don't quite get the "test logic" issue - why would a subset of fixers be necessary ? Having many fixers applied adds a small delay at program launch, but apart from that they are supposed to be all safe to apply (else it's a bug). So why not apply them all, and see if the tests succeed or not ?

jayvdb commented 4 years ago

I am quite happy with using python-main-wrapper + django-compat-patcher , rather than tight integration ; we may do some minor changes here so that it works easier with python-main-wrapper , but documenting this solution would suffice.

The reason to select a subset of fixers is to determine which fixers are needed. My goal when patching a package is to determine how much effort is needed to get the package working on the target platforms. I cant submit a package which depends on django-compat-patcher , but I can use django-compat-patcher to understand the hurdles I need to overcome in order to manually patch the codebase. If there are only a few problems, and django-compat-patcher fixes them all, it is clear that an hour or two is sufficient to manually patch the codebase. Or django-compat-patcher might be using a fixer that I know is "hard" to re-do as a manual patch.

pakal commented 4 years ago

Allright I see, maybe then it should be made possible to pass DCP settings via ENV vars (by following conventions for that, I don't remember just now), possibly with shortcut settings like "how many previous versions of Django the fixers should go down to" (currently one must provide all the fixer ids or classes, it's a bit verbose) ? Or would it be better to add these as additional options to the DCP-specific python-main-wrapper ?

jayvdb commented 4 years ago

https://build.opensuse.org/package/show/home:jayvdb:django/python-django-admin-steroids has django-compat-patcher working as part of the test.

Essentially

export DJANGO_SETTINGS_MODULE=admin_steroids.tests.settings
python -m mainwrapper django_compat_patcher:patch django test

So, no "script" or entry point is needed. However, creating a __main__ or a script could have some benefits.

It would allow a different set of CLI args.

There are some arg issues already with main-wrapper. c.f. https://github.com/rpatterson/python-main-wrapper/issues/1 , but it can also be solved there.

Given the problems with argparse/optparse with Django , I am shy of adding more CLI args.

ENV vars sounds good.

Another option would be to have a config file, and allow the user to define sets of fixers , giving the set a custom name. I can see a config file being useful for

  1. single project needs, stored in the project repo.
  2. user needs, stored in XDG config, providing defaults, or multiple sections offering different "profiles"

Sets of fixers could be custom defined families, similar to "django1.9"

pakal commented 4 years ago

I've added to README some instructions for mainwrapper, thanks, it works great indeed (especially since DCP settinsg can now be passed as ENV vars).

I don't see really the use for fixer sets in user config or elsewhere - by default activating all (except unsafe ones) is good, and when probing the project compatibility, once just has to play with FAMILIES and then individual fixers IDs to have a good overview. And at worst one can use sourced bash files with proper ENV vars.

Something nice would maybe be a script which loops on a command, removing 1 old fixer at a time, and lists the fixers which are necessary for the command (e.g. pytest) to pass. Wouldn't it ?

jayvdb commented 4 years ago

Something nice would maybe be a script which loops on a command, removing 1 old fixer at a time

Yup, and again sounds like something that is generally useful beyond just DCP.

pakal commented 3 years ago

For now, just playing with fixer families should be sufficient - the overhead of DCP init isn't that important in the context of server processes. The important will be to ensure that fixers are as SAFE as possible.