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

Should automatically call django.setup() on django 1.7 #119

Closed
ionelmc opened this issue Jun 22, 2014 · 22 comments
Closed

Should automatically call django.setup() on django 1.7 #119

ionelmc opened this issue Jun 22, 2014 · 22 comments
Labels

Comments

@ionelmc
Copy link
Member

ionelmc commented Jun 22, 2014

For now I have this in my conftest.py:

try:
    from django import setup
except ImportError:
    pass
else:
    def pytest_configure():
        setup()

Without it I have these mind-boggling errors (on django 1.7b4):

Traceback (most recent call last):
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/core/handlers/base.py", line 111, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/contrib/admin/sites.py", line 225, in wrapper
    return self.admin_view(view, cacheable)(*args, **kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/utils/decorators.py", line 105, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/contrib/admin/sites.py", line 204, in inner
    return view(request, *args, **kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/contrib/admin/sites.py", line 401, in index
    'app_url': reverse('admin:app_list', kwargs={'app_label': app_label}, current_app=self.name),
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/core/urlresolvers.py", line 542, in reverse
    return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs))
  File "/home/ionel/osp/django-admin-utils/.tox/3.4-1.7/lib/python3.4/site-packages/django/core/urlresolvers.py", line 459, in _reverse_with_prefix
    (lookup_view_s, args, kwargs, len(patterns), patterns))
django.core.urlresolvers.NoReverseMatch: Reverse for 'app_list' with arguments '()' and keyword arguments '{'app_label': 'auth'}' not found. 1 pattern(s) tried: ['admin/(?P<app_label>test_app)/$']
@ionelmc ionelmc changed the title Should call django.setup() on django 1.7 Should automatically call django.setup() on django 1.7 Jun 22, 2014
@l0kix2
Copy link

l0kix2 commented Jun 23, 2014

Ran into this too today, fixed in conftest also.

@pelme
Copy link
Member

pelme commented Jul 2, 2014

Thanks for the report. The issue is a bit odd though, calling django.setup() is already done before any test runs:

https://github.com/pelme/pytest_django/blob/master/pytest_django/plugin.py#L129

Can you provide some additional context where this happens? Please provide the full py.test command output

@maryokhin
Copy link

On Django 1.7c1 getting AppRegistryNotReady almost no matter what I do if django.setup() is not called

@paulcollinsiii
Copy link

@pelme I just ran into this as well. The cursor wrapper referred to there happens during test running correct? During test discovery for example, if you're importing models and something funny is going on then you can run into some crazy corner cases. I ran into this by having models that use django-simple-history.
Since my tests imported models that used simple history, and simple history reaches into the AppConfig, I was getting AppRegistryNotReady exceptions during test discovery. I'll make a quick PR for where I put a django.setup() that fixed that issue.

This happened on django 1.7c1

@pelme
Copy link
Member

pelme commented Jul 17, 2014

There probably are/will be a ton of problems with third party Django apps that does not support Django 1.7 properly with regard to the new app loading.

If AppRegistryNotReady happens during import time (which means that some module reaches into the available apps during import time) - that app does not work well on Django 1.7 at all, and that is a bug with that application.

The solution is to defer any actions that require access to other applications to AppConfig.ready(). I guess a lot of people will notice this when running/collecting tests, but this is just the first sign of a broken third party app or any other code that relies on having the app cache available during import time.

I discussed this briefly with Aymeric Augustin during Djangocon, and that is the advice he gave to the best of my understanding for how to port applications to Django 1.7.

If you are unsure whether a problem with app cache is a bug in your code, a third party app or pytest-django, please provide full tracebacks of the failures and we can discuss each case. Please provide the entire terminal output to make it easier to understand where things go wrong.

@paulcollinsiii
Copy link

So here's an example that I'm running into.

py.test --ds datanav.config.settings --dc Testing ./tests --tb native --collect-only 

# long list of tests snipped...

tests/functional/test_form5500/test_admin.py:8: in <module>
    from form5500 import models as f5500_models
