tools/android/loading: Implements loading_trace_analyzer.py's unittest.

tools/android/loading: Implements loading_trace_analyzer.py's unittest.

BUG=582080
Committed: https://crrev.com/77bd4a8ce7c24b6c8d8ddbc22130f351d6c01b95
Cr-Commit-Position: refs/heads/master@{#378531}

[gabadie, 2016-02-25 18:33:46]

gabadie@chromium.org changed reviewers:
+ lizeb@chromium.org, mattcary@chromium.org, pasko@chromium.org

[gabadie, 2016-02-25 18:33:47]

Hi guys,

Found that you guys have checked-in a loading trace, So I wrote an unittest for
loading_trace_analyzer.py using it.

PTAL. Thanks! =)

[mattcary, 2016-02-26 09:42:29]

lgtm

Thanks!

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_trace_analyzer_unittest.py (right):

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace_analyzer_unittest.py:15:
LOADING_TRACE_ANALYZER = os.path.join(LOADING_DIR, 'loading_trace_analyzer.py')
Generally it would be better to call loading_trace_analyzer._RequestsSubcommand
directly rather than redirecting through a process. Using the programmatic API
is more maintainable long-term than running a command line. (that is, doing
something like: args=object(); args.where_statement='zzz';
_RequestsSubcommand(args) )
 

Similarly, it would be best to refactor _RequestsSubcommand so that it returns
the output rather than writing it to a file which the test.

But that's a little involved and it's btter to have some unittest for this than
none at all!

[pasko, 2016-02-26 16:50:53]

lgtm, and I agree with Matt that using python functions in the test are better
than calling a script. Please put a TODO(gabadie): to provide a python interface
for  loading_trace_analyzer and convert testing to it.

In general it makes sense to also test the command-line interface, but this time
it seems the commandline stuff is not complicated.

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_trace_analyzer_unittest.py (right):

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace_analyzer_unittest.py:43: def
test_RequestsCmd(self):
It seems that naming like testRequestsCmd (without underscore) is more commonly
used in chromium (and non-blink) sources. Please change.

[gabadie, 2016-02-29 18:58:45]

Thanks! Waiting for dependencies to land.

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_trace_analyzer_unittest.py (right):

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace_analyzer_unittest.py:15:
LOADING_TRACE_ANALYZER = os.path.join(LOADING_DIR, 'loading_trace_analyzer.py')
On 2016/02/26 09:42:29, mattcary wrote:
> Generally it would be better to call
loading_trace_analyzer._RequestsSubcommand
> directly rather than redirecting through a process. Using the programmatic API
> is more maintainable long-term than running a command line. (that is, doing
> something like: args=object(); args.where_statement='zzz';
> _RequestsSubcommand(args) )
>  
> 
> Similarly, it would be best to refactor _RequestsSubcommand so that it returns
> the output rather than writing it to a file which the test.
> 
> But that's a little involved and it's btter to have some unittest for this
than
> none at all!

Done. But I am a not fan of this. It has just moved some code out of the
function and increased the uncovered tests.

https://codereview.chromium.org/1738803003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace_analyzer_unittest.py:43: def
test_RequestsCmd(self):
On 2016/02/26 16:50:53, pasko wrote:
> It seems that naming like testRequestsCmd (without underscore) is more
commonly
> used in chromium (and non-blink) sources. Please change.

Done.

[mattcary, 2016-03-01 10:05:16]

> 
> Done. But I am a not fan of this. It has just moved some code out of the
> function and increased the uncovered tests.
> 
Understood. It does slightly reduce raw code coverage, but it greatly increases
maintainability and makes it much, much easier to extend tests as things get
more complicated. It may not ever matter much in this case as this is a small
example, but it is important to be consistent in how tests are structured.
Calling through a process is always oddball, takes more time to grok, and
prevents many different kinds of refactorings/maintenance/improvements.

[pasko, 2016-03-01 15:34:02]

still lgtm, thank you for updates

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_trace_analyzer.py (right):

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
tools/android/loading/loading_trace_analyzer.py:47: output_format: Output format
of the yield strings.
nit: s/yield/generated/

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_trace_analyzer_unittest.py (right):

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
tools/android/loading/loading_trace_analyzer_unittest.py:41:
self.assertNotEqual(0, len(lines))
nit: maybe an 'else' statement for the for-loop would be more readable than list
comprehensions in this example

up to you

[gabadie, 2016-03-01 17:53:08]

Thanks for your reviews! Commiting once the dependencies are landed.

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_trace_analyzer.py (right):

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
tools/android/loading/loading_trace_analyzer.py:47: output_format: Output format
of the yield strings.
On 2016/03/01 15:34:02, pasko wrote:
> nit: s/yield/generated/

Done.

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
File tools/android/loading/loading_trace_analyzer_unittest.py (right):

https://codereview.chromium.org/1738803003/diff/20001/tools/android/loading/l...
tools/android/loading/loading_trace_analyzer_unittest.py:41:
self.assertNotEqual(0, len(lines))
On 2016/03/01 15:34:02, pasko wrote:
> nit: maybe an 'else' statement for the for-loop would be more readable than
list
> comprehensions in this example
> 
> up to you

As discussed offline, doesn't apply.

[gabadie, 2016-03-01 18:59:27]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-03-01 18:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org, pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1738803003/#ps40001
(title: "Addresses pasko's nit in the ListRequests's doc string")

[commit-bot: I haz the power, 2016-03-01 19:00:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738803003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738803003/40001

[commit-bot: I haz the power, 2016-03-01 19:44:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 19:46:01]

Description was changed from

==========
tools/android/loading: Implements loading_trace_analyzer.py's unittest.

BUG=582080
==========

to

==========
tools/android/loading: Implements loading_trace_analyzer.py's unittest.

BUG=582080

Committed: https://crrev.com/77bd4a8ce7c24b6c8d8ddbc22130f351d6c01b95
Cr-Commit-Position: refs/heads/master@{#378531}
==========

[commit-bot: I haz the power, 2016-03-01 19:46:02]

Patchset 3 (id:??) landed as
https://crrev.com/77bd4a8ce7c24b6c8d8ddbc22130f351d6c01b95
Cr-Commit-Position: refs/heads/master@{#378531}