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
+            # 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.



-- 
Guilherme Salgado <email address hidden>