In webkit-patch rebaseline-cl, abort if results are missing.

In webkit-patch rebaseline-cl, abort if results are missing.

Reason: In general, if layout test results for some builder cannot be found in GS, and they are not downloaded, then when baselines are deduped, some old (incorrect) baselines may be left behind for those platforms that we couldn't get results for.

As-is, rebaseline-cl sort of depends on having results for all support platforms whenever rebaselining in order to get correct results.

Main changes in this CL:

 1. Results are always fetched, even if tests are directly passed. This way the behavior is consistent regardless of whether the list of tests to rebaseline is explicitly specified or decided based on results.
 2. Fetching of the results is done in a method called _fetch_results(), which may raise an exception if any results are missing; this exception is caught in execute(), and an error message is printed.
 3. _builds_to_tests is removed, and part of _tests_to_rebaseline is moved to _fetch_results(). buildbot.fetch_results is used instead of buildbot.fetch_layout_test_results.

Later, we might consider adding a flag to force continuing with rebaselining even if some results are not available, but I'm not sure if this is necessary.

BUG=669653
Committed: https://crrev.com/ff4522901de3100f338fc1f606e301ede5e96c3d
Cr-Commit-Position: refs/heads/master@{#436982}

[qyearsley, 2016-12-03 00:21:22]

Description was changed from

==========
In webkit-patch rebaseline-cl, abort if results are missing.

BUG=669653
==========

to

==========
In webkit-patch rebaseline-cl, abort if results are missing.

Reason: In general, if layout test results for some builder cannot be found in
GS, and they are not downloaded, then when baselines are deduped, some old
(incorrect) baselines may be left behind for those platforms that we couldn't
get results for.

As-is, rebaseline-cl sort of depends on having results for all support platforms
whenever rebaselining in order to get correct results.

Main changes in this CL:

 1. Results are always fetched, even if tests are directly passed. This way the
behavior is consistent regardless of whether the list of tests to rebaseline is
explicitly specified or decided based on results.
 2. Fetching of the results is done in a method called _fetch_results(), which
may raise an exception if any results are missing; this exception is caught in
execute(), and an error message is printed.
 3. _builds_to_tests is removed, and part of _tests_to_rebaseline is moved to
_fetch_results(). buildbot.fetch_results is used instead of
buildbot.fetch_layout_test_results.

Later, we might consider adding a flag to force continuing with rebaselining
even if some results are not available, but I'm not sure if this is necessary.

BUG=669653
==========

[qyearsley, 2016-12-03 00:21:23]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, jeffcarp@chromium.org

[qyearsley, 2016-12-03 00:21:38]

qyearsley@chromium.org changed reviewers:
+ wkorman@chromium.org
- jeffcarp@chromium.org

[wkorman, 2016-12-05 05:58:35]

lgtm

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69:
_log.debug('Getting results for Rietveld issue %d.', issue_number)
Q for whomever may know: when are we moving to Gerrit, and are there
ramifications for this and other tools?

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:159:
raise _MissingResults(
Worth documenting exceptions a method may raise in the method doc comment?

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:239:
class _MissingResults(Exception):
Probably worth a doc comment or something in name to make it clearer what the
missing results are, since results is very general.

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:242:
self.tool.buildbot.set_results(Build("MOCK Try Win", 5000), None)
Perhaps worth a test case that has at least one with results as well.

[qyearsley, 2016-12-05 17:55:21]

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69:
_log.debug('Getting results for Rietveld issue %d.', issue_number)
On 2016/12/05 at 05:58:35, wkorman wrote:
> Q for whomever may know: when are we moving to Gerrit, and are there
ramifications for this and other tools?

There are ramifications for this tool, but not so much for others. Currently,
this tool makes direct requests to Rietveld to:
 - List of try jobs for the latest patch for a CL.
 - List changed files for a CL.

The reason why requests are made to Rietveld is so that arbitrary CLs including
other people's CLs can be supported -- but, if rebaseline-cl was changed only
support CLs associated with the current local branch, then this information can
also be gotten via `git diff` and `git cl try-results`.

For other things, including status of try jobs and starting try jobs,
rebaseline-cl uses `git cl try` and `git cl try-results`.

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:159:
raise _MissingResults(
On 2016/12/05 at 05:58:35, wkorman wrote:
> Worth documenting exceptions a method may raise in the method doc comment?

Added a "Raises:" part to the docstring.

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:239:
class _MissingResults(Exception):
On 2016/12/05 at 05:58:35, wkorman wrote:
> Probably worth a doc comment or something in name to make it clearer what the
missing results are, since results is very general.

Changed name to _MissingLayoutTestResults.

My rationale for using an exception was originally so that I could raise it in
_tests_to_rebaseline and catch it in execute() and not have to worry about
passing a special value up the call stack to indicate that the whole process
should be aborted.

But now, it's raised directly in _fetch_results and it would probably be just as
simple now to print an error and return None from _fetch_results instead of
adding this exception here. Does that sound better?

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:242:
self.tool.buildbot.set_results(Build("MOCK Try Win", 5000), None)
On 2016/12/05 at 05:58:35, wkorman wrote:
> Perhaps worth a test case that has at least one with results as well.

Currently test_execute_with_issue_number_given (and several of the other methods
above) test the case where there is results -- but they don't explicitly set the
results. Instead, they rely on results implicitly set by default in MockBuildBot
(self.tool.buildbot).

I think it would be better to set results explicitly though, maybe in the setUp
method for this class. Sound good?

[wkorman, 2016-12-05 18:56:25]

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69:
_log.debug('Getting results for Rietveld issue %d.', issue_number)
On 2016/12/05 at 17:55:21, qyearsley wrote:
> On 2016/12/05 at 05:58:35, wkorman wrote:
> > Q for whomever may know: when are we moving to Gerrit, and are there
ramifications for this and other tools?
> 
> There are ramifications for this tool, but not so much for others. Currently,
this tool makes direct requests to Rietveld to:
>  - List of try jobs for the latest patch for a CL.
>  - List changed files for a CL.
> 
> The reason why requests are made to Rietveld is so that arbitrary CLs
including other people's CLs can be supported -- but, if rebaseline-cl was
changed only support CLs associated with the current local branch, then this
information can also be gotten via `git diff` and `git cl try-results`.
> 
> For other things, including status of try jobs and starting try jobs,
rebaseline-cl uses `git cl try` and `git cl try-results`.

Sounds reasonable, perhaps worth checking in with agable@ who I think may be
involved in the gerrit move to make sure this is considered as part of the
migration. And/or open a bug to track your note re: arbitrary CLs. Is there a
way to find out how many CLs would be impacted if we shifted to only use local
branches? We could do a small survey.

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:239:
class _MissingResults(Exception):
On 2016/12/05 at 17:55:21, qyearsley wrote:
> On 2016/12/05 at 05:58:35, wkorman wrote:
> > Probably worth a doc comment or something in name to make it clearer what
the missing results are, since results is very general.
> 
> Changed name to _MissingLayoutTestResults.
> 
> My rationale for using an exception was originally so that I could raise it in
_tests_to_rebaseline and catch it in execute() and not have to worry about
passing a special value up the call stack to indicate that the whole process
should be aborted.
> 
> But now, it's raised directly in _fetch_results and it would probably be just
as simple now to print an error and return None from _fetch_results instead of
adding this exception here. Does that sound better?

Yes, that sounds preferable.

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:242:
self.tool.buildbot.set_results(Build("MOCK Try Win", 5000), None)
On 2016/12/05 at 17:55:21, qyearsley wrote:
> On 2016/12/05 at 05:58:35, wkorman wrote:
> > Perhaps worth a test case that has at least one with results as well.
> 
> Currently test_execute_with_issue_number_given (and several of the other
methods above) test the case where there is results -- but they don't explicitly
set the results. Instead, they rely on results implicitly set by default in
MockBuildBot (self.tool.buildbot).
> 
> I think it would be better to set results explicitly though, maybe in the
setUp method for this class. Sound good?

Ah, I see. Yes, that wasn't immediately apparent. Does not need to be changed in
this CL, up to you.

[qyearsley, 2016-12-07 00:42:27]

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:69:
_log.debug('Getting results for Rietveld issue %d.', issue_number)
On 2016/12/05 at 18:56:25, wkorman wrote:
> On 2016/12/05 at 17:55:21, qyearsley wrote:
> > On 2016/12/05 at 05:58:35, wkorman wrote:
> > > Q for whomever may know: when are we moving to Gerrit, and are there
ramifications for this and other tools?
> > 
> > There are ramifications for this tool, but not so much for others.
Currently, this tool makes direct requests to Rietveld to:
> >  - List of try jobs for the latest patch for a CL.
> >  - List changed files for a CL.
> > 
> > The reason why requests are made to Rietveld is so that arbitrary CLs
including other people's CLs can be supported -- but, if rebaseline-cl was
changed only support CLs associated with the current local branch, then this
information can also be gotten via `git diff` and `git cl try-results`.
> > 
> > For other things, including status of try jobs and starting try jobs,
rebaseline-cl uses `git cl try` and `git cl try-results`.
> 
> Sounds reasonable, perhaps worth checking in with agable@ who I think may be
involved in the gerrit move to make sure this is considered as part of the
migration. And/or open a bug to track your note re: arbitrary CLs. Is there a
way to find out how many CLs would be impacted if we shifted to only use local
branches? We could do a small survey.

Aye - just filed a bug for this: http://crbug.com/671684.

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py:239:
class _MissingResults(Exception):
On 2016/12/05 at 18:56:25, wkorman wrote:
> On 2016/12/05 at 17:55:21, qyearsley wrote:
> > On 2016/12/05 at 05:58:35, wkorman wrote:
> > > Probably worth a doc comment or something in name to make it clearer what
the missing results are, since results is very general.
> > 
> > Changed name to _MissingLayoutTestResults.
> > 
> > My rationale for using an exception was originally so that I could raise it
in _tests_to_rebaseline and catch it in execute() and not have to worry about
passing a special value up the call stack to indicate that the whole process
should be aborted.
> > 
> > But now, it's raised directly in _fetch_results and it would probably be
just as simple now to print an error and return None from _fetch_results instead
of adding this exception here. Does that sound better?
> 
> Yes, that sounds preferable.

Now done :-)

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

https://codereview.chromium.org/2547153002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:242:
self.tool.buildbot.set_results(Build("MOCK Try Win", 5000), None)
On 2016/12/05 at 18:56:25, wkorman wrote:
> On 2016/12/05 at 17:55:21, qyearsley wrote:
> > On 2016/12/05 at 05:58:35, wkorman wrote:
> > > Perhaps worth a test case that has at least one with results as well.
> > 
> > Currently test_execute_with_issue_number_given (and several of the other
methods above) test the case where there is results -- but they don't explicitly
set the results. Instead, they rely on results implicitly set by default in
MockBuildBot (self.tool.buildbot).
> > 
> > I think it would be better to set results explicitly though, maybe in the
setUp method for this class. Sound good?
> 
> Ah, I see. Yes, that wasn't immediately apparent. Does not need to be changed
in this CL, up to you.

Probably nicer to make this change in a separate CL, I think.

[qyearsley, 2016-12-07 00:42:33]

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

[commit-bot: I haz the power, 2016-12-07 00:43:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 02:07:20]

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

[commit-bot: I haz the power, 2016-12-07 02:07:21]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-12-07 16:39:13]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-12-07 16:39:14]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2547153002/#ps60001
(title: "Remove exception; use return value of None to indicate missing
results.")

[commit-bot: I haz the power, 2016-12-07 16:39:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 16:45:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481128753835670,
"parent_rev": "1c6d24770f45320c38af81e21980e7bd60b4b904", "commit_rev":
"e395eaef504b30b5718bc9b76aa5097bff6c2e69"}

[commit-bot: I haz the power, 2016-12-07 16:45:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-07 16:50:22]

Description was changed from

==========
In webkit-patch rebaseline-cl, abort if results are missing.

Reason: In general, if layout test results for some builder cannot be found in
GS, and they are not downloaded, then when baselines are deduped, some old
(incorrect) baselines may be left behind for those platforms that we couldn't
get results for.

As-is, rebaseline-cl sort of depends on having results for all support platforms
whenever rebaselining in order to get correct results.

Main changes in this CL:

 1. Results are always fetched, even if tests are directly passed. This way the
behavior is consistent regardless of whether the list of tests to rebaseline is
explicitly specified or decided based on results.
 2. Fetching of the results is done in a method called _fetch_results(), which
may raise an exception if any results are missing; this exception is caught in
execute(), and an error message is printed.
 3. _builds_to_tests is removed, and part of _tests_to_rebaseline is moved to
_fetch_results(). buildbot.fetch_results is used instead of
buildbot.fetch_layout_test_results.

Later, we might consider adding a flag to force continuing with rebaselining
even if some results are not available, but I'm not sure if this is necessary.

BUG=669653
==========

to

==========
In webkit-patch rebaseline-cl, abort if results are missing.

Reason: In general, if layout test results for some builder cannot be found in
GS, and they are not downloaded, then when baselines are deduped, some old
(incorrect) baselines may be left behind for those platforms that we couldn't
get results for.

As-is, rebaseline-cl sort of depends on having results for all support platforms
whenever rebaselining in order to get correct results.

Main changes in this CL:

 1. Results are always fetched, even if tests are directly passed. This way the
behavior is consistent regardless of whether the list of tests to rebaseline is
explicitly specified or decided based on results.
 2. Fetching of the results is done in a method called _fetch_results(), which
may raise an exception if any results are missing; this exception is caught in
execute(), and an error message is printed.
 3. _builds_to_tests is removed, and part of _tests_to_rebaseline is moved to
_fetch_results(). buildbot.fetch_results is used instead of
buildbot.fetch_layout_test_results.

Later, we might consider adding a flag to force continuing with rebaselining
even if some results are not available, but I'm not sure if this is necessary.

BUG=669653

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

[commit-bot: I haz the power, 2016-12-07 16:50:23]

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