Opened 12 years ago

Closed 12 years ago

#17966 closed Bug (fixed)

Tests fail with trivial usage of AUTH_PROFILE_MODULE

Reported by: Steve Lacy Owned by: nobody
Component: contrib.auth Version: 1.4
Severity: Normal Keywords:
Cc: tiramiseb, rob@…, taylor@…, osirius@…, scotty@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I started a totally blank project using 1.4 release (Version in Ticket in 1.4-rc2 because there's no 1.4 release in the ticket system yet).

I added a new app called "account" with a models.py that looks like this:

from django.db import models
from django.contrib.auth.models import User
    
class Account(models.Model):
    user = models.ForeignKey(User)

I put "account" in INSTALLED_APPS
I put "AUTH_PROFILE_MODULE = 'account.Account'" in settings.py

I ran python ./manage.py test and got:

ERROR: test_site_profile_not_available (django.contrib.auth.tests.models.ProfileTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slacy/src/tmp/env/local/lib/python2.7/site-packages/django/contrib/auth/tests/models.py", line 29, in test_site_profile_not_available
    del settings.AUTH_PROFILE_MODULE
  File "/home/slacy/src/tmp/env/local/lib/python2.7/site-packages/django/utils/functional.py", line 215, in __delattr__
    delattr(self._wrapped, name)
AttributeError: AUTH_PROFILE_MODULE

----------------------------------------------------------------------
Ran 417 tests in 10.302s

FAILED (errors=1, skipped=1)
Destroying test database for alias 'default'...

Attachments (1)

17966-1.diff (2.2 KB ) - added by Claude Paroz 12 years ago.
Possible fix

Download all attachments as: .zip

Change History (30)

comment:1 by Steve Lacy, 12 years ago

Owner: changed from nobody to aagugstin

I've used git bisect to narrow this down to commit(s):

e669c483ffcd8313b51a43f602ba2c8c74fd78bd
c88db46afdcacef8504c5831254b6d5592ac0cf0

Both of which are by aaugustin

comment:2 by Steve Lacy, 12 years ago

Owner: changed from aagugstin to Aymeric Augustin

comment:3 by Steve Lacy, 12 years ago

Version: 1.4-rc-21.4

comment:4 by Aymeric Augustin, 12 years ago

Owner: changed from Aymeric Augustin to nobody

I'm probably going to take a look at this, but the common practice here is *not* to assign tickets to other people. Assigning a ticket to oneself means "I'm currently working on a patch", and in this case, it isn't true.

by Claude Paroz, 12 years ago

Attachment: 17966-1.diff added

Possible fix

comment:5 by Aymeric Augustin, 12 years ago

Component: Uncategorizedcontrib.auth
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

I can see how this happened -- mixing override_settings and code to achieve the same effect manually in setUp and tearDown probably isn't a good idea.

We should refactor the test suite to use override_settings consistently. This is a much broader effort than fixing this specific bug.

It's also a n-th instance of the usual problem: "tests of contrib apps aren't sufficiently isolated"...

comment:6 by rob@…, 12 years ago

I'm experiencing this issue also, on a completely blank project with AUTH_PROFILE_MODULE set.

comment:7 by tiramiseb, 12 years ago

Cc: tiramiseb added

comment:8 by rob@…, 12 years ago

Cc: rob@… added

comment:9 by Taylor Fausak, 12 years ago

Cc: taylor@… added

comment:10 by Sam Bull, 12 years ago

Cc: osirius@… added

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

in reply to:  10 comment:11 by rob@…, 12 years ago

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

I'm in the same situation, and would really like to migrate. I've been trying to figure out a workaround for this, and can't get the test to pass. I certainly don't want to use a custom version of Django in production.

in reply to:  10 ; comment:12 by Aymeric Augustin, 12 years ago

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

in reply to:  12 ; comment:13 by Sam Bull, 12 years ago

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

Is there any way to set the AUTH_PROFILE_MODULE setting without causing this test to error out?

Also, I take it this failure isn't getting caught by django's test suite, so shouldn't that be updated too?

in reply to:  13 ; comment:14 by Carl Meyer, 12 years ago

Replying to SamBull:

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

That was the theory at one time, but it doesn't work very well in practice. There are so many ways to integrate contrib or other third-party apps, oftentimes with customizations, wrapped views, etc., and it's almost impossible for a reusable-app author to write tests that will reliably pass with all functional customized integrations. Over time, as "failing test" reports (just like this one) come in against contrib apps and are fixed, the tests become more and more isolated from the settings of the project they are running in (provide their own templates, temporarily override settings to known values, etc), and eventually lose any value they may have once had as integration tests.

Fundamentally, integration tests are the responsibility of the integrator, who is the only party who knows how their specific integration is supposed to work.

For Django 1.5 it is likely that the default test runner will not run tests from external apps by default. So yes, I would say that changing your continuous integration tests now to match that practice would be sensible, and I don't believe that in practice you'll lose significant value in terms of coverage of your code and settings.

in reply to:  14 ; comment:15 by rob@…, 12 years ago

Replying to carljm:

Replying to SamBull:

Replying to aaugustin:

Replying to SamBull:

Is there a workaround for this issue? It's preventing us from migrating to 1.4 because our deployment system won't let us push out a release with failing tests.

You should only test your own apps, not Django's contrib apps: ./manage.py test myapp1 myapp2 ...

Huh. That's not true is it? Some of django's tests are integration tests that help us verify that we're using everything correctly (providing essential templates, settings, etc.).

That was the theory at one time, but it doesn't work very well in practice. There are so many ways to integrate contrib or other third-party apps, oftentimes with customizations, wrapped views, etc., and it's almost impossible for a reusable-app author to write tests that will reliably pass with all functional customized integrations. Over time, as "failing test" reports (just like this one) come in against contrib apps and are fixed, the tests become more and more isolated from the settings of the project they are running in (provide their own templates, temporarily override settings to known values, etc), and eventually lose any value they may have once had as integration tests.

Fundamentally, integration tests are the responsibility of the integrator, who is the only party who knows how their specific integration is supposed to work.

For Django 1.5 it is likely that the default test runner will not run tests from external apps by default. So yes, I would say that changing your continuous integration tests now to match that practice would be sensible, and I don't believe that in practice you'll lose significant value in terms of coverage of your code and settings.

I find that often the strange edge-cases are caught by running the tests in contrib, which my own tests may not necessarily cover (though this should obviously be rectified, if such a situation is discovered).

For example, the behaviour of custom middleware on login/logout views. I'd feel somewhat uncomfortable excluding Django's tests from the build (is there even a way to do this - or must you specify each of your own apps explicitly on the command line?)

in reply to:  15 comment:16 by Carl Meyer, 12 years ago

I replied on #17365, as this discussion is really more on-topic there.

comment:17 by scotty@…, 12 years ago

Cc: scotty@… added

comment:18 by robgolding63, 12 years ago

I've opened a pull request with a fix for this one: https://github.com/django/django/pull/36.

I know the use of GitHub is new to the Django workflow and some things still need to be worked out, but I figured having it there waiting is the easiest way to show the changes. It's just a single commit, which I think fits with Django's current procedures.

comment:19 by Claude Paroz <claude@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

in reply to:  19 ; comment:20 by Mathijs de Bruin, 12 years ago

Replying to Claude Paroz <claude@…>:

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

The patch seems to fix the initial problem but the tests now fail due to SiteProfileNotAvailable now not being raised in my specific setup. It seems as though override_settings is not doing quite what it ought to do:

  • With AUTH_PROFILE_MODULE commented out in my settings, the test runs fine.
  • When I comment out the full @override_settings line the test runs fine.
  • When I remove the AUTH_PROFILE_MODULE override, the test fails with AttributeError: AUTH_PROFILE_MODULE as in the original bug report.
  • When I remove the USE_TZ override, I get the SiteProfileNotAvailable exception not being raised.

No doubt this is a weird and difficult problem. Let me know if more feedback is required - I personally have no real clue what's going on here and do not have the time to delve much deeper but would love to help this bug get fixed.

Lastly, it seems that Rob Golding's original patch from the pull request passes the unittest just fine.

comment:21 by Claude Paroz, 12 years ago

Please provide the necessary settings to be able to reproduce your problem. Please try also to debug the test to see why SiteProfileNotAvailable is not raised.

in reply to:  20 comment:22 by anonymous, 12 years ago

Replying to dokterbob:

Replying to Claude Paroz <claude@…>:

In [531878302735e6a2b36d82b584947bbf8eae8111]:

Fixed #17966 -- Isolated ProfileTestCase from custom AUTH_PROFILE_MODULE

Thanks Rob Golding for helping on the patch.

The patch seems to fix the initial problem but the tests now fail due to SiteProfileNotAvailable now not being raised in my specific setup. It seems as though override_settings is not doing quite what it ought to do:

  • With AUTH_PROFILE_MODULE commented out in my settings, the test runs fine.
  • When I comment out the full @override_settings line the test runs fine.
  • When I remove the AUTH_PROFILE_MODULE override, the test fails with AttributeError: AUTH_PROFILE_MODULE as in the original bug report.
  • When I remove the USE_TZ override, I get the SiteProfileNotAvailable exception not being raised.

No doubt this is a weird and difficult problem. Let me know if more feedback is required - I personally have no real clue what's going on here and do not have the time to delve much deeper but would love to help this bug get fixed.

Lastly, it seems that Rob Golding's original patch from the pull request passes the unittest just fine.

Just wanted to note that I'm having the same issue on Python 2.7, Django 1.5.0. What seems to be occurring is that prior to line 15: del settings.AUTH_PROFILE_MODULE, settings.AUTH_PROFILE_MODULE reports itself as an empty string. However, after the del call, settings.AUTH_PROFILE_MODULE reports itself as the model defined in my settings.py. As a result, user.get_profile() is able to run fine and thus causes a test failure by not raising an error.

I was able to replicate the issue by doing the following:

At this point, running python manage.py test exhibits the problematic behavior described above where settings.AUTH_PROFILE_MODULE defaults to what I defined in settings.py after the attribute is deleted.

comment:23 by Claude Paroz, 12 years ago

Resolution: fixed
Status: closedreopened

comment:24 by Claude Paroz, 12 years ago

This problem is a symptom of #18824.

comment:25 by robgolding63, 12 years ago

As far as I can see, moving the separate tests to their own functions would remove the need to use del settings.AUTH_PROFILE_MODULE, and so would work around this issue. As it stands there are 3 tests sitting in the same function (test_site_profile_not_available), so it probably makes more sense to break them out?

comment:26 by Claude Paroz, 12 years ago

I don't think so. AUTH_PROFILE_MODULE can be set in the current project settings and therefore I don't see how we can avoid the del. However, you have a point in that the del would probably succeed if the test was not in an override_settings context. But then we should also restore it at the end of the test, exactly the sort of manipulation we want to avoid with override_settings. That's why I still favour the solution proposed in #18824.

comment:27 by robgolding63, 12 years ago

My thinking was that override_settings is a class decorator, so by having AUTH_PROFILE_MODULE set to the empty string (or None), it's effectively unset at the start of each method call. We could then set it as required, or just leave it unset in this particular case.

Perhaps the only way that would work, though, is if the tests were in separate classes altogether - which isn't exactly ideal.

In any case, implementing #18824 will no doubt be useful in other places too - so that's probably the best course of action.

comment:28 by alan.johnson@…, 12 years ago

I'm a rather new Django developer, and I'm trying to build a site in a test-driven way...is the takeaway of this that tests will always fail when following the UserProfile instructions from the docs? Is there a workaround?

comment:29 by Claude Paroz, 12 years ago

Resolution: fixed
Status: reopenedclosed

This should be resolved after commit [22f85b9057825b5b4139abf5fd7e8c4ba0d16981].

@alan.johnson@…:
See comment:12

Note: See TracTickets for help on using tickets.
Back to Top