pytest-django 2.7.0 breaks usage with pytest-cov

[carljm]

With the upgrade from 2.6.2 to 2.7.0 (on a Django 1.7 project), suddenly a bunch of module-level code is recorded as uncovered by pytest-cov.

I believe this is because django.setup() is now called sooner, before pytest-cov has a chance to start coverage measurement. Probably due to 85581fb

Still exploring, will suggest a fix once I find one.

[carljm]

It seems that fixing this will basically require rolling back a bunch of changes that were made in 2.7 that had the effect of importing and configuring Django eagerly rather than lazily. I'm not sure what the purpose of those changes was - was it just a matter of better error reporting on failure to import settings?

I can submit a pull request to go back to lazy configuration of Django, as it seems to me that that is better (and doesn't break pytest-cov), but I would be interested first in knowing what the reasons were for moving to eager configuration. Do you have any input, @pelme ?

[pelme]

The change to call django.setup() eagerly was intentional, most of the discussion that lead to that change happened in #119. I also discussed and had help from Jannis Leidel and other Django contributors at the EuroPython sprint. I can see that the commit and test does not communicate the motivation and consideration that went into this change.

#119 (comment) describes the reasoning why this is needed: pytest-django won't be able to support Django 1.9 at all unless django.setup() is called before any Django related import.

From my point of view, your suggestion to lazy import during test collection and then call django.setup() before anything actually runs (test/fixture invocations) makes a lot of sense. This is the way I expect the Python imports to generally work and it makes it easier to reason about imports in that way.

I can see two possible solutions (to solve the original coverage issue):

1. Somehow, by changing Django (I'm not sure if this is possible or what side effects such a change will have) lift the restriction that django.setup() must do the initial import of all models and revert pytest-django to the pre 85581fb behavior.

2. Reorganize hooks/coordinate the point where coverage instrumentation starts with pytest-cov to handle this. I am not sure how difficult this is, but it seems problematic since pytest-django calls django.setup() during the initial conftest loading phase. Other pytest plugins (pytest-cov) may not have been loaded here since it is very early. The reason that pytest-django loads things this early is to be able to access Django things with normal imports in conftest.py files (which is also a pytest plugin). If Django is configured from the standard pytest_configure hook, it would not be possible to do any imports at the module level in project conftest.py files, which would be very bad.

[carljm]

I see. I already limit module-level imports in conftest.py files because it causes the same missing-coverage issue, so for that reason I haven't run into this problem; even with -Wall there are no warnings about too-early model imports.

I'm not a fan of Django 1.7+ trying to control the import order in this way, but I understand the reasons why that was done and the difficulties doing it any other way, given the constraints of Django's design. In theory I would still like to look into the possibility of relaxing these import-order restrictions, but I'm not sanguine about the likelihood of success.

I am also not sure that it will be possible to redesign pytest-cov to start coverage so early, but I can look into that too.

For my own projects it almost seems simplest to just abandon pytest-cov and use coverage run py.test instead, though it will be unfortunate if pytest-django and pytest-cov are permanently incompatible.

Last question: I see that in the current code in master, _setup_django() is called twice, once in pytest_load_initial_conftests hook and then again in a pytest_configure hook. Is that necessary?

[pelme]

It is certainly a bad situation if pytest-cov and pytest-django is not compatible. I have not actually configured coverage reporting for my own projects yet.. it is on the TODO-list. :(

I quick observation (I have not tried this yet): The pytest_load_initial_conftests hook in pytest-cov specifies the pytest.mark.try_last marker - which is actually misspelled (should be trylast without the _) and therefore does not actually do anything. I am not sure what purpose it serves, but if it never actually worked the way it was intended anyways, maybe it could be removed?

If pytest-django used the pytest.mark.trylast marker for its pytest_load_initial_conftests, django.setup() should be called after pytest-covs startup. I have not a project where I can try this out right now but I will try to set something up where I can test this. It might be a solution with a trivial fix. :)

Regarding the second _setup_django() call: _setup_django() is called in a pytest_configure hook with the pytest.mark.trylast marker, which means it will run after other pytest_configure hooks. This makes it possible to not set DJANGO_SETTINGS_MODULE and instead call django.conf.settings.configure() from a project pytest_configure conftest.py hook, but still have django.setup() called. It's fine to call django.setup() multiple times, therefore no check is made if it has been done earlier. This feature is documented here: http://pytest-django.readthedocs.org/en/latest/configuring_django.html#using-django-conf-settings-configure

Thanks for taking the time looking into this, this kind of feedback is very welcome!

[carljm]

Unfortunately the pytest_load_initial_conftests hook in pytest-cov is not where it actually starts coverage measurement; that doesn't happen until a pytest_sessionstart hook (in the CovPlugin class). And I think the use of pytest_sessionstart hook in pytest-cov is necessary for its support for coverage measurement on distributed testing (which I don't fully understand since I haven't used it). This is why I am not confident that it will be feasible to change pytest-cov in such a way as to make it compatible with the current behavior of pytest-django. I think the requirement "must be able to make arbitrary module-level imports in conftest.py" is fundamentally incompatible with the design of pytest-cov.

I wonder if we could lower our aim here a bit and have there be some explicit way to tell pytest-django "I am ok with not being able to do Django imports at module level in my conftest.py files, please don't call django.setup() so early." Since this restriction on module-level imports in conftest.py is basically necessary anyway for effective use of pytest-cov, this would preserve the default behavior you are aiming for here, while still providing a workable path for pytest-cov users.

[carljm]

FWIW, on further reflection I've decided that in fact pytest-django's new behavior is correct, and the fault here really lies with pytest-cov. I've filed pytest-dev/pytest-cov#19 for this, but I think for myself I am going to switch to coverage run and abandon pytest-cov; I think in the long run that's a more reliable approach anyway. So I am not likely to try to sort out this incompatibility further.

[carljm]

...and to compound my embarrassment, it turns out that a new version of pytest-cov has already been released that actually does fix this problem; I just didn't understand well enough how various hooks (including the undocumented pytest_sessionstart) interact and when they fire.

So I think there is now no problem here, and this can be closed. Apologies for all the noise :/

[pelme]

Thanks a lot for taking the time and effort to dig into this rather involved issue. Your contributions (to Django, testing in Python and other projects) are much appreciated!