Merge lp:~salgado/launchpad/fix-readonly-test-helpers into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

Change the read-only test helpers to assert that the mode switch was
successful, thus forcing the mode change to be logged at the moment they
happen.

[Brad Crittenden (bac)]

review: Approve (code)

[Guilherme Salgado (salgado)]

Thanks for the review, Brad, but my changes have broken an existing test
because it was calling remove_read_only_file() after calling
publication.beforeTraversal(request). That's problematic because the
mode switch can't actually happen at that point (it can only happen at
the beginning of a request) so the new assertion I added was failing.

The change below makes the new assertion conditional on a newly added
method argument, so that it can be skipped when needed.

=== modified file 'lib/canonical/launchpad/tests/readonly.py'
--- lib/canonical/launchpad/tests/readonly.py 2010-01-26 13:06:36 +0000
+++ lib/canonical/launchpad/tests/readonly.py 2010-01-26 17:41:29 +0000
@@ -31,9 +31,17 @@
     assert is_read_only(), "Switching to read-only failed."

-def remove_read_only_file():
- """Remove the file named read-only.txt from the root of the tree."""
+def remove_read_only_file(assert_mode_switch=True):
+ """Remove the file named read-only.txt from the root of the tree.
+
+ May also assert that the mode switch actually happened (i.e. not
+ is_read_only()). This assertion has to be conditional because some tests
+ will use this during the processing of a request, when a mode change can't
+ happen (i.e. is_read_only() will still return True during that request's
+ processing, even though the read-only.txt file has been removed).
+ """
     os.remove(read_only_file_path)
- # Assert that the switch succeeded and make sure the mode change is
- # logged.
- assert not is_read_only(), "Switching to read-write failed."
+ if assert_mode_switch:
+ # Assert that the switch succeeded and make sure the mode change is
+ # logged.
+ assert not is_read_only(), "Switching to read-write failed."

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py 2010-01-19 04:22:17 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py 2010-01-26 17:53:28 +0000
@@ -23,6 +23,7 @@
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.interfaces.oauth import IOAuthConsumerSet
 from canonical.launchpad.ftests import ANONYMOUS, login
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.tests.readonly import (
     remove_read_only_file, touch_read_only_file)
 import canonical.launchpad.webapp.adapter as dbadapter
@@ -79,6 +80,9 @@
         # Cleanup needed so that further tests can start processing other
         # requests (e.g. calling beforeTraversal).
         self.publication.endRequest(self.request, None)
+ # Force pending mode switches to actually happen and get logged so
+ # that we don't interfere with other tests.
+ assert not is_read_only()

     def setUp(self):
         TestCase.setUp(self)
@@ -127,7 +131,10 @@
             touch_read_only_file()
             self.publication.beforeTraversal(self.request)
         finally:
- remove_read_only_file()
+ # Tell remove_read_only_file() to not assert that the mode switch
+ # actually happened, as we know it won't happen until this request
+ # ...

[Brad Crittenden (bac)]

I've looked at your mods and it looks good.

self.request, None)
+ # Force pending mode switches to actually happen and get logged so
+ # that we don't interfere with other tests.
+ assert not is_read_only()

Please add a failure message to this assert.

review: Approve (code)

[Gary Poster (gary)]

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/tests/readonly.py'
--- lib/canonical/launchpad/tests/readonly.py	2010-01-13 20:06:09 +0000
+++ lib/canonical/launchpad/tests/readonly.py	2010-01-27 11:28:15 +0000
@@ -13,7 +13,7 @@
 import os
 
 from canonical.launchpad.readonly import (
-    read_only_file_exists, read_only_file_path)
+    is_read_only, read_only_file_exists, read_only_file_path)
 
 
 def touch_read_only_file():
@@ -26,9 +26,22 @@
         "already exists.")
     f = open(read_only_file_path, 'w')
     f.close()
-
-
-def remove_read_only_file():
-    """Remove the file named read-only.txt from the root of the tree."""
+    # Assert that the switch succeeded and make sure the mode change is
+    # logged.
+    assert is_read_only(), "Switching to read-only failed."
+
+
+def remove_read_only_file(assert_mode_switch=True):
+    """Remove the file named read-only.txt from the root of the tree.
+
+    May also assert that the mode switch actually happened (i.e. not 
+    is_read_only()). This assertion has to be conditional because some tests
+    will use this during the processing of a request, when a mode change can't
+    happen (i.e. is_read_only() will still return True during that request's
+    processing, even though the read-only.txt file has been removed).
+    """
     os.remove(read_only_file_path)
-
+    if assert_mode_switch:
+        # Assert that the switch succeeded and make sure the mode change is
+        # logged.
+        assert not is_read_only(), "Switching to read-write failed."

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2010-01-19 04:22:17 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2010-01-27 11:28:15 +0000
@@ -23,6 +23,7 @@
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.interfaces.oauth import IOAuthConsumerSet
 from canonical.launchpad.ftests import ANONYMOUS, login
+from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.tests.readonly import (
     remove_read_only_file, touch_read_only_file)
 import canonical.launchpad.webapp.adapter as dbadapter
@@ -79,6 +80,11 @@
         # Cleanup needed so that further tests can start processing other
         # requests (e.g. calling beforeTraversal).
         self.publication.endRequest(self.request, None)
+        # Force pending mode switches to actually happen and get logged so
+        # that we don't interfere with other tests.
+        assert not is_read_only(), (
+            "A test failed to clean things up properly, leaving the app "
+            "in read-only mode.")
 
     def setUp(self):
         TestCase.setUp(self)
@@ -127,7 +133,10 @@
             touch_read_only_file()
             self.publication.beforeTraversal(self.request)
         finally:
-            remove_read_only_file()
+            # Tell remove_read_only_file() to not assert that the mode switch
+            # actually happened, as we know it won't happen until this request
+            # is finished.
+            remove_read_only_file(assert_mode_switch=False)
 
         # Here the mode has changed to read-only, so the stores were removed
         # from zstorm.