Give better error messages when changes aren't mergeable.
Instead of reporting that a change is "not commit ready
anymore", we should give a specific error message explaining what the
exact problem is.
BUG=chromium:336632
TEST=Updated unit tests.
Change-Id: Ibd852afab6d2603dd9a068b99145d88d1af92068
Reviewed-on: https://chromium-review.googlesource.com/235790
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Tested-by: David James <davidjames@chromium.org>
Trybot-Ready: David James <davidjames@chromium.org>
Reviewed-by: Aviv Keshet <akeshet@chromium.org>
Commit-Queue: David James <davidjames@chromium.org>
diff --git a/cbuildbot/validation_pool.py b/cbuildbot/validation_pool.py
index 17dbae8..5949941 100644
--- a/cbuildbot/validation_pool.py
+++ b/cbuildbot/validation_pool.py
@@ -114,13 +114,6 @@
"""Raised if we try to apply a not-in-manifest change."""
-class PatchNotCommitReady(cros_patch.PatchException):
- """Raised if a patch is not marked as commit ready."""
-
- def ShortExplanation(self):
- return 'isn\'t marked as Commit-Ready anymore.'
-
-
class PatchModified(cros_patch.PatchException):
"""Raised if a patch is modified while the CQ is running."""
@@ -483,7 +476,7 @@
if self._is_submitting:
raise PatchRejected(dep_change)
else:
- raise PatchNotCommitReady(dep_change)
+ raise dep_change.GetMergeException() or PatchRejected(dep_change)
unsatisfied.append(dep_change)
@@ -1670,13 +1663,7 @@
raise TreeIsClosedException(closed_or_throttled=not throttled_ok)
# Filter out changes that were modified during the CQ run.
- unmodified_changes, errors = self.FilterModifiedChanges(changes)
-
- # Filter out changes that aren't marked as CR=+2, CQ=+1, V=+1 anymore, in
- # case the patch status changed during the CQ run.
- filtered_changes = [x for x in unmodified_changes if x.IsMergeable()]
- for change in set(unmodified_changes) - set(filtered_changes):
- errors[change] = PatchNotCommitReady(change)
+ filtered_changes, errors = self.FilterModifiedChanges(changes)
patch_series = PatchSeries(self.build_root, helper_pool=self._helper_pool,
is_submitting=True)
@@ -1739,8 +1726,8 @@
Returns:
This returns a tuple (unmodified_changes, errors).
- unmodified_changes: A reloaded list of changes, only including unmodified
- and unsubmitted changes.
+ unmodified_changes: A reloaded list of changes, only including mergeable,
+ unmodified and unsubmitted changes.
errors: A dictionary. This dictionary will contain all patches that have
encountered errors, and map them to the associated exception object.
"""
@@ -1749,15 +1736,18 @@
# that occurs below will be mostly up-to-date.
unmodified_changes, errors = [], {}
reloaded_changes = list(cls.ReloadChanges(changes))
- old_changes = cros_patch.PatchCache(changes)
- for change in reloaded_changes:
- if change.IsAlreadyMerged():
+ for change, reloaded_change in zip(changes, reloaded_changes):
+ if reloaded_change.IsAlreadyMerged():
logging.warning('%s is already merged. It was most likely chumped '
'during the current CQ run.', change)
- elif change.patch_number != old_changes[change].patch_number:
+ elif reloaded_change.patch_number != change.patch_number:
# If users upload new versions of a CL while the CQ is in-flight, then
# their CLs are no longer tested. These CLs should be rejected.
errors[change] = PatchModified(change)
+ elif not reloaded_change.IsMergeable():
+ # Get the reason why this change is not mergeable anymore.
+ errors[change] = reloaded_change.GetMergeException()
+ errors[change].patch = change
else:
unmodified_changes.append(change)
diff --git a/cbuildbot/validation_pool_unittest.py b/cbuildbot/validation_pool_unittest.py
index 4e8fbc1..87848b0 100755
--- a/cbuildbot/validation_pool_unittest.py
+++ b/cbuildbot/validation_pool_unittest.py
@@ -1450,13 +1450,17 @@
"""Test that a CL is rejected if its approvals were pulled."""
def _ReloadPatches(patches):
reloaded = copy.deepcopy(patches)
- self.PatchObject(reloaded[1], 'HasApproval', return_value=False)
+ approvals = {('VRIF', '1'): False}
+ backup = reloaded[1].HasApproval
+ self.PatchObject(
+ reloaded[1], 'HasApproval',
+ side_effect=lambda *args: approvals.get(args, backup(*args)))
return reloaded
self.PatchObject(gerrit, 'GetGerritPatchInfoWithPatchQueries',
_ReloadPatches)
self.SubmitPool(submitted=self.patches[:1], rejected=self.patches[1:])
- error = validation_pool.PatchNotCommitReady(self.patches[1])
- self.assertEqualNotifyArg(error, self.patches[1], 'error')
+ message = 'CL:2 is not marked Verified=+1.'
+ self.assertEqualNotifyArg(message, self.patches[1], 'error')
def testAlreadyMerged(self):
"""Test that a CL that was chumped during the run was not rejected."""
diff --git a/lib/patch.py b/lib/patch.py
index bead1bd..fb2ae7c 100644
--- a/lib/patch.py
+++ b/lib/patch.py
@@ -294,6 +294,18 @@
return 'could not be found in the repo manifest.'
+class PatchNotMergeable(PatchException):
+ """Raised if a patch is not mergeable."""
+
+ def __init__(self, patch, reason):
+ PatchException.__init__(self, patch)
+ self.reason = str(reason)
+ self.args = (patch, reason)
+
+ def ShortExplanation(self):
+ return self.reason
+
+
def MakeChangeId(unusable=False):
"""Create a random Change-Id.
@@ -1534,14 +1546,40 @@
def IsMergeable(self):
"""Return true if all Gerrit approvals required for submission are set."""
- return (self.status == 'NEW' and not self.WasVetoed() and
- not self.IsDraft() and
- self.HasApprovals({'CRVW': '2', 'VRIF': '1', 'COMR': ('1', '2')}))
+ return not self.GetMergeException()
def HasReadyFlag(self):
"""Return true if the trybot-ready or commit-ready flag is set."""
return self.HasApproval('COMR', ('1', '2')) or self.HasApproval('TRY', '1')
+ def GetMergeException(self):
+ """Return the reason why this change is not mergeable.
+
+ If the change is in fact mergeable, return None.
+ """
+ if self.IsDraft():
+ return PatchNotMergeable(self, 'is a draft.')
+
+ if self.status != 'NEW':
+ statuses = {
+ 'MERGED': 'is already merged.',
+ 'SUBMITTED': 'is being merged.',
+ 'ABANDONED': 'is abandoned.',
+ }
+ message = statuses.get(self.status, 'has status %s.' % self.status)
+ return PatchNotMergeable(self, message)
+
+ if self.HasApproval('VRIF', '-1'):
+ return PatchNotMergeable(self, 'is marked as Verified=-1.')
+ elif self.HasApproval('CRVW', '-2'):
+ return PatchNotMergeable(self, 'is marked as Code-Review=-2.')
+ elif not self.HasApproval('CRVW', '2'):
+ return PatchNotMergeable(self, 'is not marked Code-Review=+2.')
+ elif not self.HasApproval('VRIF', '1'):
+ return PatchNotMergeable(self, 'is not marked Verified=+1.')
+ elif not self.HasApproval('COMR', ('1', '2')):
+ return PatchNotMergeable(self, 'is not marked Commit-Queue>=+1.')
+
def GetLatestApproval(self, field):
"""Return most recent value of specific field on the current patchset.
