Revert of CQ to always post a message when Commit box is unchecked. (https://codereview.chromium.org/138173005/)
Reason for revert:
This created a lot of spam at commit event for some obscure reason. I couldn't fix it fast, so reverting.
Original issue's description:
> 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
>
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=247488
[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=238283
Review URL: https://codereview.chromium.org/138133014
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/commit-queue@248624 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/pending_manager.py b/pending_manager.py
index 6f61d4b..4ef64d6 100644
--- a/pending_manager.py
+++ b/pending_manager.py
@@ -227,8 +227,13 @@
# 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, 'CQ bit was unchecked on CL. Ignoring.')
+ self._discard_pending(pending, None)
# Find new issues.
for issue_id in new_issues:
@@ -298,10 +303,7 @@
pending.issue, pending.sort_key, missing, pending.get_state()))
self._verify_pending(pending)
except base.DiscardPending as e:
- message = e.status
- if not message:
- message = 'process_new_pending_commit: ' + self.FAILED_NO_MESSAGE
- self._discard_pending(e.pending, message)
+ self._discard_pending(e.pending, e.status)
def update_status(self):
"""Updates the status for each pending commit verifier."""
@@ -313,10 +315,7 @@
except base.DiscardPending as e:
# It's not efficient since it takes a full loop for each pending
# commit to discard.
- message = e.status
- if not message:
- message = 'update_status: ' + self.FAILED_NO_MESSAGE
- self._discard_pending(e.pending, message)
+ self._discard_pending(e.pending, e.status)
for pending in self.queue.iterate():
why_not = pending.why_not()
@@ -332,10 +331,8 @@
for pending in self.queue.iterate():
state = pending.get_state()
if state == base.FAILED:
- message = pending.error_message()
- if not message:
- message = 'scan_results(FAILED): ' + self.FAILED_NO_MESSAGE
- self._discard_pending(pending, message)
+ self._discard_pending(
+ pending, pending.error_message() or self.FAILED_NO_MESSAGE)
elif state == base.SUCCEEDED:
if self._throttle(pending):
continue
@@ -351,13 +348,9 @@
self._commit_patch(pending)
except base.DiscardPending as e:
- message = e.status
- if not message:
- message = 'scan_results(discard): ' + self.FAILED_NO_MESSAGE
- self._discard_pending(e.pending, message)
+ self._discard_pending(e.pending, e.status)
except Exception as e:
- message = 'scan_result(Exception): ' + self.INTERNAL_EXCEPTION
- self._discard_pending(pending, message)
+ self._discard_pending(pending, self.INTERNAL_EXCEPTION)
raise
else:
# When state is IGNORED, we need to keep this issue so it's not fetched
@@ -462,7 +455,6 @@
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:
@@ -471,23 +463,24 @@
except urllib2.HTTPError as e:
logging.error(
'Failed to set the flag to False for %s with message %s' % (
- pending.pending_name(), msg))
+ pending.pending_name(), message))
traceback.print_stack()
logging.error(str(e))
errors.send_stack(e)
- 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 }})
+ 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 }})
finally:
# Most importantly, remove the PendingCommit from the queue.
self.queue.remove(pending.issue)
@@ -566,10 +559,7 @@
out += '\n%s' % stdout
raise base.DiscardPending(pending, out)
except base.DiscardPending as e:
- message = e.status
- if not message:
- message = '_commit_patch: ' + self.FAILED_NO_MESSAGE
- self._discard_pending(e.pending, message)
+ self._discard_pending(e.pending, e.status)
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 12f73d4..0407eae 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, fake.failed_message())])
+ "add_comment(%d, %r)" % (issue, pc.FAILED_NO_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, fake.failed_message())])
+ "add_comment(%d, %r)" % (issue, pc.FAILED_NO_MESSAGE)])
else:
self.context.rietveld.check_calls(
[ "set_flag(%d, 1, 'commit', False)" % issue,
- "add_comment(%d, %r)" % (issue, fake.failed_message())])
+ "add_comment(%d, %r)" % (issue, pc.FAILED_NO_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, fake.failed_message()),
+ "add_comment(%d, %r)" % (issue, pc.FAILED_NO_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 testDisappeared(self):
+ def testDisapeared(self):
issue = 31337
verifiers = [
project_base.ProjectBaseUrlVerifier(
@@ -591,8 +591,6 @@
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 9b95854..c38009e 100644
--- a/verification/fake.py
+++ b/verification/fake.py
@@ -6,8 +6,6 @@
from verification import base
-def failed_message():
- return 'FakeVerifier FAILED'
class FakeVerifier(base.Verifier):
name = 'fake'
@@ -17,11 +15,7 @@
self.state = state
def verify(self, pending):
- 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
+ pending.verifications[self.name] = base.SimpleStatus(self.state)
def update_status(self, queue):
pass
@@ -45,6 +39,3 @@
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()
