launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/fix-librarian-spuriousity into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #706992 intermittent test failures: test_404 test_oldurl
  https://bugs.launchpad.net/bugs/706992

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/fix-librarian-spuriousity/+merge/51068

So it turns out that str.replace()ing integers in a URL with a random port is not such a good idea.
-- 
https://code.launchpad.net/~wgrant/launchpad/fix-librarian-spuriousity/+merge/51068
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/fix-librarian-spuriousity into lp:launchpad.

=== modified file 'lib/canonical/librarian/ftests/test_web.py'
--- lib/canonical/librarian/ftests/test_web.py	2011-01-19 00:32:50 +0000
+++ lib/canonical/librarian/ftests/test_web.py	2011-02-24 04:43:17 +0000
@@ -11,6 +11,7 @@
     )
 from urlparse import urlparse
 
+from lazr.uri import URI
 import pytz
 from storm.expr import SQL
 import transaction
@@ -39,6 +40,12 @@
     )
 
 
+def uri_path_replace(url, old, new):
+    """Replace a substring of a URL's path."""
+    parsed = URI(url)
+    return str(parsed.replace(path=parsed.path.replace(old, new)))
+
+
 class LibrarianWebTestCase(unittest.TestCase):
     """Test the librarian's web interface."""
     layer = LaunchpadFunctionalLayer
@@ -149,11 +156,11 @@
         url = client.getURLForAlias(aid)
         self.assertEqual(urlopen(url).read(), 'sample')
 
-        old_url = url.replace(str(aid), '42/%d' % aid)
+        old_url = uri_path_replace(url, str(aid), '42/%d' % aid)
         self.assertEqual(urlopen(old_url).read(), 'sample')
 
         # If the content id is not an integer, a 404 is raised
-        old_url = url.replace(str(aid), 'foo/%d' % aid)
+        old_url = uri_path_replace(url, str(aid), 'foo/%d' % aid)
         self.require404(old_url)
 
     def test_404(self):
@@ -166,11 +173,13 @@
 
         # Change the aliasid and assert we get a 404
         self.failUnless(str(aid) in url)
-        self.require404(url.replace(str(aid), str(aid+1)))
+        bad_id_url = uri_path_replace(url, str(aid), str(aid+1))
+        self.require404(bad_id_url)
 
         # Change the filename and assert we get a 404
-        self.failUnless(str(filename) in url)
-        self.require404(url.replace(filename, 'different.txt'))
+        self.failUnless(filename in url)
+        bad_name_url = uri_path_replace(url, filename, 'different.txt')
+        self.require404(bad_name_url)
 
     def test_duplicateuploads(self):
         client = LibrarianClient()