launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/stop-mirroring-hosted into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This is the final branch in the stack that enforces not calling the branch mirror methods on hosted branches.

Exceptions are now raised if requestMirror, startMirror, or mirrorFailed are called on hosted branches.

Several tests that were doing this have been fixed.


-- 
https://code.launchpad.net/~thumper/launchpad/stop-mirroring-hosted/+merge/31116
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/stop-mirroring-hosted into lp:launchpad/devel.

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2010-07-09 10:11:47 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2010-07-28 04:28:50 +0000
@@ -125,7 +125,7 @@
 
     def testMirrorStatusMessageIsTruncated(self):
         """mirror_status_message is truncated if the text is overly long."""
-        branch = self.factory.makeBranch()
+        branch = self.factory.makeBranch(branch_type=BranchType.MIRRORED)
         branch.mirrorFailed(
             "on quick brown fox the dog jumps to" *
             BranchMirrorStatusView.MAXIMUM_STATUS_MESSAGE_LENGTH)
@@ -137,7 +137,7 @@
 
     def testMirrorStatusMessage(self):
         """mirror_status_message on the view is the same as on the branch."""
-        branch = self.factory.makeBranch()
+        branch = self.factory.makeBranch(branch_type=BranchType.MIRRORED)
         branch.mirrorFailed("This is a short error message.")
         branch_view = BranchMirrorStatusView(branch, self.request)
         self.assertTrue(

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-07-25 12:55:56 +0000
+++ lib/lp/code/model/branch.py	2010-07-28 04:28:50 +0000
@@ -848,10 +848,11 @@
         mirrored.
         """
         new_data_pushed = (
-             self.branch_type in (BranchType.HOSTED, BranchType.IMPORTED)
+             self.branch_type == BranchType.IMPORTED
              and self.next_mirror_time is not None)
         # XXX 2010-04-22, MichaelHudson: This should really look for a branch
-        # scan job.
+        # scan job. Yes because if the scan job fails, the branch gets stuck
+        # in updating.
         pulled_but_not_scanned = self.last_mirrored_id != self.last_scanned_id
         pull_in_progress = (
             self.last_mirror_attempt is not None
@@ -896,7 +897,7 @@
 
     def requestMirror(self):
         """See `IBranch`."""
-        if self.branch_type == BranchType.REMOTE:
+        if self.branch_type in (BranchType.REMOTE, BranchType.HOSTED):
             raise BranchTypeError(self.unique_name)
         branch = Store.of(self).find(
             Branch,
@@ -909,7 +910,7 @@
 
     def startMirroring(self):
         """See `IBranch`."""
-        if self.branch_type == BranchType.REMOTE:
+        if self.branch_type in (BranchType.REMOTE, BranchType.HOSTED):
             raise BranchTypeError(self.unique_name)
         self.last_mirror_attempt = UTC_NOW
         self.next_mirror_time = None
@@ -948,7 +949,7 @@
 
     def mirrorFailed(self, reason):
         """See `IBranch`."""
-        if self.branch_type == BranchType.REMOTE:
+        if self.branch_type in (BranchType.REMOTE, BranchType.HOSTED):
             raise BranchTypeError(self.unique_name)
         self.mirror_failures += 1
         self.mirror_status_message = reason

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-07-25 12:55:56 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-07-28 04:28:50 +0000
@@ -1954,11 +1954,11 @@
         branch = self.factory.makeAnyBranch()
         self.assertEqual(False, branch.pending_writes)
 
-    def test_requestMirror_for_hosted(self):
-        # If a hosted branch has a requested mirror, then someone has just
-        # pushed something up. Therefore, pending writes.
+    def test_branchChanged_for_hosted(self):
+        # If branchChanged has been called with a new tip revision id, there
+        # are pending writes.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.HOSTED)
-        branch.requestMirror()
+        branch.branchChanged('', 'new-tip', None, None, None)
         self.assertEqual(True, branch.pending_writes)
 
     def test_requestMirror_for_imported(self):
@@ -1980,7 +1980,7 @@
         # If a branch has been pulled (mirrored) but not scanned, then we have
         # yet to load the revisions into the database. This means there are
         # pending writes.
-        branch = self.factory.makeAnyBranch()
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(
@@ -1990,7 +1990,7 @@
     def test_pulled_and_scanned(self):
         # If a branch has been pulled and scanned, then there are no pending
         # writes.
-        branch = self.factory.makeAnyBranch()
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(
@@ -2004,14 +2004,14 @@
     def test_first_mirror_started(self):
         # If we have started mirroring the branch for the first time, then
         # there are probably pending writes.
-        branch = self.factory.makeAnyBranch()
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         self.assertEqual(True, branch.pending_writes)
 
     def test_following_mirror_started(self):
         # If we have started mirroring the branch, then there are probably
         # pending writes.
-        branch = self.factory.makeAnyBranch()
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
         removeSecurityProxy(branch).branchChanged(

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2010-04-30 04:51:04 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2010-07-28 04:28:50 +0000
@@ -123,7 +123,6 @@
     def test_getOopsMailController(self):
         """The registrant is notified about merge proposal creation issues."""
         bmp = self.factory.makeBranchMergeProposal()
-        bmp.source_branch.requestMirror()
         job = MergeProposalCreatedJob.create(bmp)
         ctrl = job.getOopsMailController('1234')
         self.assertEqual([bmp.registrant.preferredemail.email], ctrl.to_addrs)

=== modified file 'lib/lp/code/model/tests/test_branchtarget.py'
--- lib/lp/code/model/tests/test_branchtarget.py	2010-04-23 08:26:27 +0000
+++ lib/lp/code/model/tests/test_branchtarget.py	2010-07-28 04:28:50 +0000
@@ -107,7 +107,6 @@
         development_package = self.original.development_version
         default_branch = self.factory.makePackageBranch(
             sourcepackage=development_package)
-        default_branch.startMirroring()
         removeSecurityProxy(default_branch).branchChanged(
             '', self.factory.getUniqueString(), None, None, None)
         ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
@@ -347,7 +346,6 @@
         # default stacked-on branch.
         branch = self.factory.makeProductBranch(product=self.original)
         self._setDevelopmentFocus(self.original, branch)
-        branch.startMirroring()
         removeSecurityProxy(branch).branchChanged(
             '', 'rev1', None, None, None)
         target = IBranchTarget(self.original)
@@ -468,17 +466,13 @@
         # the current user, even if those branches have already been mirrored.
         branch = self.factory.makeAnyBranch(private=True)
         naked_branch = removeSecurityProxy(branch)
-        naked_branch.startMirroring()
         naked_branch.branchChanged(
             '', self.factory.getUniqueString(), None, None, None)
         self.assertIs(None, check_default_stacked_on(branch))
 
     def test_been_mirrored(self):
-        # `check_default_stacked_on` returns None if passed a remote branch.
-        # We have no Bazaar data for remote branches, so stacking on one is
-        # futile.
+        # `check_default_stacked_on` returns the branch if it has revisions.
         branch = self.factory.makeAnyBranch()
-        branch.startMirroring()
         removeSecurityProxy(branch).branchChanged(
             '', self.factory.getUniqueString(), None, None, None)
         self.assertEqual(branch, check_default_stacked_on(branch))

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-21 22:44:02 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-07-28 04:28:50 +0000
@@ -410,17 +410,11 @@
         message = "No such source package: 'ningnangnong'."
         self.assertEqual(faults.NotFound(message), fault)
 
-    def test_initialMirrorRequest(self):
-        # The default 'next_mirror_time' for a newly created hosted branch
-        # should be None.
-        branch = self.factory.makeAnyBranch(branch_type=BranchType.HOSTED)
-        self.assertIs(None, branch.next_mirror_time)
-
     def test_requestMirror(self):
         # requestMirror should set the next_mirror_time field to be the
         # current time.
         requester = self.factory.makePerson()
-        branch = self.factory.makeAnyBranch(branch_type=BranchType.HOSTED)
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         self.codehosting_api.requestMirror(requester.id, branch.id)
         self.assertSqlAttributeEqualsDate(
             branch, 'next_mirror_time', UTC_NOW)
@@ -428,7 +422,8 @@
     def test_requestMirror_private(self):
         # requestMirror can be used to request the mirror of a private branch.
         requester = self.factory.makePerson()
-        branch = self.factory.makeAnyBranch(owner=requester, private=True)
+        branch = self.factory.makeAnyBranch(
+            owner=requester, private=True, branch_type=BranchType.MIRRORED)
         branch = removeSecurityProxy(branch)
         self.codehosting_api.requestMirror(requester.id, branch.id)
         self.assertSqlAttributeEqualsDate(