Launchpad itself

Merge lp:~mwhudson/launchpad/unconditionally-endInteraction-after-test into lp:launchpad

  1. unconditionally-endInteraction-after-test
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 14107
Proposed branch: lp:~mwhudson/launchpad/unconditionally-endInteraction-after-test
Merge into: lp:launchpad
Diff against target: 170 lines (+7/-36)
5 files modified
lib/canonical/launchpad/ftests/__init__.py (+0/-2)
lib/canonical/testing/layers.py (+6/-8)
lib/lp/testing/__init__.py (+0/-3)
lib/lp/testing/_login.py (+1/-15)
lib/lp/testing/tests/test_login.py (+0/-8)
To merge this branch: bzr merge lp:~mwhudson/launchpad/unconditionally-endInteraction-after-test
Reviewer Review Type Date Requested Status
Steve Kowalik (community) code Approve
Review via email: mp+78324@code.launchpad.net

Commit message

[r=stevenk][no-qa] Unconditionally tear down the interaction after each test and remove weasel worded comments

Description of the change

While poking around how we handle logging in and out (aka setting up and tearing down interactions) in tests I noticed some weasel behaviour in layers.py:

        # If tests forget to logout, we can do it for them.
        if is_logged_in():
            logout()

Well, this is just sucky: either we should unconditionally tear down the interaction, or we should fail tests that don't log out. The latter would make for a lot of cruft in doctests in particular, so let's just always log out.

Also, this only tears down interactions that have been set up by the specific test login helpers (that's what the is_logged_in() function is about). This is also daft: it's safe to call endInteraction() even if there is no interaction (maybe this wasn't always true), so let's just call that after every test.

This is the only use of the is_logged_in() function, so let's kill that and the global variable that drives it too.

To post a comment you must log in.
Revision history for this message
Steve Kowalik (stevenk) wrote :

This looks like excellent work, thank you!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/ftests/__init__.py'
2--- lib/canonical/launchpad/ftests/__init__.py 2011-06-29 12:01:59 +0000
3+++ lib/canonical/launchpad/ftests/__init__.py 2011-10-05 22:12:26 +0000
4@@ -9,7 +9,6 @@
5 'import_public_key',
6 'import_public_test_keys',
7 'import_secret_test_key',
8- 'is_logged_in',
9 'LaunchpadFormHarness',
10 'login',
11 'login_person',
12@@ -27,7 +26,6 @@
13 )
14 from lp.testing import (
15 ANONYMOUS,
16- is_logged_in,
17 login,
18 login_person,
19 logout,
20
21=== modified file 'lib/canonical/testing/layers.py'
22--- lib/canonical/testing/layers.py 2011-10-03 14:39:59 +0000
23+++ lib/canonical/testing/layers.py 2011-10-05 22:12:26 +0000
24@@ -95,7 +95,10 @@
25 )
26 from zope.component.interfaces import ComponentLookupError
27 import zope.publisher.publish
28-from zope.security.management import getSecurityPolicy
29+from zope.security.management import (
30+ endInteraction,
31+ getSecurityPolicy,
32+ )
33 from zope.security.simplepolicies import PermissiveSecurityPolicy
34 from zope.server.logger.pythonlogger import PythonLogger
35
36@@ -145,7 +148,6 @@
37 from lp.services.rabbit.server import RabbitServer
38 from lp.testing import (
39 ANONYMOUS,
40- is_logged_in,
41 login,
42 logout,
43 )
44@@ -1355,9 +1357,7 @@
45 def testTearDown(cls):
46 getUtility(IOpenLaunchBag).clear()
47
48- # If tests forget to logout, we can do it for them.
49- if is_logged_in():
50- logout()
51+ endInteraction()
52
53 # Disconnect Storm so it doesn't get in the way of database resets
54 disconnect_stores()
55@@ -1386,9 +1386,7 @@
56 def testTearDown(cls):
57 getUtility(IOpenLaunchBag).clear()
58
59- # If tests forget to logout, we can do it for them.
60- if is_logged_in():
61- logout()
62+ endInteraction()
63
64 # Reset any statistics
65 from canonical.launchpad.webapp.opstats import OpStats
66
67=== modified file 'lib/lp/testing/__init__.py'
68--- lib/lp/testing/__init__.py 2011-09-22 21:02:33 +0000
69+++ lib/lp/testing/__init__.py 2011-10-05 22:12:26 +0000
70@@ -19,7 +19,6 @@
71 'extract_lp_cache',
72 'FakeTime',
73 'get_lsb_information',
74- 'is_logged_in',
75 'launchpadlib_credentials_for',
76 'launchpadlib_for',
77 'login',
78@@ -144,7 +143,6 @@
79 from lp.testing._login import (
80 anonymous_logged_in,
81 celebrity_logged_in,
82- is_logged_in,
83 login,
84 login_as,
85 login_celebrity,
86@@ -172,7 +170,6 @@
87 anonymous_logged_in
88 api_url
89 celebrity_logged_in
90-is_logged_in
91 launchpadlib_credentials_for
92 launchpadlib_for
93 login_as
94
95=== modified file 'lib/lp/testing/_login.py'
96--- lib/lp/testing/_login.py 2011-09-21 00:24:13 +0000
97+++ lib/lp/testing/_login.py 2011-10-05 22:12:26 +0000
98@@ -14,7 +14,6 @@
99 'login_person',
100 'login_team',
101 'logout',
102- 'is_logged_in',
103 'person_logged_in',
104 'run_with_login',
105 'with_anonymous_login',
106@@ -43,20 +42,9 @@
107 from lp.testing.sampledata import ADMIN_EMAIL
108
109
110-_logged_in = False
111-
112-
113-def is_logged_in():
114- global _logged_in
115- return _logged_in
116-
117-
118 def _test_login_impl(participation):
119 # Common implementation of the test login wrappers.
120- # It sets the global _logged_in flag and create a default
121- # participation if None was specified.
122- global _logged_in
123- _logged_in = True
124+ # It creates a default participation if None was specified.
125
126 if participation is None:
127 # we use the main site as the host name. This is a guess, to make
128@@ -144,8 +132,6 @@
129 canonical.launchpad.ftest.LaunchpadFunctionalTestCase's tearDown method so
130 you generally won't need to call this.
131 """
132- global _logged_in
133- _logged_in = False
134 endInteraction()
135
136
137
138=== modified file 'lib/lp/testing/tests/test_login.py'
139--- lib/lp/testing/tests/test_login.py 2011-09-21 00:15:46 +0000
140+++ lib/lp/testing/tests/test_login.py 2011-10-05 22:12:26 +0000
141@@ -17,7 +17,6 @@
142 ANONYMOUS,
143 anonymous_logged_in,
144 celebrity_logged_in,
145- is_logged_in,
146 login,
147 login_as,
148 login_celebrity,
149@@ -74,21 +73,14 @@
150 def test_not_logged_in(self):
151 # After logout has been called, we are not logged in.
152 logout()
153- self.assertEqual(False, is_logged_in())
154 self.assertLoggedOut()
155
156 def test_logout_twice(self):
157 # Logging out twice don't harm anybody none.
158 logout()
159 logout()
160- self.assertEqual(False, is_logged_in())
161 self.assertLoggedOut()
162
163- def test_logged_in(self):
164- # After login has been called, we are logged in.
165- login_person(self.factory.makePerson())
166- self.assertEqual(True, is_logged_in())
167-
168 def test_login_person_actually_logs_in(self):
169 # login_person changes the currently logged in person.
170 person = self.factory.makePerson()