Add failure screenshots and render test images to results detail.

Add failure screenshots and render test images to results detail.

Review-Url: https://codereview.chromium.org/2701473003
Cr-Commit-Position: refs/heads/master@{#453039}
Committed: https://chromium.googlesource.com/chromium/src/+/191a06d04a38e8b322c326d42433cbb1f5575025

[mikecase (-- gone --), 2017-02-15 18:45:22]

mikecase@chromium.org changed reviewers:
+ hzl@chromium.org, jbudorick@chromium.org

[mikecase (-- gone --), 2017-02-15 18:45:22]

This CL would need to be cleaned up, but do either of you have any thoughts on
adding screenshot/render_test image into results details as links.

This change would...
(1) Result in me deleting/moving code in build/android/render_tests/ in favor of
using results_details. Would be nice to put all test results in one place.
(2) Require a hack where we upload images directly to GS bucket instead of
logdog because currently there is no way to view images uploaded to logdog I am
told.

[mikecase (-- gone --), 2017-02-16 17:12:09]

Description was changed from

==========
Add failure screenshots and render test images to results detail.

BUG=
==========

to

==========
Add failure screenshots and render test images to results detail.
==========

[mikecase (-- gone --), 2017-02-21 17:34:05]

please take a look at this when you have time.

[jbudorick, 2017-02-21 18:32:40]

This generally looks ok to me.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:275:
stream_name = 'logcat_%s_%s_%s' % (
Can you use unique_name here?

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:315:
temp_dir = None
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:328: m
= _RE_RENDER_IMAGE_NAME.match(failure_filename)
Do you do anything with m other than check it here?

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:330:
logging.warning('Unexpected file in render test failures, %s',
nit: "... failures: %s"
                  ^

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:341:
result.SetLink('%s_FAIL' % failure_filename, link)
nit: name the link something else. Not immediately sure what, though.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:354:
result.SetLink('%s_GOLDEN' % failure_filename, link)
nit: same

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:90: """Uploads image to Google
Storage.
This should not be in logdog_helper if it's not uploading to logdog.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:106: os.path.join(RENDER_TEST_BUCKET,
name)])
super nit: why is this not on the same line as image_path?

[jbudorick, 2017-02-21 18:37:12]

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:311:
if self._test_instance.should_save_images:
Also, would it make sense to extract this into its own function?

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:325:
for failure_filename in os.listdir(
Of note, though: process_render_test_results attempts to compute the image diff,
while this does not. That seems like it'd be useful functionality to preserve.

[mikecase (-- gone --), 2017-02-23 00:21:27]

CL is a little different now since I added back the image diffs. PTAL. I tested
is quite a bit locally and pretty sure everything works as intended.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:311:
if self._test_instance.should_save_images:
On 2017/02/21 at 18:37:12, jbudorick wrote:
> Also, would it make sense to extract this into its own function?

Done

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:315:
temp_dir = None
On 2017/02/21 at 18:32:40, jbudorick wrote:
>
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

Done

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:325:
for failure_filename in os.listdir(
On 2017/02/21 at 18:37:12, jbudorick wrote:
> Of note, though: process_render_test_results attempts to compute the image
diff, while this does not. That seems like it'd be useful functionality to
preserve.

Added diffing back. Makes this a bit more complex.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:328: m
= _RE_RENDER_IMAGE_NAME.match(failure_filename)
On 2017/02/21 at 18:32:39, jbudorick wrote:
> Do you do anything with m other than check it here?

no

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:330:
logging.warning('Unexpected file in render test failures, %s',
On 2017/02/21 at 18:32:39, jbudorick wrote:
> nit: "... failures: %s"
>                   ^

Done

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:341:
result.SetLink('%s_FAIL' % failure_filename, link)
On 2017/02/21 at 18:32:39, jbudorick wrote:
> nit: name the link something else. Not immediately sure what, though.

I changed this since I didnt want three separate links for _FAIL, _GOLDEN, and
now _DIFF

Instead I embed the three images into a very simple HTML page and only link
that.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:354:
result.SetLink('%s_GOLDEN' % failure_filename, link)
On 2017/02/21 at 18:32:39, jbudorick wrote:
> nit: same

See above.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
File build/android/pylib/utils/logdog_helper.py (right):

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:90: """Uploads image to Google
Storage.
On 2017/02/21 at 18:32:40, jbudorick wrote:
> This should not be in logdog_helper if it's not uploading to logdog.

done. Moved to its own module.

https://codereview.chromium.org/2701473003/diff/30007/build/android/pylib/uti...
build/android/pylib/utils/logdog_helper.py:106: os.path.join(RENDER_TEST_BUCKET,
name)])
On 2017/02/21 at 18:32:40, jbudorick wrote:
> super nit: why is this not on the same line as image_path?

it is now!

[the real yoland, 2017-02-23 18:39:07]

yolandyan@chromium.org changed reviewers:
+ yolandyan@chromium.org

[BigBossZhiling, 2017-02-23 18:43:45]

hzl@google.com changed reviewers:
+ hzl@google.com

[BigBossZhiling, 2017-02-23 18:43:45]

hzl@google.com changed reviewers:
+ hzl@google.com

[BigBossZhiling, 2017-02-23 18:43:45]

lgtm

[BigBossZhiling, 2017-02-23 18:43:46]

lgtm

[mikecase (-- gone --), 2017-02-23 19:39:06]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-02-23 19:39:06]

