Reduce spam from CQ.

My previous change https://codereview.chromium.org/138173005/ made CQ
post a message when the commit box was unchecked.  It shouldn't have
posted it when CQ committed a change, but for some obscure reason it
did.  Sometimes twice...

This patch unifies the way an issue is removed from the queue, and in
particular, should eliminate most of the unwanted messages.  Some CQ
messages about unchecked CQ box will still be posted when CQ drops it,
and it is expected to be rare.

BUG=339116
R=phajdan.jr@chromium.org

Review URL: https://codereview.chromium.org/130983010

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/commit-queue@248553 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/pending_manager.py b/pending_manager.py
index 6f61d4b..def87d8 100644
--- a/pending_manager.py
+++ b/pending_manager.py
@@ -544,18 +544,11 @@
           self.context.rietveld.close_issue(pending.issue)
           self.context.rietveld.update_description(
               pending.issue, issue_desc.description)
-          self.context.rietveld.add_comment(
-              pending.issue, 'Change committed as %s' % pending.revision)
         except (urllib2.HTTPError, urllib2.URLError) as e:
           # Ignore AppEngine flakiness.
           logging.warning('Unable to fully close the issue')
-        # And finally remove the issue. If the close_issue() call above failed,
-        # it is possible the dashboard will be confused but it is harmless.
-        try:
-          self.queue.get(pending.issue)
-        except KeyError:
-          logging.error('Internal inconsistency for %d', pending.issue)
-        self.queue.remove(pending.issue)
+        self._discard_pending(pending,
+                              'Change committed as %s' % pending.revision)
       except (
           checkout.PatchApplicationFailed, patch.UnsupportedPatchFormat) as e:
         raise base.DiscardPending(pending, str(e))
diff --git a/tests/pending_manager_test.py b/tests/pending_manager_test.py
index 12f73d4..74cbf2a 100755
--- a/tests/pending_manager_test.py
+++ b/tests/pending_manager_test.py
@@ -106,6 +106,7 @@
             [ _try_comment(),
               'close_issue(%d)' % issue,
               "update_description(%d, u'foo')" % issue,
+              "set_flag(%d, 1, 'commit', False)" % issue,
               "add_comment(%d, 'Change committed as 125')" % issue])
       else:
         self.context.checkout.check_calls(
@@ -123,6 +124,7 @@
             [ _try_comment(),
               'close_issue(%d)' % issue,
               "update_description(%d, u'foo')" % issue,
+              "set_flag(%d, 1, 'commit', False)" % issue,
               "add_comment(%d, 'Change committed as 125')" % issue])
       else:
         # checkout is never touched in that case.
@@ -210,7 +212,8 @@
     self._check_standard_verification(pc, result == base.SUCCEEDED, False)
 
     if result == base.SUCCEEDED:
-      self.context.status.check_names(['initial', 'why not', 'commit'])
+      self.context.status.check_names(
+        ['initial', 'why not', 'commit', 'abort'])
     elif send_initial_packet:
       self.context.status.check_names(['initial', 'abort'])
     else:
@@ -289,7 +292,7 @@
     self._check_standard_verification(pc, result == base.SUCCEEDED, True)
     if result == base.SUCCEEDED:
       self.context.status.check_names(['initial', 'why not', 'why not',
-                                       'why not', 'commit'])
+                                       'why not', 'commit', 'abort'])
     else:
       self.context.status.check_names(['initial', 'why not', 'why not',
                                        'abort'])
@@ -356,7 +359,7 @@
     self._check_standard_verification(
         pc, all(x == base.SUCCEEDED for x in (f1, f2, f3, f4)), False)
     if all(x == base.SUCCEEDED for x in (f1, f2, f3, f4)):
-      self.context.status.check_names(['initial', 'why not', 'commit'])
+      self.context.status.check_names(['initial', 'why not', 'commit', 'abort'])
     else:
       self.context.status.check_names(['initial', 'abort'])
 
@@ -407,7 +410,7 @@
         pc, all(x == base.SUCCEEDED for x in (f1, f2, f3, f4)), False)
     if all(x == base.SUCCEEDED for x in (f1, f2, f3, f4)):
       self.context.status.check_names(['initial', 'why not', 'why not',
-                                       'why not', 'commit'])
+                                       'why not', 'commit', 'abort'])
     else:
       self.context.status.check_names(['initial', 'why not', 'why not',
                                        'abort'])
