Add local results details pages.

Add local results details pages.

This will let people view test results in the same manner that
they are viewable on the bots. Instead of images/logs getting
uploaded to Google Storage, they are storage in
out/Debug/{suite}_{timestamp}/. A results details HTML page is
then generated that points towards these local results.

[mikecase (-- gone --), 2017-06-16 21:50:25]

Description was changed from

==========
Add --local-output arg which enables local results detail pages.

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

to

==========
Add local results details pages.

This will let people view test results in the same manner that
they are viewable on the bots. Instead of images/logs getting
uploaded to Google Storage, they are storage in
out/Debug/{suite}_{timestamp}/. A results details HTML page is
then generated that points towards these local results.
==========

[mikecase (-- gone --), 2017-06-16 21:50:25]

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

[mikecase (-- gone --), 2017-06-19 23:48:16]

This is actually pretty useful for things like looking at render test results
(we have no real way to view results outside of results detail). Would
appreciate if people could give a quick look

[jbudorick, 2017-06-20 14:12:55]

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
File build/android/pylib/base/test_run.py (right):

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
build/android/pylib/base/test_run.py:20: self._test_output_saver =
test_output_saver
Can you explain why this is in the test run rather than the environment?

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:416:
logmon, self._test_instance.should_save_logcat):
Does this option do anything after your change?

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:413:
with logcat_monitor.LogcatMonitor(
I'm wondering if there are situations in which we wouldn't want to save the
logcat.

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:550:
link = self._test_output_saver.Save(
This only saves the screenshot if we've set a results bucket. Should we be
saving it regardless & let _test_output_saver wrangle the bucket?

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/test_output_saver_factory.py:15: class
Datatype(object):
_TestOutputSaver, its subclasses, and this class should be in another module.
Let the factory handle creation and only creation.

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/test_output_saver_factory.py:34: class
_TestOutputSaver(object):
TestOutputDelegate?

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/test_output_saver_factory.py:60: timestamp =
time_helper.timestamp(local=True)
Get rid of time_helper and just write the timestamp here.

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/test_output_saver_factory.py:66: assert datatype in
(Datatype.TEXT, Datatype.IMAGE, Datatype.HTML)
Handle this w/ a regular exception rather than an assertion.

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/test_output_saver_factory.py:99: assert datatype in
(Datatype.TEXT, Datatype.IMAGE, Datatype.HTML)
same

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/time_helper.py:7: def timestamp(local=False):
As mentioned in the other file, remove this & do it inline.

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
build/android/pylib/utils/time_helper.py:14: '%Y-%m-%dT%H:%M:%SZ',
This is wrong if local=True -- the trailing Z indicates Zulu time, which is UTC:
https://en.wikipedia.org/w/index.php?title=Zulu_Time

https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
build/android/test_runner.py:864: if args.local_output:
We won't write local output on exception w/ this.

[agrieve, 2017-06-22 17:58:30]

Description was changed from

==========
Add local results details pages.

This will let people view test results in the same manner that
they are viewable on the bots. Instead of images/logs getting
uploaded to Google Storage, they are storage in
out/Debug/{suite}_{timestamp}/. A results details HTML page is
then generated that points towards these local results.
==========

to

==========
Add local results details pages.

This will let people view test results in the same manner that
they are viewable on the bots. Instead of images/logs getting
uploaded to Google Storage, they are storage in
out/Debug/{suite}_{timestamp}/. A results details HTML page is
then generated that points towards these local results.
==========

[agrieve, 2017-06-23 19:01:09]

On 2017/06/20 14:12:55, jbudorick wrote:
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
> File build/android/pylib/base/test_run.py (right):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
> build/android/pylib/base/test_run.py:20: self._test_output_saver =
> test_output_saver
> Can you explain why this is in the test run rather than the environment?
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
> File build/android/pylib/local/device/local_device_instrumentation_test_run.py
> (left):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:416:
> logmon, self._test_instance.should_save_logcat):
> Does this option do anything after your change?
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
> File build/android/pylib/local/device/local_device_instrumentation_test_run.py
> (right):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:413:
> with logcat_monitor.LogcatMonitor(
> I'm wondering if there are situations in which we wouldn't want to save the
> logcat.
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:550:
> link = self._test_output_saver.Save(
> This only saves the screenshot if we've set a results bucket. Should we be
> saving it regardless & let _test_output_saver wrangle the bucket?
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> File build/android/pylib/utils/test_output_saver_factory.py (right):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/test_output_saver_factory.py:15: class
> Datatype(object):
> _TestOutputSaver, its subclasses, and this class should be in another module.
> Let the factory handle creation and only creation.
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/test_output_saver_factory.py:34: class
> _TestOutputSaver(object):
> TestOutputDelegate?
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/test_output_saver_factory.py:60: timestamp =
> time_helper.timestamp(local=True)
> Get rid of time_helper and just write the timestamp here.
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/test_output_saver_factory.py:66: assert datatype in
> (Datatype.TEXT, Datatype.IMAGE, Datatype.HTML)
> Handle this w/ a regular exception rather than an assertion.
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/test_output_saver_factory.py:99: assert datatype in
> (Datatype.TEXT, Datatype.IMAGE, Datatype.HTML)
> same
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> File build/android/pylib/utils/time_helper.py (right):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/time_helper.py:7: def timestamp(local=False):
> As mentioned in the other file, remove this & do it inline.
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ut...
> build/android/pylib/utils/time_helper.py:14: '%Y-%m-%dT%H:%M:%SZ',
> This is wrong if local=True -- the trailing Z indicates Zulu time, which is
UTC:
> https://en.wikipedia.org/w/index.php?title=Zulu_Time
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
> build/android/test_runner.py:864: if args.local_output:
> We won't write local output on exception w/ this.


I was playing around with logcat deobfuscating yesterday & today, and took some
timings with & without logcat:

Running a batch of tests without logcat:
real	5m13.140s
user	0m5.648s
sys	0m7.840s

Running a batch of tests with logcat:
real	6m10.579s
user	0m6.668s
sys	0m9.636s

If you add in deobfuscation, it jumps to:
real	9m26.219s
user	0m9.896s
sys	0m29.576s

ouch!

I think the majority of the overhead is in starting & stopping logcat. Do you
think this could be refactored to have a single logcat capture for the entire
suite rather than starting & stopping for each test? Perhaps use the "START
$test", "END $test" logs to know how to chop up the large output for each test?

[jbudorick, 2017-06-23 19:05:50]

[snip]

> 
> I was playing around with logcat deobfuscating yesterday & today, and took
some
> timings with & without logcat:
> 
> Running a batch of tests without logcat:
> real	5m13.140s
> user	0m5.648s
> sys	0m7.840s
> 
> Running a batch of tests with logcat:
> real	6m10.579s
> user	0m6.668s
> sys	0m9.636s
> 
> If you add in deobfuscation, it jumps to:
> real	9m26.219s
> user	0m9.896s
> sys	0m29.576s

Deobfuscation adds in 3+ minutes?! ouch indeed :(

> 
> ouch!
> 
> I think the majority of the overhead is in starting & stopping logcat. Do you
> think this could be refactored to have a single logcat capture for the entire
> suite rather than starting & stopping for each test? Perhaps use the "START
> $test", "END $test" logs to know how to chop up the large output for each
test?

[agrieve, 2017-06-26 00:58:16]

On 2017/06/23 19:05:50, jbudorick wrote:
> [snip]
> 
> > 
> > I was playing around with logcat deobfuscating yesterday & today, and took
> some
> > timings with & without logcat:
> > 
> > Running a batch of tests without logcat:
> > real	5m13.140s
> > user	0m5.648s
> > sys	0m7.840s
> > 
> > Running a batch of tests with logcat:
> > real	6m10.579s
> > user	0m6.668s
> > sys	0m9.636s
> > 
> > If you add in deobfuscation, it jumps to:
> > real	9m26.219s
> > user	0m9.896s
> > sys	0m29.576s
> 
> Deobfuscation adds in 3+ minutes?! ouch indeed :(

My point was that logcat capturing on its own is an almost 20% slowdown (I'd
think this to be unacceptable). 
But yes, it seems that Retrace.jar hangs for several (5+) seconds before it even
starts to accept input. It's actually not much overhead for a long-running
logcat session however.
It also seems to buffer its input, so it's not currently possible to keep one
session around and feed it a line at a time (although I could probably fix this
by writing some .java that uses Retrace.jar as a library).

> 
> > 
> > ouch!
> > 
> > I think the majority of the overhead is in starting & stopping logcat. Do
you
> > think this could be refactored to have a single logcat capture for the
entire
> > suite rather than starting & stopping for each test? Perhaps use the "START
> > $test", "END $test" logs to know how to chop up the large output for each
> test?

[mikecase (-- gone --), 2017-07-10 17:11:11]

Reworked this CL. My sample test I run takes 47 seconds with --local-output
compared to 44 without. So minor slowdown, but people will hopefully save 3 or
more seconds viewing results with the local results presentation.

Basically how this is setup is....

with local_output_manager:
  try:
    f = tempfile.Tempfile(delete=False)
    f.write("Hello world")
  finally:
    // Archives async.
    env.output_manager.ArchiveAndDelete(f) 
// Exiting context manager, output_manager blocks until all uploads are
complete.


Also, since screenshots and render tests images will be handled by the new
--local-output option, Id probably delete args like --render-test-dir and
--screenshot-dir

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
File build/android/pylib/base/test_run.py (right):

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/ba...
build/android/pylib/base/test_run.py:20: self._test_output_saver =
test_output_saver
On 2017/06/20 at 14:12:54, jbudorick wrote:
> Can you explain why this is in the test run rather than the environment?

Well, I think test_runs are really the things saving test results. The output
you save isnt tied to whether you are running tests on your Workstation or on
Android devices, it depends on what test you are running.

Counterpoint, is that regardless of test_run you probably will always use the
same type of output_saver. Really doesnt matter where it lives. Moving to
environment since it might be simpler.

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:416:
logmon, self._test_instance.should_save_logcat):
On 2017/06/20 at 14:12:54, jbudorick wrote:
> Does this option do anything after your change?

Probably not. Would need to get rid of it I suppose

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

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:413:
with logcat_monitor.LogcatMonitor(
On 2017/06/20 at 14:12:54, jbudorick wrote:
> I'm wondering if there are situations in which we wouldn't want to save the
logcat.

Well, the default is the NoopOutputSaver, which doesnt archive the logcat. I
guess the logcat is always saved to a tmp file in my current impl (even if you
have the NoopOutputSaver)

https://codereview.chromium.org/2933993002/diff/110001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:550:
link = self._test_output_saver.Save(
On 2017/06/20 at 14:12:54, jbudorick wrote:
> This only saves the screenshot if we've set a results bucket. Should we be
saving it regardless & let _test_output_saver wrangle the bucket?

Thanks for this catch. I had fixed this for the render tests, but not here.

https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/110001/build/android/test_run...
build/android/test_runner.py:864: if args.local_output:
On 2017/06/20 at 14:12:55, jbudorick wrote:
> We won't write local output on exception w/ this.

Like, if the test_runner itself throws an exception? hmmm, maybe I should put
this in a finally block?

[mikecase (-- gone --), 2017-07-19 20:47:24]

ping, ptal

[jbudorick, 2017-07-19 22:45:09]

ArchiveAndDeleteFile really wants to be a context manager (again?)

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:30: """Context manager used to
archive file contents asynchonously.
This doesn't appear to be a context manager any more? Though looking at its
usage, it may be nice if it were.

The comment should mention that this function takes ownership of in_filename and
deletes it after archiving.

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:39:
output_manager.ArchiveAndDeleteFile()
This example doesn't pass any arguments to ArchiveAndDeleteFile?

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:41: Args:
in_filename?

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:46: Yields:
nit: Returns, not Yields

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:50: assert self._allow_upload, 'Must
run |SetUp| before attempting to upload!'
Do this w/ an explicit exception rather than an assertion.

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

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:412:
try:
It would be nice if we could make ArchiveAndDeleteFile work as a context
manager. Maybe something like

  with self._env.output_manager.ArchivedTemporaryFile(
      stream_name, 'logcat') as logcat_file:
    ...

though maybe with a better name. It's not super temporary if we're archiving it.

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:16: """Saves and manages test
output files locally in output directory.
Should this be a comment on __init__, the class, or the module? Seems more apt
on one of the latter two.

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:18: Location files will be
saved is {output_dir}/TEST_RESULTS_{timestamp}.
nit: saved in?

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
File build/android/pylib/output/remote_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:60: self._content_addressed
= False
nit: self._content_addressed = (self._datatype in (...))

?

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:68: sha1.update(f.read())
I'd be interested in how this performs. If it's computationally intensive, we
may have to consider moving it over into a separate process (a la zipping up in
DeviceUtils:
https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil...).

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/re...
File build/android/pylib/results/presentation/test_results_presentation.py
(right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/re...
build/android/pylib/results/presentation/test_results_presentation.py:311:
return html_render
I think this should return (html_render, None, None) for compatibility w/ the
other branch.

[mikecase (-- gone --), 2017-07-26 21:21:38]

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:30: """Context manager used to
archive file contents asynchonously.
On 2017/07/19 at 22:45:09, jbudorick wrote:
> This doesn't appear to be a context manager any more? Though looking at its
usage, it may be nice if it were.
> 
> The comment should mention that this function takes ownership of in_filename
and deletes it after archiving.


Done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:39:
output_manager.ArchiveAndDeleteFile()
On 2017/07/19 at 22:45:09, jbudorick wrote:
> This example doesn't pass any arguments to ArchiveAndDeleteFile?


Done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:41: Args:
On 2017/07/19 at 22:45:09, jbudorick wrote:
> in_filename?

Done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:46: Yields:
On 2017/07/19 at 22:45:09, jbudorick wrote:
> nit: Returns, not Yields

Yeah, I changed the implementation a bunch. Seems like I did a bad job of
updating docs.

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:50: assert self._allow_upload, 'Must
run |SetUp| before attempting to upload!'
On 2017/07/19 at 22:45:09, jbudorick wrote:
> Do this w/ an explicit exception rather than an assertion.

Done

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

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:412:
try:
On 2017/07/19 at 22:45:09, jbudorick wrote:
> It would be nice if we could make ArchiveAndDeleteFile work as a context
manager. Maybe something like
> 
>   with self._env.output_manager.ArchivedTemporaryFile(
>       stream_name, 'logcat') as logcat_file:
>     ...
> 
> though maybe with a better name. It's not super temporary if we're archiving
it.

This is what I wanted, unfortunatly it didnt work for content_addressed files
(unless I make stuff waaay more complex). I wish context manager could return
something on exit, but they cant.

Upload would start on exit of context manager. And the link to the uploaded file
would be known at that point. But there is no clean way to get it.

   with self._env.output_manager.ArchivedTemporaryFile(
       stream_name, 'logcat') as logcat_file, link_promise:
     # link_promise would yell strongly if you try to call str() on it before
exiting context manager.

That is the best I could think of ^

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:16: """Saves and manages test
output files locally in output directory.
On 2017/07/19 at 22:45:09, jbudorick wrote:
> Should this be a comment on __init__, the class, or the module? Seems more apt
on one of the latter two.

Done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:18: Location files will be
saved is {output_dir}/TEST_RESULTS_{timestamp}.
On 2017/07/19 at 22:45:09, jbudorick wrote:
> nit: saved in?

Done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
File build/android/pylib/output/remote_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:60: self._content_addressed
= False
On 2017/07/19 at 22:45:09, jbudorick wrote:
> nit: self._content_addressed = (self._datatype in (...))
> 
> ?

done

https://codereview.chromium.org/2933993002/diff/250001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:68: sha1.update(f.read())
On 2017/07/19 at 22:45:09, jbudorick wrote:
> I'd be interested in how this performs. If it's computationally intensive, we
may have to consider moving it over into a separate process (a la zipping up in
DeviceUtils:
https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil...).

ack

[mikecase (-- gone --), 2017-07-31 17:54:12]

cc hzl@

[mikecase (-- gone --), 2017-08-07 21:44:36]

ping

[jbudorick, 2017-08-10 16:27:38]

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:25: self._allow_upload = False
nit: maybe allow_save or allow_archive, since this doesn't necessarily involve
uploading?

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

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:411:
logcat_file = tempfile.NamedTemporaryFile(delete=False)
Should this be outside of the try/finally? If temporary file creation fails,
logcat_file won't be defined when we attempt to use it in the finally.

Alternatively, could this be done as a context manager?

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:23: self._output_root =
os.path.join(
would it be helpful to include the suite name here too?

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/re...
File build/android/pylib/results/json_results.py (right):

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/re...
build/android/pylib/results/json_results.py:140: json_result_file.flush()
Exiting the context manager here should flush the file. Is this necessary, or
perhaps left over from a previous iteration?

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

https://codereview.chromium.org/2933993002/diff/290001/build/android/pylib/ut...
build/android/pylib/utils/google_storage_helper.py:76: # TODO(mikecase): Delete
this function. Only one user of it.
You sure you want to sign up for this?

https://codereview.chromium.org/2933993002/diff/290001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/290001/build/android/test_run...
build/android/test_runner.py:154: '--local-output', action='store_true',
nit: action on its own line

https://codereview.chromium.org/2933993002/diff/290001/build/android/test_run...
build/android/test_runner.py:838: with out_manager:
Can this be part of the with line below?

[BigBossZhiling, 2017-08-15 22:48:27]

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

[BigBossZhiling, 2017-08-15 22:48:27]

Would you please also write a one or two sentence on how it can integrate with
the logcat_symbolizer?

[mikecase (-- gone --), 2017-08-15 23:11:44]

On 2017/08/15 at 22:48:27, hzl wrote:
> Would you please also write a one or two sentence on how it can integrate with
the logcat_symbolizer?

Hmmm, I wouldn't worry about it. I would just try to use your logcat_symbolizer
in current impl. Changing this file to use Context maanger. So, you would either
to something like...

logcat_file = 
with self._env.output_manager.ArchivedFile() as symbolized_logcat:
  logcat_symbolizer.symbolize(logcat_file, symbolized_logcat)

Or, you could theoretically add a pre-archive hook to the output_manager if you
wanted symbolization to be async. The hook would probably just take as input a
file and output a new processed file.

with self._env.output_manager.ArchivedFile(preupload_hook=lambda f:
logcat_symbolizer.symbolize(f)) as symbolized_logcat:
  logcat_file =

[mikecase (-- gone --), 2017-08-23 04:28:20]

Updated to use a slick context manager. Basically, now this works like....


with output_manager.ArchivedTempfile() as f:
  f.write('blargs!!!!')

print f.Link()  # This is where the file was archived to!


And that is it! Its a little more complex, but PTAL and free me of my Chromium
CL obligations! I'll double test everything tomorrow but at least
local_output__manager was working nicely and all the code is 99% updated and
double-checked.

https://codereview.chromium.org/2933993002/diff/290001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/290001/build/android/test_run...
build/android/test_runner.py:154: '--local-output', action='store_true',
On 2017/08/10 at 16:27:38, jbudorick wrote:
> nit: action on its own line

Done

https://codereview.chromium.org/2933993002/diff/330001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/330001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:31: def ArchivedTempfile(
This is the main function to look at.

[jbudorick, 2017-08-23 16:16:21]

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:54: f.PrepareArchive()
The remainder of this function won't execute if an Exception is raised inside
the context. I'm not sure that's what we want, so this should probably be:

  try:
    yield f
  finally:
    f.PrepareArchive()
    ...

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:85: 
super nit: +1 blank line

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:93: self._f =
tempfile.NamedTemporaryFile(delete=False)
Does this ever get removed?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:94: self.name = self._f.name
nit: @property for this?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:110: raise Exception('Cannot get link
to archived file before archiving'
super nit: +1 space after archiving, otherwise this will be

  "... before archivinghas begun"

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:140: assert self._ready_to_archive
I think this should be an Exception, analogous to what you have above, rather
than an Assertion. All are cases of the client calling a function at the wrong
time.

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

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:435:
with self._env.output_manager.ArchivedTempfile(
This is fantastic.

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:446:
logmon.Close()
I think this should be closed inside the logcat_file context.

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:721:
posixpath.join(golden_images_device_dir, failure_filename)):
nit: maybe save this as golden_image_device_file, since you use it a few lines
down too?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:733:
posixpath.join(diff_images_device_dir, failure_filename)):
nit: same

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:23: self._output_root =
os.path.join(
Should this be ensuring output_dir is an absolute path, or is that the
responsibility of something higher up?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/noop_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:9: # TODO(mikecase): This
class is currently mostly unused.
haha, maybe TODO someone else. (me?)

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:11: # --local-ouput args are
both not passed to test runner then use this
super nit: --local-output

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:14: # pylint:
disable=abstract-method
Is this because one of these doesn't override a NotImplementedError method?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:16: # pylint:
disable=super-init-not-called
Why doesn't NoopArchivedFile call super.__init__()?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/remote_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:79: if
self._content_addressed:
nit:

  if (self._content_addressed
      and google_storage_helper...):
    return

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

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ut...
build/android/pylib/utils/google_storage_helper.py:76: # TODO(mikecase): Delete
this function. Only one user of it.
Similarly, TODO someone else or TODO a bug.

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
build/android/test_runner.py:850: with out_manager:
This has to be in its own with statement and indentation level because we have
to exit json_writer before calling result_details?

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
build/android/test_runner.py:914: json_path=args.json_results_file,
What happens here if args.json_results_file is None?

[mikecase (-- gone --), 2017-08-24 05:29:08]

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:54: f.PrepareArchive()
On 2017/08/23 at 16:16:19, jbudorick wrote:
> The remainder of this function won't execute if an Exception is raised inside
the context. I'm not sure that's what we want, so this should probably be:
> 
>   try:
>     yield f
>   finally:
>     f.PrepareArchive()
>     ...

Done. GOod catch

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:85: 
On 2017/08/23 at 16:16:19, jbudorick wrote:
> super nit: +1 blank line

Done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:93: self._f =
tempfile.NamedTemporaryFile(delete=False)
On 2017/08/23 at 16:16:19, jbudorick wrote:
> Does this ever get removed?

Yes. so, I think Im doing it an alright way.

os.remove(f.name) is called by the thread spawned by ArchivedTempfile.

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:94: self.name = self._f.name
On 2017/08/23 at 16:16:19, jbudorick wrote:
> nit: @property for this?

Good idea, for some reason I forgot about those....

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:110: raise Exception('Cannot get link
to archived file before archiving'
On 2017/08/23 at 16:16:19, jbudorick wrote:
> super nit: +1 space after archiving, otherwise this will be
> 
>   "... before archivinghas begun"

Done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:140: assert self._ready_to_archive
On 2017/08/23 at 16:16:20, jbudorick wrote:
> I think this should be an Exception, analogous to what you have above, rather
than an Assertion. All are cases of the client calling a function at the wrong
time.

Well, this isnt well documented, but the only thing that should call Archive is
the ArchivedTempfile context manager. This shouldnt have to be altered by
anything/anyone. But changing to exception.

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

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:435:
with self._env.output_manager.ArchivedTempfile(
On 2017/08/23 at 16:16:20, jbudorick wrote:
> This is fantastic.

^.^

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:446:
logmon.Close()
On 2017/08/23 at 16:16:20, jbudorick wrote:
> I think this should be closed inside the logcat_file context.

Done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:721:
posixpath.join(golden_images_device_dir, failure_filename)):
On 2017/08/23 at 16:16:20, jbudorick wrote:
> nit: maybe save this as golden_image_device_file, since you use it a few lines
down too?

Done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:733:
posixpath.join(diff_images_device_dir, failure_filename)):
On 2017/08/23 at 16:16:20, jbudorick wrote:
> nit: same

Done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:23: self._output_root =
os.path.join(
On 2017/08/23 at 16:16:20, jbudorick wrote:
> Should this be ensuring output_dir is an absolute path, or is that the
responsibility of something higher up?
Does it need to be an absolute path?

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/noop_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:9: # TODO(mikecase): This
class is currently mostly unused.
On 2017/08/23 at 16:16:20, jbudorick wrote:
> haha, maybe TODO someone else. (me?)

done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:11: # --local-ouput args are
both not passed to test runner then use this
On 2017/08/23 at 16:16:20, jbudorick wrote:
> super nit: --local-output

done

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:16: # pylint:
disable=super-init-not-called
On 2017/08/23 at 16:16:20, jbudorick wrote:
> Why doesn't NoopArchivedFile call super.__init__()?

Because it creates a tempfile. Basically, I hate supressing these pylint things,
but I just want noop_output_manager to do actual nothing. Just return an object
that lets you call the ARchiveFile / OutputManager APIs without breaking.

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/remote_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/remote_output_manager.py:79: if
self._content_addressed:
On 2017/08/23 at 16:16:20, jbudorick wrote:
> nit:
> 
>   if (self._content_addressed
>       and google_storage_helper...):
>     return

Done

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

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ut...
build/android/pylib/utils/google_storage_helper.py:76: # TODO(mikecase): Delete
this function. Only one user of it.
On 2017/08/23 at 16:16:20, jbudorick wrote:
> Similarly, TODO someone else or TODO a bug.

You asked for it!

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
build/android/test_runner.py:850: with out_manager:
On 2017/08/23 at 16:16:20, jbudorick wrote:
> This has to be in its own with statement and indentation level because we have
to exit json_writer before calling result_details?

yes, pretty sure that was it. One of the things has to be exited, pretty sure
that one. Wish it wasnt so.

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
build/android/test_runner.py:914: json_path=args.json_results_file,
On 2017/08/23 at 16:16:20, jbudorick wrote:
> What happens here if args.json_results_file is None?

the if statement above is not entered...

if args.local_output and args.json_results_file

Oh yeah, thats another thing that should be improved. Someone needs to add
--json-results-file AND --local-output.
Should really just create a tempfile for json_results_file if local_output is
specified and it is not. TODO

[jbudorick, 2017-08-24 13:18:45]

lgtm w/ nits

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
File build/android/pylib/base/output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ba...
build/android/pylib/base/output_manager.py:93: self._f =
tempfile.NamedTemporaryFile(delete=False)
On 2017/08/24 05:29:07, mikecase (-- gone --) wrote:
> On 2017/08/23 at 16:16:19, jbudorick wrote:
> > Does this ever get removed?
> 
> Yes. so, I think Im doing it an alright way.
> 
> os.remove(f.name) is called by the thread spawned by ArchivedTempfile. 

ah, ok.

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
File build/android/pylib/output/local_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/pylib/ou...
build/android/pylib/output/local_output_manager.py:23: self._output_root =
os.path.join(
On 2017/08/24 05:29:07, mikecase (-- gone --) wrote:
> On 2017/08/23 at 16:16:20, jbudorick wrote:
> > Should this be ensuring output_dir is an absolute path, or is that the
> responsibility of something higher up?
> Does it need to be an absolute path?

Yeah, I don't think the file URL will work with a relative URL.

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2933993002/diff/370001/build/android/test_run...
build/android/test_runner.py:914: json_path=args.json_results_file,
On 2017/08/24 05:29:08, mikecase (-- gone --) wrote:
> On 2017/08/23 at 16:16:20, jbudorick wrote:
> > What happens here if args.json_results_file is None?
> 
> the if statement above is not entered...

derp, missed that entirely.

> 
> if args.local_output and args.json_results_file
> 
> Oh yeah, thats another thing that should be improved. Someone needs to add
> --json-results-file AND --local-output.
> Should really just create a tempfile for json_results_file if local_output is
> specified and it is not. TODO

sgtm

https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
File build/android/pylib/output/noop_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:27: yield NoopArchivedFile()
super nit: 2-space indent, not 4

[mikecase (-- gone --), 2017-08-25 01:40:44]

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

[commit-bot: I haz the power, 2017-08-25 01:40:58]

Dry run: CQ is trying da patch.

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

[mikecase (-- gone --), 2017-08-25 01:41:23]

Probably going to commit this 2nite if CQ goes well.

https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
File build/android/pylib/output/noop_output_manager.py (right):

https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
build/android/pylib/output/noop_output_manager.py:27: yield NoopArchivedFile()
On 2017/08/24 at 13:18:45, jbudorick wrote:
> super nit: 2-space indent, not 4

Done

[commit-bot: I haz the power, 2017-08-25 01:43:26]

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

[commit-bot: I haz the power, 2017-08-25 01:43:27]

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

[jbudorick, 2017-08-25 02:01:24]

On 2017/08/25 01:41:23, mikecase (-- gone --) wrote:
> Probably going to commit this 2nite if CQ goes well.

sgtm

> 
>
https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
> File build/android/pylib/output/noop_output_manager.py (right):
> 
>
https://codereview.chromium.org/2933993002/diff/390001/build/android/pylib/ou...
> build/android/pylib/output/noop_output_manager.py:27: yield NoopArchivedFile()
> On 2017/08/24 at 13:18:45, jbudorick wrote:
> > super nit: 2-space indent, not 4
> 
> Done