Add a webkit-patch command which gets a list of relevant try jobs.

Add a webkit-patch command which gets a list of relevant try jobs.

This is part of local command which will be used to rebaseline layout tests from try job results.

See: https://docs.google.com/document/d/1Cl3KR_9xkBFHsjwoXnnN-ya6BSdKP1oNkoIRuKVr75k/edit#heading=h.6wchzcwf8xmh

BUG=474273
Committed: https://crrev.com/c9844abe312e09a18584036971e6b2cf10eba353
Cr-Commit-Position: refs/heads/master@{#401352}

[qyearsley, 2016-06-14 22:45:35]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 22:46:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069863002/20001

[qyearsley, 2016-06-14 22:50:41]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, prasadv@chromium.org, wkorman@chromium.org

[qyearsley, 2016-06-14 22:50:42]

https://codereview.chromium.org/2069863002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2069863002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:5: """Utility
functions to communicate with Rietveld."""
This may only be used by webkit-patch rebaseline-from-try-jobs, but these
functions seemed logically separate enough that I wanted to separate them out
into a different file.

https://codereview.chromium.org/2069863002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:22:
patchset_number: Use a specific patchset instead of the latest one.
Added this because I anticipate adding a --patchset-number option to
webkit-patch rebaseline-from-try-jobs, in case people want to download baselines
from a previous patchset.

It would also be possible to not add this option, and always try to download
baselines from the last patchset that has completed try jobs for the builders
we're interested in.

https://codereview.chromium.org/2069863002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py
(right):

https://codereview.chromium.org/2069863002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py:33:
name = "rebaseline-from-try-jobs"
This name is terribly long -- any suggestions for shorter names?

[commit-bot: I haz the power, 2016-06-15 00:12:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 00:12:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qyearsley, 2016-06-15 04:44:23]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 04:44:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069863002/40001

[commit-bot: I haz the power, 2016-06-15 05:54:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 05:54:11]

Dry run: This issue passed the CQ dry run.

[wkorman, 2016-06-15 17:43:17]

lgtm

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:52: return
json.loads(web.get_binary(url))
Maybe catch error and deal gracefully, returning empty string or otherwise?

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

It looks like we don't do that elsewhere currently, for example:

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

but perhaps now is a good time to start vs full stack trace killing the script.
You know what they say about error codes, Always Be Checking, or somesuch.
Thoughts?

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:56: return
'https://codereview.chromium.org/api/%s' % issue_number
Perhaps move string to BASE_CODEREVIEW_URL constant as file or method constant

[qyearsley, 2016-06-15 21:34:15]

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:51: assert not
isinstance(web, tuple), web
Note, this was a debugging statement, removing now.

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:52: return
json.loads(web.get_binary(url))
On 2016/06/15 at 17:43:17, wkorman wrote:
> Maybe catch error and deal gracefully, returning empty string or otherwise?
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> It looks like we don't do that elsewhere currently, for example:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> but perhaps now is a good time to start vs full stack trace killing the
script. You know what they say about error codes, Always Be Checking, or
somesuch. Thoughts?

Advantages of catching errors and printing error messages:
 - It's clearer to the user what happened when the response from Rietveld is
wrong.
 - More explicit.

Advantages of leaving it with no error checking:
 - Code is slightly less simple.
 - The program will terminate immediately after receiving a bad response, which
is likely what we want to happen in this case.

In the latest patchset, I've now made it so:
 - _get_json may log error messages (including URL or response contents that
failed)
 - these errors are handled and latest_try_jobs returns an empty list in the
case of any errors.


Any thoughts? Is it too verbose, or is it OK?

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:56: return
'https://codereview.chromium.org/api/%s' % issue_number
On 2016/06/15 at 17:43:17, wkorman wrote:
> Perhaps move string to BASE_CODEREVIEW_URL constant as file or method constant

Done.

[qyearsley, 2016-06-16 01:20:35]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 01:20:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069863002/60001

[commit-bot: I haz the power, 2016-06-16 03:00:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 03:00:33]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-06-20 16:44:50]

On 2016/06/16 at 03:00:34, commit-bot wrote:
> Dry run: This issue passed the CQ dry run.

Dirk or Prasad, could you see anything to improve about this start to
rebaseline_from_try_jobs.py or helper module rietveld.py, or unit tests?

[wkorman, 2016-06-22 01:22:44]

lgtm

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2069863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:52: return
json.loads(web.get_binary(url))
On 2016/06/15 at 21:34:15, qyearsley wrote:
> On 2016/06/15 at 17:43:17, wkorman wrote:
> > Maybe catch error and deal gracefully, returning empty string or otherwise?
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > 
> > It looks like we don't do that elsewhere currently, for example:
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > 
> > but perhaps now is a good time to start vs full stack trace killing the
script. You know what they say about error codes, Always Be Checking, or
somesuch. Thoughts?
> 
> Advantages of catching errors and printing error messages:
>  - It's clearer to the user what happened when the response from Rietveld is
wrong.
>  - More explicit.
> 
> Advantages of leaving it with no error checking:
>  - Code is slightly less simple.
>  - The program will terminate immediately after receiving a bad response,
which is likely what we want to happen in this case.
> 
> In the latest patchset, I've now made it so:
>  - _get_json may log error messages (including URL or response contents that
failed)
>  - these errors are handled and latest_try_jobs returns an empty list in the
case of any errors.
> 
> 
> Any thoughts? Is it too verbose, or is it OK?

Looks ok to me, there are a few other places in webkitpy that check
urllib2.URLError and they're roughly similar.

[qyearsley, 2016-06-22 16:32:09]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-06-22 16:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069863002/120001

[commit-bot: I haz the power, 2016-06-22 18:18:20]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 18:22:37]

Description was changed from

==========
Add a webkit-patch command which gets a list of relevant try jobs.

This is part of local command which will be used to rebaseline layout tests from
try job results.

See:
https://docs.google.com/document/d/1Cl3KR_9xkBFHsjwoXnnN-ya6BSdKP1oNkoIRuKVr7...

BUG=474273
==========

to

==========
Add a webkit-patch command which gets a list of relevant try jobs.

This is part of local command which will be used to rebaseline layout tests from
try job results.

See:
https://docs.google.com/document/d/1Cl3KR_9xkBFHsjwoXnnN-ya6BSdKP1oNkoIRuKVr7...

BUG=474273

Committed: https://crrev.com/c9844abe312e09a18584036971e6b2cf10eba353
Cr-Commit-Position: refs/heads/master@{#401352}
==========

[commit-bot: I haz the power, 2016-06-22 18:22:38]

Patchset 7 (id:??) landed as
https://crrev.com/c9844abe312e09a18584036971e6b2cf10eba353
Cr-Commit-Position: refs/heads/master@{#401352}