Made _discard_pending() always post a message. Added meaningful
default messages to all the places where it is called in the code. In
particular, CQ will now always post a message whenever the Commit
checkbox is unchecked, for any reason.
Updated unit tests accordingly, and made them a bit more robust by
adding an error message to the failing FakeVerifier.
This does NOT fix the bug, but will hopefully give more visibility to
developers when a CL is dropped.
BUG=238283
Review URL: https://codereview.chromium.org/138173005
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/commit-queue@247488 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/pending_manager.py b/pending_manager.py
index b8b176b..c392a91 100644
--- a/pending_manager.py
+++ b/pending_manager.py
@@ -197,13 +197,8 @@
# are str due to json support.
if pending.issue not in new_issues:
logging.info('Flushing issue %d' % pending.issue)
- self.context.status.send(
- pending,
- { 'verification': 'abort',
- 'payload': {
- 'output': 'CQ bit was unchecked on CL. Ignoring.' }})
pending.get_state = lambda: base.IGNORED
- self._discard_pending(pending, None)
+ self._discard_pending(pending, 'CQ bit was unchecked on CL. Ignoring.')
# Find new issues.
for issue_id in new_issues:
@@ -271,7 +266,10 @@
pending.issue, missing, pending.get_state()))
self._verify_pending(pending)
except base.DiscardPending as e:
- self._discard_pending(e.pending, e.status)
+ message = e.status
+ if not message:
+ message = 'process_new_pending_commit: ' + self.FAILED_NO_MESSAGE
+ self._discard_pending(e.pending, message)
def update_status(self):
"""Updates the status for each pending commit verifier."""
@@ -283,7 +281,10 @@
except base.DiscardPending as e:
# It's not efficient since it takes a full loop for each pending
# commit to discard.
- self._discard_pending(e.pending, e.status)
+ message = e.status
+ if not message:
+ message = 'update_status: ' + self.FAILED_NO_MESSAGE
+ self._discard_pending(e.pending, message)
for pending in self.queue.iterate():
why_not = pending.why_not()
@@ -299,8 +300,10 @@
for pending in self.queue.iterate():
state = pending.get_state()
if state == base.FAILED:
- self._discard_pending(
- pending, pending.error_message() or self.FAILED_NO_MESSAGE)
+ message = pending.error_message()
+ if not message:
+ message = 'scan_results(FAILED): ' + self.FAILED_NO_MESSAGE
+ self._discard_pending(pending, message)
elif state == base.SUCCEEDED:
if self._throttle(pending):
continue
@@ -316,9 +319,13 @@
self._commit_patch(pending)
except base.DiscardPending as e:
- self._discard_pending(e.pending, e.status)
+ message = e.status
+ if not message:
+ message = 'scan_results(discard): ' + self.FAILED_NO_MESSAGE
+ self._discard_pending(e.pending, message)
except Exception as e:
- self._discard_pending(pending, self.INTERNAL_EXCEPTION)
+ message = 'scan_result(Exception): ' + self.INTERNAL_EXCEPTION
+ self._discard_pending(pending, message)
raise
else:
# When state is IGNORED, we need to keep this issue so it's not fetched
@@ -423,6 +430,7 @@
def _discard_pending(self, pending, message):
"""Discards a pending commit. Attach an optional message to the review."""
logging.debug('_discard_pending(%s, %s)', pending.issue, message)
+ msg = message or self.FAILED_NO_MESSAGE
try:
try:
if pending.get_state() != base.IGNORED:
@@ -431,24 +439,23 @@
except urllib2.HTTPError as e:
logging.error(
'Failed to set the flag to False for %s with message %s' % (
- pending.pending_name(), message))
+ pending.pending_name(), msg))
traceback.print_stack()
logging.error(str(e))
errors.send_stack(e)
- if message:
- try:
- self.context.rietveld.add_comment(pending.issue, message)
- except urllib2.HTTPError as e:
- logging.error(
- 'Failed to add comment for %s with message %s' % (
- pending.pending_name(), message))
- traceback.print_stack()
- errors.send_stack(e)
- self.context.status.send(
- pending,
- { 'verification': 'abort',
- 'payload': {
- 'output': message }})
+ try:
+ self.context.rietveld.add_comment(pending.issue, msg)
+ except urllib2.HTTPError as e:
+ logging.error(
+ 'Failed to add comment for %s with message %s' % (
+ pending.pending_name(), msg))
+ traceback.print_stack()
+ errors.send_stack(e)
+ self.context.status.send(
+ pending,
+ { 'verification': 'abort',
+ 'payload': {
+ 'output': msg }})
finally:
# Most importantly, remove the PendingCommit from the queue.
self.queue.remove(pending.issue)
@@ -527,7 +534,10 @@
out += '\n%s' % stdout
raise base.DiscardPending(pending, out)
except base.DiscardPending as e:
- self._discard_pending(e.pending, e.status)
+ message = e.status
+ if not message:
+ message = '_commit_patch: ' + self.FAILED_NO_MESSAGE
+ self._discard_pending(e.pending, message)
def _throttle(self, pending):
"""Returns True if a commit should be delayed."""
diff --git a/tests/pending_manager_test.py b/tests/pending_manager_test.py
index 0407eae..12f73d4 100755
--- a/tests/pending_manager_test.py
+++ b/tests/pending_manager_test.py
@@ -114,7 +114,7 @@
self.context.rietveld.check_calls(
[ _try_comment(),
"set_flag(%d, 1, 'commit', False)" % issue,
- "add_comment(%d, %r)" % (issue, pc.FAILED_NO_MESSAGE)])
+ "add_comment(%d, %r)" % (issue, fake.failed_message())])
else:
if success:
self.context.checkout.check_calls(
@@ -131,11 +131,11 @@
self.context.rietveld.check_calls(
[ _try_comment(),
"set_flag(%d, 1, 'commit', False)" % issue,
- "add_comment(%d, %r)" % (issue, pc.FAILED_NO_MESSAGE)])
+ "add_comment(%d, %r)" % (issue, fake.failed_message())])
else:
self.context.rietveld.check_calls(
[ "set_flag(%d, 1, 'commit', False)" % issue,
- "add_comment(%d, %r)" % (issue, pc.FAILED_NO_MESSAGE)])
+ "add_comment(%d, %r)" % (issue, fake.failed_message())])
def _prepare_apply_commit(self, index, issue, server_hooks_missing=False):
"""Returns a frequent sequence of action happening on the Checkout object.
@@ -265,7 +265,7 @@
])
self.context.rietveld.check_calls(
[ _try_comment(),
- "add_comment(%d, %r)" % (issue, pc.FAILED_NO_MESSAGE),
+ "add_comment(%d, %r)" % (issue, fake.failed_message()),
])
self.context.status.check_names(['initial', 'abort'])
@@ -571,7 +571,7 @@
self.context.checkout.check_calls(
self._prepare_apply_commit(0, issue, server_hooks_missing=True))
- def testDisapeared(self):
+ def testDisappeared(self):
issue = 31337
verifiers = [
project_base.ProjectBaseUrlVerifier(
@@ -591,6 +591,8 @@
pc.scan_results()
self.assertEqual(0, len(pc.queue.iterate()))
self.context.status.check_names(['abort'])
+ self.context.rietveld.check_calls(
+ ["add_comment(%d, 'CQ bit was unchecked on CL. Ignoring.')" % issue])
def _get_pc_reviewer(self):
verifiers = [
diff --git a/verification/fake.py b/verification/fake.py
index c38009e..9b95854 100644
--- a/verification/fake.py
+++ b/verification/fake.py
@@ -6,6 +6,8 @@
from verification import base
+def failed_message():
+ return 'FakeVerifier FAILED'
class FakeVerifier(base.Verifier):
name = 'fake'
@@ -15,7 +17,11 @@
self.state = state
def verify(self, pending):
- pending.verifications[self.name] = base.SimpleStatus(self.state)
+ fake = base.SimpleStatus(self.state)
+ # Make sure to leave a message, so CQ tests can reliably test for it.
+ if self.state == base.FAILED:
+ fake.error_message = failed_message()
+ pending.verifications[self.name] = fake
def update_status(self, queue):
pass
@@ -39,3 +45,6 @@
for _, fake in self.loop(queue, base.SimpleStatus, True):
fake.state = self.state
+ # Make sure to leave a message, so CQ tests can reliably test for it.
+ if self.state == base.FAILED:
+ fake.error_message = failed_message()
