Revert of Reduce spam from CQ. (https://codereview.chromium.org/130983010/)
Reason for revert:
This did not fix the noise from the CQ. Reverting this and the previous patches.
Original issue's description:
> 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
> [email protected]
>
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=248553
[email protected],[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=339116
Review URL: https://codereview.chromium.org/136643007
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/commit-queue@248623 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/pending_manager.py b/pending_manager.py
index def87d8..6f61d4b 100644
--- a/pending_manager.py
+++ b/pending_manager.py
@@ -544,11 +544,18 @@
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')
- self._discard_pending(pending,
- 'Change committed as %s' % pending.revision)
+ # 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)
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 74cbf2a..12f73d4 100755
--- a/tests/pending_manager_test.py
+++ b/tests/pending_manager_test.py
@@ -106,7 +106,6 @@
[ _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(
@@ -124,7 +123,6 @@
[ _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.
@@ -212,8 +210,7 @@
self._check_standard_verification(pc, result == base.SUCCEEDED, False)
if result == base.SUCCEEDED:
- self.context.status.check_names(
- ['initial', 'why not', 'commit', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
elif send_initial_packet:
self.context.status.check_names(['initial', 'abort'])
else:
@@ -292,7 +289,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', 'abort'])
+ 'why not', 'commit'])
else:
self.context.status.check_names(['initial', 'why not', 'why not',
'abort'])
@@ -359,7 +356,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', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
else:
self.context.status.check_names(['initial', 'abort'])
@@ -410,7 +407,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', 'abort'])
+ 'why not', 'commit'])
else:
self.context.status.check_names(['initial', 'why not', 'why not',
'abort'])
@@ -452,9 +449,8 @@
[ _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', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
def testCommitBurst(self):
issue = 31337
@@ -486,25 +482,21 @@
_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', 'abort'] *
+ (['why not', 'commit'] *
pc.MAX_COMMIT_BURST) +
['why not'] * 3)
@@ -523,11 +515,9 @@
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', 'abort'] +
- ['why not'] * 2)
+ self.context.status.check_names(['why not', 'commit'] + ['why not'] * 2)
# After a delay, must flush the queue.
timestamp.append(timestamp[-1] + pc.COMMIT_BURST_DELAY + 1)
pc.scan_results()
@@ -537,13 +527,11 @@
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', 'abort'] * 2)
+ self.context.status.check_names(['why not', 'commit'] * 2)
def testIgnored(self):
issue = 31337
@@ -578,9 +566,8 @@
[ _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', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
self.context.checkout.check_calls(
self._prepare_apply_commit(0, issue, server_hooks_missing=True))
@@ -661,9 +648,8 @@
[ _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', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
self.context.checkout.check_calls(
self._prepare_apply_commit(0, issue))
@@ -702,9 +688,8 @@
[ _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', 'abort'])
+ self.context.status.check_names(['initial', 'why not', 'commit'])
self.context.checkout.check_calls(
self._prepare_apply_commit(0, issue))
@@ -735,9 +720,8 @@
[_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', 'abort'])
+ self.context.status.check_names(['why not', 'commit'])
self.context.checkout.check_calls(
self._prepare_apply_commit(0, issue))
diff --git a/tests/project_test.py b/tests/project_test.py
index f885fb1..a559781 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(16))
+ self.time = map(lambda x: float(x*35), range(15))
self.urlrequests = [
('https://chromium-status.appspot.com/allstatus?format=json&endTime=85',
# Cheap hack here.
@@ -486,7 +486,6 @@
"{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([
@@ -498,7 +497,7 @@
"u'[email protected]')",
])
self.context.status.check_names(['initial', 'why not', 'why not',
- 'why not', 'commit', 'abort'])
+ 'why not', 'commit'])
