Canonical SSO provider

Merge lp:~jml/canonical-identity-provider/reliable-tests into lp:canonical-identity-provider/release

  1. reliable-tests
  2. Merge into trunk
Proposed by Jonathan Lange
Status: Merged
Approved by: Michael Foord
Approved revision: no longer in the source branch.
Merged at revision: 549
Proposed branch: lp:~jml/canonical-identity-provider/reliable-tests
Merge into: lp:canonical-identity-provider/release
Diff against target: 98 lines (+27/-5)
4 files modified
django_project/config_dev/config/devel.cfg (+1/-1)
identityprovider/tests/runner.py (+13/-2)
identityprovider/tests/unit/test_command_cleanup.py (+2/-2)
webui/tests/test_decorators.py (+11/-0)
To merge this branch: bzr merge lp:~jml/canonical-identity-provider/reliable-tests
Reviewer Review Type Date Requested Status
Michael Foord (community) Approve
Ricardo Kirkner (community) Approve
Review via email: mp+136932@code.launchpad.net

Commit message

Make decorator tests more reliable and suppress "cleaning nonces" message during test run

Description of the change

Two tests currently fail intermittently: identityprovider.tests.unit.test_decorators.SSOLoginRequiredTestCase.test_decoration_{no,with}_params

They do so because they rely on a feature flag being in a certain state, and because gargoyle's implementation makes feature flag state in tests unreliable.

It uses modeldict which implements two levels of caching: an internal Python cache in a dict and the django cache, which is set to a system memcache in canonical-identity-provider's development config.

The flag in question is 'TWOFACTOR', and it is enabled by the twofactor fixture in test_views_server. This enables the flag in the database, in the django cache and in modeldict's custom cache. Django correctly cleans up the change to the database, but leaves the django cache and modeldict's cache with the flag enabled.

Thus, when test_decoration_{no,with}_params run, they get redirected to the two factor auth page, rather than the correct 'SUCCESS'.

To reliably trigger this failure, do either:
  $ python django_project/manage.py test --noinput \
    identityprovider.tests.unit.test_views_server.Decide2FTestCase \
    identityprovider.tests.unit.test_decorators.SSOLoginRequiredTestCase.test_decoration_{no,with}_params

Or:
  $ python django_project/manage.py test --noinput \
    identityprovider.tests.unit.test_views_server.Decide2FTestCase
  $ python django_project/manage.py test --noinput \
    identityprovider.tests.unit.test_decorators.SSOLoginRequiredTestCase.test_decoration_{no,with}_params

They both exercise different caching bugs.

This branch begins to address the problem by setting the timeout of modeldict's own cache to 0, thus effectively disabling its cache in unit tests.

It does not address the problem of django's cache containing invalid data. That is work-in-progress.

Options:
 1. disable django's cache in tests using the dummy cache
 2. run cache.clear() after each test
 3. write a cache decorator that records all keys added, set & deleted and then provides a mechanism for reversing those operations. call this mechanism after each test
 4. work-around the problem in the failing tests (e.g. rewrite them to expect redirects sometimes, have them flush the cache, etc.)

Option 1 breaks many, many tests. I haven't dug into why.

Options 2 & 3 are slightly more difficult due to identityprovider not having a standard base TestCase, thus having no easy way to do something after every test.

Option 4 just means this problem will bite us later.

Option 3 is currently my favourite.

I hope to return to this tomorrow, but will be happy if someone takes it from me.

See also:
  https://groups.google.com/d/topic/django-developers/Ex7X2oFAHoc/discussion

jml

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

The changes to identityprovider/tests/unit/test_command_cleanup.py are to prevent unnecessary writing to stdout. The noise made the problem harder to debug.

Revision history for this message
Ricardo Kirkner (ricardokirkner) wrote :

Wow, the work you've done here is impressive. Thanks a lot for digging into it.