src/form5500/models/__init__.py:7: in <module>
    from . import (base, basic, basic_sf, schedule_a, schedule_b, schedule_c, schedule_d,
src/form5500/models/basic.py:306: in <module>
    class Form5500Basic_2006(_Form5500Basic_1999_2008):
/opt/boxen/data/virturalenvs/didj17/lib/python2.7/site-packages/django/db/models/base.py:285: in __new__
    new_class._prepare()
/opt/boxen/data/virturalenvs/didj17/lib/python2.7/site-packages/django/db/models/base.py:343: in _prepare
    signals.class_prepared.send(sender=cls)
/opt/boxen/data/virturalenvs/didj17/lib/python2.7/site-packages/django/dispatch/dispatcher.py:198: in send
    response = receiver(signal=self, sender=sender, **named)
/opt/boxen/data/virturalenvs/didj17/lib/python2.7/site-packages/simple_history/models.py:88: in finalize
    history_model = self.create_history_model(sender)
/opt/boxen/data/virturalenvs/didj17/lib/python2.7/site-packages/simple_history/models.py:120: in create_history_model
    app = apps.app_configs[model._meta.app_label]
E   KeyError: 'form5500'

so ya, simple_history is messing with the app_configs dictionary but during normal use it ends up working because models aren't imported until after AppConfig has been setup. In https://github.com/pelme/pytest_django/blob/master/pytest_django/plugin.py#L76
if I add

from .compat import setup
setup()

then that particular set of errors goes away (I'm fighting a thing with FactoryBoy as well...)
Of course by adding those lines I cause all kinds of test failures in your tests 😢

paulcollinsiii added a commit to paulcollinsiii/pytest_django that referenced this issue Jul 17, 2014
Added ropeproject folders to gitignore,
Added call to setup() during loading. This FAILS some tests, but I'm
posting it up to get some help with those failures.
@pelme
Copy link
Member

pelme commented Jul 17, 2014

Hmm. It looks like django-simple-history is intentionally hooking into the app config even when it is not ready, to be able to dynamically create the history model.

It also looks like django-simple-history is using some kind of heuristic to find the proper module for the models. This might be the problem in this case since you are not using a plain models.py.

I am not familiar with django-simple-history internals, but it uses internal Django API:s and uses a lot of meta things. This might be an edge case that breaks with how it detects models, but I think it can possibly be fixed by moving some parts of the initialization to AppConfig.ready().

This bug might be an error of implicit module loading (the very error that the Django 1.7 app loading should fix). This may work when invoked from runserver just because of the import's happens in another order then, which does not trigger the error.

What happens if you just run a python shell (not manage.py shell, just python) and run "from form5500 import models as f5500_models" ? Does that work?

Could you open an issue on django-simple-history to see if they think this is a bug?

@pelme
Copy link
Member

pelme commented Jul 17, 2014

(Btw, just running setup() whenever pytest-django is loaded will not work. pytest-django is very careful to not load Django when Django is not imported, and this would just break that feature. setup() is called properly, but after test collection, but always before a test runs)

@paulcollinsiii
Copy link

Oh, darn. I didn't realize that was a very deliberate feature. I'll close my PR then since it's completely the wrong solution in that case. On that feature though, doesn't the normal django test running fully load settings then do test discovery?

As for importing those models through the normal shell I get
django.core.exceptions.ImproperlyConfigured: Requested setting AUTH_USER_MODEL, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings. If I define the django settings module, then django-configurations squawks. I have a feeling that loading that model any way other than manage.py shell will cause 🔥

I guess the end result for my tests is just update my conftest.py module to force loading of django settings before test discovery? Or should I update my PR to make that an option? I have a feeling that during the whole 1.7 upgrade there are going to be a lot of third party modules that have these kinds of issues.

@blueyed
Copy link
Contributor

blueyed commented Jul 17, 2014

@pelme

I think it can possibly be fixed by moving some parts of the initialization to AppConfig.ready()

Do you mean that this is something that pytest-django could do?

@paulcollinsiii
I also think that filing an issue with django-simple-history would help to address, discuss and fix this in general.

@pelme
Copy link
Member

pelme commented Jul 18, 2014

@blueyed No, I mean that django-simple-history should probably move any initialization that depends on having app config ready from import time to its AppConfig.ready().

I left a comment in the django-simple-history issue too.

I think that the order of things should be

  • Import things
  • Run django.setup() (which in turn triggers all AppConfig.ready() triggers, with a fully reliable and populated app config)
  • Run anything else (test, webservers, celery workers, ...)

The "Import things" step can be omitted when running a web server for instance, since you do not really care about the modules, and django.setup() will import all things needed.

This is currently how it works in pytest-django. If anyone else is having 1.7 issues, please post test outputs and we can try to debug/discuss them to find the root cause.

@paulcollinsiii
Copy link

Okay so I'm still in a bit of a quandary as to what to do.

py.test goes through and loads my tests, which in turn see my factory_boy tests which then go out to my models. If django.setup() hasn't run then even if django-simple-history is using AppConfig.ready() it still can't rely on AppConfig.get_app_config('label') because apps haven't even been loaded.

I guess my point is, if you're using factory boy with your tests, or if your tests import your models then in django 1.7+ that loading order breaks AppConfig slightly. Probably not noticeably initially except in these corner cases.

In pre-1.7 cases I can see why you don't want django to go through all it's import machinations during test discovery but in an AppConfig world even in the case of unit tests it makes sense to me to have django load all it's settings early. It's also entirely possible I'm missing an obvious use case here, but if not then I'm happy to go and re-work my PR a bit to get the tests passing and have setup() called before test discovery.

@aaugustin
Copy link

@pelme I'm sorry, but I haven't explained this sufficiently clearly when we talked.

django.setup() should run before anything else and especially before other imports.

If you run into AppRegistryNotReady while django.setup() is running, then you need to reorganize your code somehow to avoid depending on the app registry while it's being populated.

However, if pytest imports modules and runs into AppRegistryNotReady before running django.setup() then the answer is to run django.setup() earlier.

I would encourage everyone attempting to debug this to run python -Wall. Django will warn you if you import a model before running django.setup(). That will be forbidden in Django 1.9.

@pelme
Copy link
Member

pelme commented Jul 26, 2014

@aaugustin Thanks for clearing it up. I will look into this to change to that behavior.

pelme added a commit that referenced this issue Jul 27, 2014
This patch is still not working properly with regard to loading settings
from within the test directories.
pelme added a commit that referenced this issue Jul 27, 2014
@pelme
Copy link
Member

pelme commented Jul 27, 2014

Thanks everybody for helping out with this.

django.setup() is now called as soon as possible after the settings is configured and I hope that fixes all problems.

Please open new issues if there are still problems!

@dustinfarris
Copy link

I just pulled in the latest commit. My conftest.py has to look like this for tests to pass:

import os

from django import setup


def pytest_configure():
    os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'myproject.settings')
    setup()

If i leave out the django.setup part, I get this when running py.test:

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/main.py", line 79, in wrap_session
INTERNALERROR>     config.do_configure()
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/config.py", line 610, in do_configure
INTERNALERROR>     self.hook.pytest_configure(config=self)
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/core.py", line 412, in __call__
INTERNALERROR>     return self._docall(methods, kwargs)
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/core.py", line 423, in _docall
INTERNALERROR>     res = mc.execute()
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/core.py", line 314, in execute
INTERNALERROR>     res = method(**kwargs)
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/xdist/plugin.py", line 62, in pytest_configure
INTERNALERROR>     __multicall__.execute()
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/core.py", line 314, in execute
INTERNALERROR>     res = method(**kwargs)
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/pastebin.py", line 18, in pytest_configure
INTERNALERROR>     __multicall__.execute()
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/_pytest/core.py", line 314, in execute
INTERNALERROR>     res = method(**kwargs)
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/pytest_django/plugin.py", line 116, in pytest_configure
INTERNALERROR>     if django_settings_is_configured():
INTERNALERROR>   File "/Users/dustin/.virtualenvs/myproject/lib/python3.4/site-packages/pytest_django/lazy_django.py", line 23, in django_settings_is_configured
INTERNALERROR>     assert settings.configured is True
INTERNALERROR> AssertionError: assert <django.conf.LazySettings object at 0x10eb0e4e0>.configured is True

@pelme
Copy link
Member

pelme commented Jul 27, 2014

I see, configuring DJANGO_SETTINGS_MODULE in pytest_configure is not really supported/tested at the moment, but should be easy to support properly. Also, you should not need to call setup() explicitly then, pytest-django could take care of it.

I opened a new issue to track this this: #146.

@ionelmc
Copy link
Member Author

ionelmc commented Jul 28, 2014

@dustinfarris Out of curiosity, why do you need to set DJANGO_SETTINGS_MODULE in pytest_configure ?

@dustinfarris
Copy link

@ionelmc to offload the sys.path boilerplate to pytest.

@dustinfarris
Copy link

@ionelmc I guess I put it elsewhere, (tox.ini, etc..), but since conftest.py is already there, why not use it.

@ionelmc
Copy link
Member Author

ionelmc commented Jul 28, 2014

@dustinfarris I was intrigued, cause I always put it in tox.ini as I test with a single project.

@blueyed
Copy link
Contributor

blueyed commented Jul 29, 2014

I am closing this issue since it has been fixed basically and new issues should be / have been opened separately.

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

No branches or pull requests

8 participants