Code review comment for lp:~salgado/launchpad/fix-readonly-test-helpers

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>