launchpad-reviewers team mailing list archive

[Michael Hudson-Doyle <michael.hudson+lp@xxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging lp:~mwhudson/launchpad/unconditionally-endInteraction-after-test into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~mwhudson/launchpad/unconditionally-endInteraction-after-test/+merge/78324

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.
-- 
https://code.launchpad.net/~mwhudson/launchpad/unconditionally-endInteraction-after-test/+merge/78324
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/unconditionally-endInteraction-after-test into lp:launchpad.

=== modified file 'lib/canonical/launchpad/ftests/__init__.py'
--- lib/canonical/launchpad/ftests/__init__.py	2011-06-29 12:01:59 +0000
+++ lib/canonical/launchpad/ftests/__init__.py	2011-10-05 21:50:30 +0000
@@ -9,7 +9,6 @@
     'import_public_key',
     'import_public_test_keys',
     'import_secret_test_key',
-    'is_logged_in',
     'LaunchpadFormHarness',
     'login',
     'login_person',
@@ -27,7 +26,6 @@
     )
 from lp.testing import (
     ANONYMOUS,
-    is_logged_in,
     login,
     login_person,
     logout,

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-10-03 14:39:59 +0000
+++ lib/canonical/testing/layers.py	2011-10-05 21:50:30 +0000
@@ -95,7 +95,10 @@
     )
 from zope.component.interfaces import ComponentLookupError
 import zope.publisher.publish
-from zope.security.management import getSecurityPolicy
+from zope.security.management import (
+    endInteraction,
+    getSecurityPolicy,
+    )
 from zope.security.simplepolicies import PermissiveSecurityPolicy
 from zope.server.logger.pythonlogger import PythonLogger
 
@@ -145,7 +148,6 @@
 from lp.services.rabbit.server import RabbitServer
 from lp.testing import (
     ANONYMOUS,
-    is_logged_in,
     login,
     logout,
     )
@@ -1355,9 +1357,7 @@
     def testTearDown(cls):
         getUtility(IOpenLaunchBag).clear()
 
-        # If tests forget to logout, we can do it for them.
-        if is_logged_in():
-            logout()
+        endInteraction()
 
         # Disconnect Storm so it doesn't get in the way of database resets
         disconnect_stores()
@@ -1386,9 +1386,7 @@
     def testTearDown(cls):
         getUtility(IOpenLaunchBag).clear()
 
-        # If tests forget to logout, we can do it for them.
-        if is_logged_in():
-            logout()
+        endInteraction()
 
         # Reset any statistics
         from canonical.launchpad.webapp.opstats import OpStats

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-09-22 21:02:33 +0000
+++ lib/lp/testing/__init__.py	2011-10-05 21:50:30 +0000
@@ -19,7 +19,6 @@
     'extract_lp_cache',
     'FakeTime',
     'get_lsb_information',
-    'is_logged_in',
     'launchpadlib_credentials_for',
     'launchpadlib_for',
     'login',
@@ -144,7 +143,6 @@
 from lp.testing._login import (
     anonymous_logged_in,
     celebrity_logged_in,
-    is_logged_in,
     login,
     login_as,
     login_celebrity,
@@ -172,7 +170,6 @@
 anonymous_logged_in
 api_url
 celebrity_logged_in
-is_logged_in
 launchpadlib_credentials_for
 launchpadlib_for
 login_as

=== modified file 'lib/lp/testing/_login.py'
--- lib/lp/testing/_login.py	2011-09-21 00:24:13 +0000
+++ lib/lp/testing/_login.py	2011-10-05 21:50:30 +0000
@@ -14,7 +14,6 @@
     'login_person',
     'login_team',
     'logout',
-    'is_logged_in',
     'person_logged_in',
     'run_with_login',
     'with_anonymous_login',
@@ -43,20 +42,12 @@
 from lp.testing.sampledata import ADMIN_EMAIL
 
 
-_logged_in = False
-
-
-def is_logged_in():
-    global _logged_in
-    return _logged_in
 
 
 def _test_login_impl(participation):
     # Common implementation of the test login wrappers.
     # It sets the global _logged_in flag and create a default
     # participation if None was specified.
-    global _logged_in
-    _logged_in = True
 
     if participation is None:
         # we use the main site as the host name.  This is a guess, to make
@@ -144,8 +135,6 @@
     canonical.launchpad.ftest.LaunchpadFunctionalTestCase's tearDown method so
     you generally won't need to call this.
     """
-    global _logged_in
-    _logged_in = False
     endInteraction()
 
 

=== modified file 'lib/lp/testing/tests/test_login.py'
--- lib/lp/testing/tests/test_login.py	2011-09-21 00:15:46 +0000
+++ lib/lp/testing/tests/test_login.py	2011-10-05 21:50:30 +0000
@@ -17,7 +17,6 @@
     ANONYMOUS,
     anonymous_logged_in,
     celebrity_logged_in,
-    is_logged_in,
     login,
     login_as,
     login_celebrity,
@@ -74,21 +73,14 @@
     def test_not_logged_in(self):
         # After logout has been called, we are not logged in.
         logout()
-        self.assertEqual(False, is_logged_in())
         self.assertLoggedOut()
 
     def test_logout_twice(self):
         # Logging out twice don't harm anybody none.
         logout()
         logout()
-        self.assertEqual(False, is_logged_in())
         self.assertLoggedOut()
 
-    def test_logged_in(self):
-        # After login has been called, we are logged in.
-        login_person(self.factory.makePerson())
-        self.assertEqual(True, is_logged_in())
-
     def test_login_person_actually_logs_in(self):
         # login_person changes the currently logged in person.
         person = self.factory.makePerson()