Closed
Bug 656648
Opened 13 years ago
Closed 13 years ago
[homepage] tests break because of LDAP authentication backend actually working in tests
Categories
(Webtools Graveyard :: Elmo, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: peterbe, Assigned: peterbe)
References
()
Details
Attachments
(1 file, 3 obsolete files)
5.77 KB,
patch
|
Pike
:
review+
|
Details | Diff | Splinter Review |
To run all tests, including those in apps/homepage/tests.py and those in lib/auth/tests.py you need to have some ldap_settings. If you have ldap_settings, the tests in 'homepage' will try to go out on the network and connect to an LDAP server. LDAP authentication should be allowed to be in play during tests but individual tests orthogonal to LDAP authentication should work without the network.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #531964 -
Flags: review?(l10n)
Comment 2•13 years ago
|
||
Comment on attachment 531964 [details] [diff] [review] Patch to greatly simplify scripts/build.sh and to make all tests run without needing LDAP network access Review of attachment 531964 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following comments: ::: scripts/build.sh @@ +34,5 @@ > " > settings/local.py > +# these settings are overwritten by the test setups but the file must exist > +echo " > +LDAP_HOST = LDAP_DN = LDAP_PASS = 'test' > +" > settings/ldap_settings.py Make this copy over ldap_settings.py-dist instead? That way we ensure that our example -dist data is actually useful for contributors, and I think that should work. Which also means that we need to fix ldap_settings.py-dist to use non-empty strings for the LDAP_* settings.
Attachment #531964 -
Flags: review?(l10n) → review+
Assignee | ||
Comment 3•13 years ago
|
||
as discussed on irc
Attachment #531964 -
Attachment is obsolete: true
Attachment #531978 -
Flags: review?(l10n)
Comment 4•13 years ago
|
||
Comment on attachment 531978 [details] [diff] [review] Use ldap_settings.py-dist instead So those asserts in auth/backends.py aren't doing anything reliably, obviously. If you run python -O, they don't do stuff, if you run plain python, they assert. Where the web says that -O can be default, even. Anyways, making the error reporting around our ldap backend do the right thing is a follow up bug. I want our -dist settings to satisfy the assertions for the time being, so make them non-empty strings in ldap_settings.py-dist?
Attachment #531978 -
Flags: review?(l10n) → review-
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > flag: review?(l10n@mozilla.com) => review-Comment on attachment 531978 [details] [diff] [review] > [details] [review] > Use ldap_settings.py-dist instead > > So those asserts in auth/backends.py aren't doing anything reliably, > obviously. If you run python -O, they don't do stuff, if you run plain > python, they assert. Where the web says that -O can be default, even. > The asserts in auth/backends.py makes sure the developer he doesn't try to run the server without having configured his LDAP settings. No one runs django with `python -O` and if you do you'll just get LDAP related errors. > Anyways, making the error reporting around our ldap backend do the right > thing is a follow up bug. > > I want our -dist settings to satisfy the assertions for the time being, so > make them non-empty strings in ldap_settings.py-dist? I have another idea. Instead of this assertion business, how about a proper error raised. The enabling ldap_settings.py (i.e. copying from ldap_settings.py-dist) means you want to use LDAP and you must then type in the right DN, password etc. If you don't do this, it won't be noticed until you run the server. So how about something like this (ignore the excessively verbose comments): # in settings/base.py try: import ldap_settings # cool, if created, all settings must be entered for each in 'LDAP_HOST', 'LDAP_DN', 'LDAP_PASS': if not getattr(ldap_settings, each, None): raise ValueError('%s must be set' % each) except ImportError: ldap_loaded = False if ldap_loaded: # ImportErrors are not acceptable if ldap_loaded is True import ldap This way, you notice straight away what's wrong if you've tried to enable LDAP. Thoughts?
Comment 6•13 years ago
|
||
Yeah, sounds good. Let's table the discussion on where to put that code in case we liberate our ldap auth backend for use in other webdev django projects. That'd be: - remove the asserts - change base.py as above (*) - update ldap_settings.py-dist to have non-empty strings (as they'd ValueError?) You'll a from ldap_settings import * to put the LDAP* into django settings. Not sure if the check is better to be if each not in locals(): ? You didn't make an explicit ldap_loaded=True, probably just a typo. Just technicalities, make it work ;-)
Assignee | ||
Comment 7•13 years ago
|
||
settings/base.py is getting ugly but it works.
Attachment #531978 -
Attachment is obsolete: true
Attachment #532271 -
Flags: review?(l10n)
Comment 8•13 years ago
|
||
Comment on attachment 532271 [details] [diff] [review] Now errors are raised early and explicitely if any ldap settings are wrong Review of attachment 532271 [details] [diff] [review]: ----------------------------------------------------------------- ::: settings/base.py @@ +278,2 @@ > ldap_loaded = False > Maybe, instead of ldap_loaded, you could do something along the lines: try: import ldap_settings except ImportError: pass else: for each in 'LDAP_HOST', 'LDAP_DN', 'LDAP_PASS': if not getattr(ldap_settings, each, None): raise ValueError('%s must be set' % each) from ldap_settings import * # ImportErrors are not acceptable if ldap_loaded is True import ldap MIDDLEWARE_CLASSES = (MIDDLEWARE_CLASSES + ('django.contrib.auth.middleware.RemoteUserMiddleware',)) AUTHENTICATION_BACKENDS = ('lib.auth.backends.MozLdapBackend',) Not sure if that qualifies as prettier or uglier, though :)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #532271 -
Attachment is obsolete: true
Attachment #532271 -
Flags: review?(l10n)
Attachment #532344 -
Flags: review?(l10n)
Comment 10•13 years ago
|
||
Comment on attachment 532344 [details] [diff] [review] Updated patch with Stas's feedback. No more ldap_loaded variable This patch got good enough. r=me. I still expect that ldap_settings.py-dist needs non-'' values for the bool check to succeed, if so, land a fix to that extent with r=me assumed?
Attachment #532344 -
Flags: review?(l10n) → review+
Assignee | ||
Comment 11•13 years ago
|
||
Landed on origin/develop https://github.com/mozilla/elmo/commit/afba746f4c19c1613ce58f492f6ff862e1d36b14
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•13 years ago
|
||
Also, Pike, as per your suggestion (assuming I understood it) I added default settings to ldap_settings.py-dist that aren't useless. https://github.com/mozilla/elmo/commit/d57fa7873a03c8357b7b00fda10b7059ede9990d Now the whole test suite passes by running scripts/build.sh
Updated•13 years ago
|
Target Milestone: --- → 1.2
Updated•10 years ago
|
Assignee: nobody → peterbe
Updated•4 years ago
|
Product: Webtools → Webtools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•