The patchset sent to the CQ was uploaded after l-g-t-m from hzl@google.com
Link to the patchset: https://codereview.chromium.org/2701473003/#ps110001
(title: "Small fix to HTML template")

[commit-bot: I haz the power, 2017-02-23 19:39:28]

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, 2017-02-23 19:39:29]

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

[commit-bot: I haz the power, 2017-02-23 19:39:30]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[the real yoland, 2017-02-24 23:00:32]

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:509:
temp_html.flush()
nit: I think exit implies flush?

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/ut...
File build/android/pylib/utils/google_storage_helper.py (right):

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/google_storage_helper.py:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
hmm, why not using upload_to_google_storage.py?

[mikecase (-- gone --), 2017-02-24 23:41:30]

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:509:
temp_html.flush()
On 2017/02/24 at 23:00:31, the real yoland wrote:
> nit: I think exit implies flush?

I think you're right. Done.

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/ut...
File build/android/pylib/utils/google_storage_helper.py (right):

https://codereview.chromium.org/2701473003/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/google_storage_helper.py:1: # Copyright 2017 The
Chromium Authors. All rights reserved.
On 2017/02/24 at 23:00:31, the real yoland wrote:
> hmm, why not using upload_to_google_storage.py?

Since its very specific. It requires you to provide sha1 hashes and I think is
built for a different purpose (syncing only changed files to/from Google
Storage)

[the real yoland, 2017-02-24 23:59:48]

lgtm

[mikecase (-- gone --), 2017-02-25 00:15:43]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-02-25 00:15:44]

The patchset sent to the CQ was uploaded after l-g-t-m from hzl@google.com
Link to the patchset: https://codereview.chromium.org/2701473003/#ps130001
(title: "Add failure screenshots and render test images to results detail.")

[commit-bot: I haz the power, 2017-02-25 00:16:17]

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, 2017-02-25 01:25:57]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1487981743985370,
"parent_rev": "a01e5c6de666b2f1f450f1429c1c0ad4c1405f66", "commit_rev":
"191a06d04a38e8b322c326d42433cbb1f5575025"}

[commit-bot: I haz the power, 2017-02-25 01:26:44]

Description was changed from

==========
Add failure screenshots and render test images to results detail.
==========

to

==========
Add failure screenshots and render test images to results detail.

Review-Url: https://codereview.chromium.org/2701473003
Cr-Commit-Position: refs/heads/master@{#453039}
Committed:
https://chromium.googlesource.com/chromium/src/+/191a06d04a38e8b322c326d42433...
==========

[commit-bot: I haz the power, 2017-02-25 01:26:46]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/191a06d04a38e8b322c326d42433...

[mikecase (-- gone --), 2017-03-08 23:29:44]

A revert of this CL (patchset #8 id:130001) has been created in
https://codereview.chromium.org/2737223002/ by mikecase@chromium.org.

The reason for reverting is: Reverting :( this since using find_depot_tools
doesnt work on some bots. Will have to find alternative and reland..