@@ -449,8 +452,9 @@
         [ _try_comment(),
           'close_issue(%d)' % issue,
           "update_description(%d, u'foo')" % issue,
+          "set_flag(%d, 1, 'commit', False)" % issue,
           "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['initial', 'why not', 'commit'])
+    self.context.status.check_names(['initial', 'why not', 'commit', 'abort'])
 
   def testCommitBurst(self):
     issue = 31337
@@ -482,21 +486,25 @@
           _try_comment(),
           'close_issue(0)',
           "update_description(0, u'foo')",
+          "set_flag(0, 1, 'commit', False)",
           "add_comment(0, 'Change committed as 125')",
           'close_issue(1)',
           "update_description(1, u'foo')",
+          "set_flag(1, 1, 'commit', False)",
           "add_comment(1, 'Change committed as 125')",
           'close_issue(2)',
           "update_description(2, u'foo')",
+          "set_flag(2, 1, 'commit', False)",
           "add_comment(2, 'Change committed as 125')",
           'close_issue(3)',
           "update_description(3, u'foo')",
+          "set_flag(3, 1, 'commit', False)",
           "add_comment(3, 'Change committed as 125')",
         ])
     self.assertEqual(3, len(pc.queue.iterate()))
     total = pc.MAX_COMMIT_BURST + 3
     self.context.status.check_names(['initial'] * total +
-                                    (['why not', 'commit'] *
+                                    (['why not', 'commit', 'abort'] *
                                      pc.MAX_COMMIT_BURST) +
                                     ['why not'] * 3)
 
@@ -515,9 +523,11 @@
     self.context.rietveld.check_calls(
         [ 'close_issue(%d)' % next_item,
           "update_description(%d, u'foo')" % next_item,
+          "set_flag(%d, 1, 'commit', False)" % next_item,
           "add_comment(%d, 'Change committed as 125')" % next_item,
         ])
-    self.context.status.check_names(['why not', 'commit'] + ['why not'] * 2)
+    self.context.status.check_names(['why not', 'commit', 'abort'] +
+                                    ['why not'] * 2)
     # After a delay, must flush the queue.
     timestamp.append(timestamp[-1] + pc.COMMIT_BURST_DELAY + 1)
     pc.scan_results()
@@ -527,11 +537,13 @@
     self.context.rietveld.check_calls(
         [ 'close_issue(%d)' % (next_item + 1),
           "update_description(%d, u'foo')" % (next_item + 1),
+          "set_flag(%d, 1, 'commit', False)" % (next_item + 1),
           "add_comment(%d, 'Change committed as 125')" % (next_item + 1),
           'close_issue(%d)' % issue,
           "update_description(%d, u'foo')" % issue,
+          "set_flag(%d, 1, 'commit', False)" % issue,
           "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['why not', 'commit'] * 2)
+    self.context.status.check_names(['why not', 'commit', 'abort'] * 2)
 
   def testIgnored(self):
     issue = 31337
@@ -566,8 +578,9 @@
         [ _try_comment(),
           'close_issue(%d)' % issue,
           "update_description(%d, u'foo')" % issue,
+          "set_flag(%d, 1, 'commit', False)" % issue,
           "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['initial', 'why not', 'commit'])
+    self.context.status.check_names(['initial', 'why not', 'commit', 'abort'])
     self.context.checkout.check_calls(
         self._prepare_apply_commit(0, issue, server_hooks_missing=True))
 
@@ -648,8 +661,9 @@
         [ _try_comment(),
           'close_issue(%d)' % issue,
           "update_description(%d, u'foo')" % issue,
+          "set_flag(%d, 1, 'commit', False)" % issue,
           "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['initial', 'why not', 'commit'])
+    self.context.status.check_names(['initial', 'why not', 'commit', 'abort'])
     self.context.checkout.check_calls(
         self._prepare_apply_commit(0, issue))
 
@@ -688,8 +702,9 @@
         [ _try_comment(),
           'close_issue(%d)' % issue,
           "update_description(%d, u'foo')" % issue,
+          "set_flag(%d, 1, 'commit', False)" % issue,
           "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['initial', 'why not', 'commit'])
+    self.context.status.check_names(['initial', 'why not', 'commit', 'abort'])
     self.context.checkout.check_calls(
         self._prepare_apply_commit(0, issue))
 
@@ -720,8 +735,9 @@
       [_try_comment(),
        'close_issue(%d)' % issue,
        "update_description(%d, u'foo')" % issue,
+       "set_flag(%d, 1, 'commit', False)" % issue,
        "add_comment(%d, 'Change committed as 125')" % issue])
-    self.context.status.check_names(['why not', 'commit'])
+    self.context.status.check_names(['why not', 'commit', 'abort'])
     self.context.checkout.check_calls(
         self._prepare_apply_commit(0, issue))
 
diff --git a/tests/project_test.py b/tests/project_test.py
index a559781..f885fb1 100755
--- a/tests/project_test.py
+++ b/tests/project_test.py
@@ -435,7 +435,7 @@
                              why_not, rietveld_calls)
 
   def test_tbr(self):
-    self.time = map(lambda x: float(x*35), range(15))
+    self.time = map(lambda x: float(x*35), range(16))
     self.urlrequests = [
       ('https://chromium-status.appspot.com/allstatus?format=json&endTime=85',
         # Cheap hack here.
@@ -486,6 +486,7 @@
           "{u'chromium_presubmit': ['presubmit']})",
       'close_issue(31337)',
       "update_description(31337, u'foo\\nTBR=')",
+      "set_flag(31337, 1, 'commit', False)",
       "add_comment(31337, 'Change committed as 125')",
       ])
     self.context.checkout.check_calls([
@@ -497,7 +498,7 @@
           "u'user@example.com')",
       ])
     self.context.status.check_names(['initial', 'why not', 'why not',
-                                     'why not', 'commit'])
+                                     'why not', 'commit', 'abort'])