Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 138173005: CQ to always post a message when Commit box is unchecked. (Closed)

Created:
5 years, 7 months ago by Sergey Berezin
Modified:
5 years, 7 months ago
Reviewers:
Paweł Hajdan Jr.
CC:
chromium-reviews, cmp-cc_chromium.org, ilevy-cc_chromium.org
Visibility:
Public.

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Addressed reviewer's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -34 lines) Patch
M pending_manager.py View 1 8 chunks +38 lines, -28 lines 0 comments Download
M tests/pending_manager_test.py View 5 chunks +7 lines, -5 lines 0 comments Download
M verification/fake.py View 1 3 chunks +10 lines, -1 line 0 comments Download

Messages

Total messages: 7 (0 generated)
Sergey Berezin
5 years, 7 months ago (2014-01-28 00:26:03 UTC) #1
Paweł Hajdan Jr.
Generally looks good, minor comments only. https://codereview.chromium.org/138173005/diff/1/pending_manager.py File pending_manager.py (right): https://codereview.chromium.org/138173005/diff/1/pending_manager.py#newcode269 pending_manager.py:269: self._discard_pending(e.pending, e.status or ...
5 years, 7 months ago (2014-01-28 01:13:45 UTC) #2
Sergey Berezin
PTAL. I changed the "msg or default_msg" expressions to more explicit if not message: ...
5 years, 7 months ago (2014-01-28 18:40:55 UTC) #3
Paweł Hajdan Jr.
LGTM
5 years, 7 months ago (2014-01-28 19:34:39 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyberezin@chromium.org/138173005/20001
5 years, 7 months ago (2014-01-28 20:02:49 UTC) #5
commit-bot: I haz the power
Change committed as 247488
5 years, 7 months ago (2014-01-28 20:03:28 UTC) #6
Sergey Berezin
5 years, 6 months ago (2014-02-04 00:06:05 UTC) #7
Message was sent while issue was closed.
A revert of this CL has been created in
https://codereview.chromium.org/138133014/ by sergeyberezin@chromium.org.

The reason for reverting is: This created a lot of spam at commit event for some
obscure reason. I couldn't fix it fast, so reverting..

Powered by Google App Engine
This is Rietveld 408576698