I am quite annoyed at finding about this cache in gargoyle this way but I can only blame myself for it (it doesn't seem to be documented anywhere in gargoyle) even though it's part of it's public api and clearly affects how you work with tests involving flags.

The simplest change we could do (though we might be affected by the caching issue later as you mention) is to decorate the failing tests with a decorator that makes sure the TWOFACTOR flags is disabled while running the tests. This would look like

@switches(TWOFACTOR=False)
def test_decoration_{no,with}_params(self):
...

If you still think we should do the caching work to prevent other possible future issues, I'm ok with you exploring that route.

thanks,
Ricardo

review: Approve
Revision history for this message
Jonathan Lange (jml) wrote :

On 29 November 2012 20:19, Ricardo Kirkner
<email address hidden> wrote:
> Review: Approve
>
> Wow, the work you've done here is impressive. Thanks a lot for digging into it.
>
> I am quite annoyed at finding about this cache in gargoyle this way but I can only blame myself for it (it doesn't seem to be documented anywhere in gargoyle) even though it's part of it's public api and clearly affects how you work with tests involving flags.
>
> The simplest change we could do (though we might be affected by the caching issue later as you mention) is to decorate the failing tests with a decorator that makes sure the TWOFACTOR flags is disabled while running the tests. This would look like
>
> @switches(TWOFACTOR=False)
> def test_decoration_{no,with}_params(self):
> ...
>
> If you still think we should do the caching work to prevent other possible future issues, I'm ok with you exploring that route.

I do. At the moment I think the "right" thing to do is wrap Django's
cache layer, record cache entries & clear them out at the end of
tests. I'll chase that up in https://launchpad.net/djangofixture,
since it's got little to do w/ openid & will probably be useful for
other things.

In the mean time, will add the switches decorator (a variant of option
4) so we can get some momentum on pre-merge testing.

jml

Revision history for this message
Michael Foord (mfoord) wrote :

Looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'django_project/config_dev/config/devel.cfg'
2--- django_project/config_dev/config/devel.cfg 2012-12-06 13:33:51 +0000
3+++ django_project/config_dev/config/devel.cfg 2012-12-10 10:08:19 +0000
4@@ -58,7 +58,7 @@
5 # identityprovider.middleware.profile.ProfileMiddleware
6 # allow cookies to be stored when accessing via http
7 session_cookie_secure = false
8-test_runner = discover_runner.runner.DiscoverRunner
9+test_runner = identityprovider.tests.runner.IsolatedTestRunner
10
11 [__main__]
12 db_connections = db_master
13
14=== modified file 'identityprovider/tests/runner.py'
15--- identityprovider/tests/runner.py 2012-07-23 16:43:08 +0000
16+++ identityprovider/tests/runner.py 2012-12-10 10:08:19 +0000
17@@ -4,8 +4,19 @@
18 from django_jenkins import signals
19 from django_jenkins.runner import CITestSuiteRunner
20
21-
22-class DiscoveryCITestSuiteRunner(CITestSuiteRunner, DiscoverRunner):
23+from gargoyle import gargoyle
24+
25+
26+class IsolatedTestRunner(DiscoverRunner):
27+
28+ def setup_test_environment(self, *args, **kwargs):
29+ super(IsolatedTestRunner, self).setup_test_environment(*args, **kwargs)
30+ # gargoyle caches settings. Setting timeout to 0 prevents this,
31+ # increasing test isolation.
32+ gargoyle.timeout = 0
33+
34+
35+class DiscoveryCITestSuiteRunner(CITestSuiteRunner, IsolatedTestRunner):
36 """Discovery based test runner for running with django-jenkins."""
37 def build_suite(self, test_labels, extra_tests=None, **kwargs):
38 suite = DiscoverRunner.build_suite(
39
40=== modified file 'identityprovider/tests/unit/test_command_cleanup.py'
41--- identityprovider/tests/unit/test_command_cleanup.py 2012-10-29 16:06:36 +0000
42+++ identityprovider/tests/unit/test_command_cleanup.py 2012-12-10 10:08:19 +0000
43@@ -54,7 +54,7 @@
44
45 def test_cleanup_sessions(self):
46 self.populate()
47- call_command('cleanup', sessions=True)
48+ call_command('cleanup', sessions=True, verbosity=0)
49
50 self.assertEqual(0, Session.objects.filter(
51 expire_date__lt=datetime.utcnow()).count())
52@@ -62,7 +62,7 @@
53
54 def test_cleanup_nonces(self):
55 self.populate()
56- call_command('cleanup', nonces=True)
57+ call_command('cleanup', nonces=True, verbosity=0)
58
59 self.assertEqual(500, Session.objects.count())
60 self.assertEqual(0, OpenIDNonce.objects.filter(timestamp__lt=(
61
62=== modified file 'webui/tests/test_decorators.py'
63--- webui/tests/test_decorators.py 2012-12-03 20:19:22 +0000
64+++ webui/tests/test_decorators.py 2012-12-10 10:08:19 +0000
65@@ -1,6 +1,7 @@
66 from datetime import datetime, timedelta
67 from django.conf import settings
68 from django.test import TestCase
69+from gargoyle.testutils import switches
70
71 from mock import ANY, Mock, patch, MagicMock
72
73@@ -142,7 +143,12 @@
74 self.assertEqual(response.status_code, 302)
75 self.assertEqual(response['Location'], '/two_factor_auth?next=/target')
76
77+ @switches(TWOFACTOR=False)
78 def test_decoration_no_params(self):
79+ # Needs two-factor authentication to be disabled so we get 'SUCCESS'
80+ # rather than a redirect. Because of
81+ # https://github.com/disqus/gargoyle/issues/57, we cannot rely on the
82+ # switch being in any particular state, so we use a decorator.
83
84 @sso_login_required
85 def view(request, *args, **kwargs):
86@@ -152,7 +158,12 @@
87 # if this works, the decorator handles this invocation properly
88 self.assertEqual(response, 'SUCCESS')
89
90+ @switches(TWOFACTOR=False)
91 def test_decoration_with_params(self):
92+ # Needs two-factor authentication to be disabled so we get 'SUCCESS'
93+ # rather than a redirect. Because of
94+ # https://github.com/disqus/gargoyle/issues/57, we cannot rely on the
95+ # switch being in any particular state, so we use a decorator.
96
97 @sso_login_required(login_url='/alt_login')
98 def view(request, *args, **kwargs):