CQ to always post a message when Commit box is unchecked.

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

[Paweł Hajdan Jr., 2014-01-28 01:13:45]

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
nit: Indentation in these "or" expressions might be confusing.

How about just constructing the actual message first, and then just using
variable name in _discard_pending, e.g.

message = e.status or ...
self._discard_pending(... , message)

Applies to all lines where currently "or" is at the end of the line.

https://codereview.chromium.org/138173005/diff/1/pending_manager.py#newcode528
pending_manager.py:528: # Apply default message here
nit: This comment line seems to be out of place - maybe just remove it.

https://codereview.chromium.org/138173005/diff/1/verification/fake.py
File verification/fake.py (right):

https://codereview.chromium.org/138173005/diff/1/verification/fake.py#newcode21
verification/fake.py:21: # Make sure to leave a message, so CQ tests can
reliably test for it
nit: End all sentences with a dot, here and below.

[Sergey Berezin, 2014-01-28 18:40:55]

PTAL.

I changed the "msg or default_msg" expressions to more explicit if not message:
... control flow, hopefully this reads better. And added periods to sentences.
No other changes.

On 2014/01/28 01:13:45, Paweł Hajdan Jr. wrote:
> 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
> nit: Indentation in these "or" expressions might be confusing.
> 
> How about just constructing the actual message first, and then just using
> variable name in _discard_pending, e.g.
> 
> message = e.status or ...
> self._discard_pending(... , message)
> 
> Applies to all lines where currently "or" is at the end of the line.
> 
> https://codereview.chromium.org/138173005/diff/1/pending_manager.py#newcode528
> pending_manager.py:528: # Apply default message here
> nit: This comment line seems to be out of place - maybe just remove it.
> 
> https://codereview.chromium.org/138173005/diff/1/verification/fake.py
> File verification/fake.py (right):
> 
>
https://codereview.chromium.org/138173005/diff/1/verification/fake.py#newcode21
> verification/fake.py:21: # Make sure to leave a message, so CQ tests can
> reliably test for it
> nit: End all sentences with a dot, here and below.

[Paweł Hajdan Jr., 2014-01-28 19:34:39]

LGTM

[commit-bot: I haz the power, 2014-01-28 20:02:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyberezin@chromium.org/138173005/2...

[commit-bot: I haz the power, 2014-01-28 20:03:28]

Change committed as 247488

[Sergey Berezin, 2014-02-04 00:06:05]

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..