Fetch retry summary and use it to determine what tests to rebaseline.

Fetch retry summary and use it to determine what tests to rebaseline.

This CL changes RebaselineCL._tests_to_rebaseline so that after it fetches the layout test results and extracts a list of failing tests to rebaseline, it also tries to fetch the retry summary from Google Storage; if this is available, then it filters the tests to rebaseline to only include tests that failed with the patch but not without.

To support this, this CL also adds a method in the BuildBot and BuildBotMock classes.

BUG=642980
Committed: https://crrev.com/f9833f761d7bd907e91fcfbc669aaa5235a5a471
Cr-Commit-Position: refs/heads/master@{#427536}

[qyearsley, 2016-10-21 22:36:30]

Description was changed from

==========
Fetch retry summary and use it to determine what tests to rebaseline.

BUG=642980
==========

to

==========
Fetch retry summary and use it to determine what tests to rebaseline.

This CL changes RebaselineCL._tests_to_rebaseline so that after it fetches the
layout test results and extracts a list of failing tests to rebaseline, it also
tries to fetch the retry summary from Google Storage; if this is available, then
it filters the tests to rebaseline to only include tests that failed with the
patch but not without.

To support this, this CL also adds a method in the BuildBot and BuildBotMock
classes.

BUG=642980
==========

[qyearsley, 2016-10-21 22:36:30]

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

[qyearsley, 2016-10-21 22:39:21]

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

[qyearsley, 2016-10-21 22:39:22]

This CL contains the changes in rebaseline-cl to fetch and use the
retry_summary.json uploaded by after "webkit_tests (retry summary)" in order to
decide what tests to rebaseline.

Is there anything here that isn't clear or could be made simpler or more
readable? Does the idea overall still seem OK?

[qyearsley, 2016-10-24 18:32:28]

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

[commit-bot: I haz the power, 2016-10-24 18:33:00]

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-10-24 20:11:01]

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

[commit-bot: I haz the power, 2016-10-24 20:11:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wkorman, 2016-10-24 21:47:38]

lgtm

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

https://codereview.chromium.org/2446463002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:84: with and
without a patch without a patch in a try job, with information
dupe 'without a patch' text

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

https://codereview.chromium.org/2446463002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:151:
def test_execute_with_flaky_test_that_fails_on_retry(self):
Do we have test coverage to make sure with code added in this change, with
--only-changed-tests passed:
- there is no avail retry summary (would want to fetch then)
- there is a retry summary, the test(s) changed in the current change *are not*
failing in the retry (would not want to fetch then)
- there is a retry summary, the test(s) changed in the current change *are*
failing in the retry (would want to fetch then)

My expectation in parens but open to thought.

[qyearsley, 2016-10-25 21:09:25]

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

[commit-bot: I haz the power, 2016-10-25 21:09:56]

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

[qyearsley, 2016-10-25 21:53:53]

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

https://codereview.chromium.org/2446463002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py:84: with and
without a patch without a patch in a try job, with information
On 2016/10/24 at 21:47:38, wkorman wrote:
> dupe 'without a patch' text

Fixed

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

https://codereview.chromium.org/2446463002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:151:
def test_execute_with_flaky_test_that_fails_on_retry(self):
On 2016/10/24 at 21:47:38, wkorman wrote:
> Do we have test coverage to make sure with code added in this change, with
--only-changed-tests passed:
> - there is no avail retry summary (would want to fetch then)
> - there is a retry summary, the test(s) changed in the current change *are
not* failing in the retry (would not want to fetch then)
> - there is a retry summary, the test(s) changed in the current change *are*
failing in the retry (would want to fetch then)
> 
> My expectation in parens but open to thought.

In the first patchset:

 1. scenario 1 isn't covered yet
 2. scenario 2 should be covered by the other test methods above, since a canned
response is added in setUp with buildbot.set_retry_sumary_json, with no tests
"ignored"; in schenario 2, where the tests fail with the patch but not in the
retry without the patch, then that means they *should* be rebaselined and we
would want to fetch then.
 3. scenario 3 should be covered by this method,
test_execute_with_flaky_test_that_fails_on_retry, where a test is in the
"ignored" list and no baseline is downloaded for that test.

Now added a test for the first case.

[qyearsley, 2016-10-25 21:53:58]

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

[commit-bot: I haz the power, 2016-10-25 21:54:34]

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

[wkorman, 2016-10-25 23:15:34]

lgtm

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

https://codereview.chromium.org/2446463002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py:151:
def test_execute_with_flaky_test_that_fails_on_retry(self):
On 2016/10/25 21:53:52, qyearsley wrote:
> On 2016/10/24 at 21:47:38, wkorman wrote:
> > Do we have test coverage to make sure with code added in this change, with
> --only-changed-tests passed:
> > - there is no avail retry summary (would want to fetch then)
> > - there is a retry summary, the test(s) changed in the current change *are
> not* failing in the retry (would not want to fetch then)
> > - there is a retry summary, the test(s) changed in the current change *are*
> failing in the retry (would want to fetch then)
> > 
> > My expectation in parens but open to thought.
> 
> In the first patchset:
> 
>  1. scenario 1 isn't covered yet
>  2. scenario 2 should be covered by the other test methods above, since a
canned
> response is added in setUp with buildbot.set_retry_sumary_json, with no tests
> "ignored"; in schenario 2, where the tests fail with the patch but not in the
> retry without the patch, then that means they *should* be rebaselined and we
> would want to fetch then.
>  3. scenario 3 should be covered by this method,
> test_execute_with_flaky_test_that_fails_on_retry, where a test is in the
> "ignored" list and no baseline is downloaded for that test.
> 
> Now added a test for the first case.

LOL, I had it backwards, but thanks to the tests now we can have confidence!

[qyearsley, 2016-10-25 23:17:40]

The CQ bit was unchecked by qyearsley@chromium.org

[qyearsley, 2016-10-25 23:17:41]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-10-25 23:18:02]

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-10-25 23:43:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 23:46:17]

Description was changed from

==========
Fetch retry summary and use it to determine what tests to rebaseline.

This CL changes RebaselineCL._tests_to_rebaseline so that after it fetches the
layout test results and extracts a list of failing tests to rebaseline, it also
tries to fetch the retry summary from Google Storage; if this is available, then
it filters the tests to rebaseline to only include tests that failed with the
patch but not without.

To support this, this CL also adds a method in the BuildBot and BuildBotMock
classes.

BUG=642980
==========

to

==========
Fetch retry summary and use it to determine what tests to rebaseline.

This CL changes RebaselineCL._tests_to_rebaseline so that after it fetches the
layout test results and extracts a list of failing tests to rebaseline, it also
tries to fetch the retry summary from Google Storage; if this is available, then
it filters the tests to rebaseline to only include tests that failed with the
patch but not without.

To support this, this CL also adds a method in the BuildBot and BuildBotMock
classes.

BUG=642980

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

[commit-bot: I haz the power, 2016-10-25 23:46:18]

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

[Dirk Pranke, 2016-10-26 18:47:29]

lgtm