GTTF: changes that allow testing CQ locally

GTTF: changes that allow testing CQ locally

This includes the following changes:

1) Use ReadOnlyRietveld from depot_tools (so that it can stay in sync with Rietveld)
2) No longer support --only-issue in dry run mode
3) Fix a unicode bug leading to an exception during testing

I've tested this as "./commit_queue.py --fake --no-try --project chromium"

BUG=291335
R=iannucci@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225056

[Paweł Hajdan Jr., 2013-09-20 17:38:20]

+Robbie, could you take a look?

[iannucci, 2013-09-20 23:42:53]

lgtm, Isaac any comments?

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py
File commit-queue/commit_queue.py (right):

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue....
commit-queue/commit_queue.py:69: return unicode('FAKE')
u'FAKE' ?

What is this fixing (I'm assuming something breaks w/o it)?

[Paweł Hajdan Jr., 2013-09-21 01:40:26]

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py
File commit-queue/commit_queue.py (right):

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue....
commit-queue/commit_queue.py:69: return unicode('FAKE')
On 2013/09/20 23:42:53, iannucci wrote:
> u'FAKE' ?
> 
> What is this fixing (I'm assuming something breaks w/o it)?

It's fixing an exception that unicode is expected when serializing the state.

[Isaac (away), 2013-09-21 09:32:49]

Could you make description more specific?

It seems like this does three things:
1) Use ReadOnlyRietveld from depot_tools.  This doesn't simplify testing but
does improve architecture
2) blocks the option I commented on below
3) fixes a unicode bug

am I missing something?

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py
File commit-queue/commit_queue.py (right):

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue....
commit-queue/commit_queue.py:191: if options.only_issue:
How come we're blocking this case now?  I think the intention was that every
option is possible in read only mode before switching to commit mode.

[Paweł Hajdan Jr., 2013-09-24 02:40:42]

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py
File commit-queue/commit_queue.py (right):

https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue....
commit-queue/commit_queue.py:191: if options.only_issue:
On 2013/09/21 09:32:49, Isaac wrote:
> How come we're blocking this case now?  I think the intention was that every
> option is possible in read only mode before switching to commit mode.

Isaac, do you consider this blocking the CL or just a comment?

I think for testing --only-issue doesn't really help in dry run mode. It also
doesn't reflect how CQ operates in production, so arguably the test is less
realistic.

Note that I can totally see the combination of only-issue and fake checkout
being useful. It should still work, and CQ would post comments to the real issue
but not touch the real repo.

[Isaac (use chromium), 2013-09-24 12:45:34]

Can you explain the advantage blocking this set of options?

phajdan.jr@chromium.org wrote:

>
>https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py
>File commit-queue/commit_queue.py (right):
>
>https://codereview.chromium.org/24303002/diff/4001/commit-queue/commit_queue.py#newcode191
>commit-queue/commit_queue.py:191: if options.only_issue:
>On 2013/09/21 09:32:49, Isaac wrote:
>> How come we're blocking this case now?  I think the intention was that
>every
>> option is possible in read only mode before switching to commit mode.
>
>Isaac, do you consider this blocking the CL or just a comment?
>
>I think for testing --only-issue doesn't really help in dry run mode. It
>also doesn't reflect how CQ operates in production, so arguably the test
>is less realistic.
>
>Note that I can totally see the combination of only-issue and fake
>checkout being useful. It should still work, and CQ would post comments
>to the real issue but not touch the real repo.
>
>https://codereview.chromium.org/24303002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Paweł Hajdan Jr., 2013-09-24 20:02:13]

Committed patchset #2 manually as r225056 (presubmit successful).

[Paweł Hajdan Jr., 2013-09-24 20:17:47]

On 2013/09/24 12:45:34, Isaac (use chromium) wrote:
> Can you explain the advantage blocking this set of options?

I think my last post should answer that, but here is another attempt (sorry I'm
not explaining it so well).

By the way my understanding is that it's not explicitly blocking the review and
given review latency over the ocean I've committed the change.

The advantage is:

1) simplicity - less combinations of options to support, which means it can work
much more reliably (and just be simpler to read and verify)

2) it doesn't seem to make much sense to me to test with dry-run and only-issue.
Real run with only-issue + fake checkout totally makes sense and is still
supported (and I have no plans of changing that).

Please let me know if you have specific questions about this or something not
covered by this.

Note that I was asking people questions about how to test CQ locally, and none
of the answers involved only-issue IIRC (sorry if I just forgot something). This
change makes it possible to test several cycles of CQ, and I'm going to use it
to test my other CQ CLs and other improvements to come.

Please remember to always make constructive comments, i.e. when pointing
something out have an acceptable alternative. :)

[Isaac (away), 2013-09-24 20:58:18]

On 2013/09/24 20:17:47, Paweł Hajdan Jr. wrote:
> On 2013/09/24 12:45:34, Isaac (use chromium) wrote:
> > Can you explain the advantage blocking this set of options?
> 
> I think my last post should answer that, but here is another attempt (sorry
I'm
> not explaining it so well).
> 
> By the way my understanding is that it's not explicitly blocking the review
and
> given review latency over the ocean I've committed the change.
> 
> The advantage is:
> 
> 1) simplicity - less combinations of options to support, which means it can
work
> much more reliably (and just be simpler to read and verify)
> 
> 2) it doesn't seem to make much sense to me to test with dry-run and
only-issue.
> Real run with only-issue + fake checkout totally makes sense and is still
> supported (and I have no plans of changing that).
> 
> Please let me know if you have specific questions about this or something not
> covered by this.
> 
> Note that I was asking people questions about how to test CQ locally, and none
> of the answers involved only-issue IIRC (sorry if I just forgot something).
This
> change makes it possible to test several cycles of CQ, and I'm going to use it
> to test my other CQ CLs and other improvements to come.

I think this is reasonable, but there are (admittedly rare) scenarios where
you'd want this combination.  For example, the real CQ gets stuck running
presubmit for an issue and you can't repro it with just git cl presubmit (which
is very similar, but not exactly the same as CQ).  I'd be more supportive of
removing option configurations if you're landing a more complex issue that a
unneeded configuration would make more complex.

That being said, my comments were not intended to be blocking.  Thanks for the
clarifications.
> 
> Please remember to always make constructive comments, i.e. when pointing
> something out have an acceptable alternative. :)