Chromium Codereview Comparison Script.

Chromium Codereview Comparison Script.

This script takes two Codereview URLs, looks at the trybot results for
the two codereviews and compares the results.

Motivation:  This should speed up the DEPS roll procedure.

BUG=skia:2039
Committed: http://code.google.com/p/skia/source/detail?r=13144

[hal.canary, 2014-01-21 19:37:41]

ptal

[DONE] Still need to suppress configs where roll is green
[DONE] Need to burrow in to get the sub-errors for telemetry_unittests &
content_browsertests
[DONE] Need to ignore "unexpected flaky" lines

[robertphillips, 2014-01-21 20:23:02]

lgtm but really need to add a python person (maybe Eric?)

[hal.canary, 2014-01-21 21:31:43]

Eric, can you comment on the Pythoness of this?  For background, we expect
Robert to execute this every morning to generate a report for him to use to
decide when to roll DEPS.

[borenet, 2014-01-21 21:55:42]

Looks fine with some nits.  I don't particularly like HTMLParser; its flow makes
for hard-to-read code IMO, where a regex is simpler to write and to read in a
lot of cases.  If you leave it as-is, please add "explain it like I'm
five"-level comments.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py
File tools/compare_codereview.py (right):

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:32: """
I'd prefer that you use the following docstring format:

"""Short description.

Args:
    somearg: type; description.

Returns:
    type; description.
"""


... but this isn't a big deal.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:53: def handle_starttag(self, tag, attrs):
I'd appreciate a docstring here and elsewhere, even though these are documented
in the official documentation.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:78: self._anchor_data += data
I find this flow hard to follow; I think I'd rather have extra variables called
"_currently_parsing_trybotdiv" or something like that instead of using the
variables you're modifying, or alternatively just add lots of explanation.

For the record, I've found that a simple regex is usually easier than
HTMLParser.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:185: def main(control_url, roll_url, verbosity):
Optional: you could add a default verbosity level here.

[hal.canary, 2014-01-22 15:25:32]

ptal

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py
File tools/compare_codereview.py (right):

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:32: """
On 2014/01/21 21:55:43, borenet wrote:
> I'd prefer that you use the following docstring format:
> 
> """Short description.
> 
> Args:
>     somearg: type; description.
> 
> Returns:
>     type; description.
> """
> 
> 
> ... but this isn't a big deal.

Done.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:53: def handle_starttag(self, tag, attrs):
On 2014/01/21 21:55:43, borenet wrote:
> I'd appreciate a docstring here and elsewhere, even though these are
documented
> in the official documentation.

I'll just note that I'm overriding a method and copy some text from the
specification.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:78: self._anchor_data += data
On 2014/01/21 21:55:43, borenet wrote:
> I find this flow hard to follow; I think I'd rather have extra variables
called
> "_currently_parsing_trybotdiv" or something like that instead of using the
> variables you're modifying, or alternatively just add lots of explanation.
> 
> For the record, I've found that a simple regex is usually easier than
> HTMLParser.

Done.

https://codereview.chromium.org/143503003/diff/1/tools/compare_codereview.py#...
tools/compare_codereview.py:185: def main(control_url, roll_url, verbosity):
On 2014/01/21 21:55:43, borenet wrote:
> Optional: you could add a default verbosity level here.

Done.

[borenet, 2014-01-22 21:31:26]

Thanks for the changes. LGTM.

[commit-bot: I haz the power, 2014-01-22 22:45:11]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/143503003/90001

[commit-bot: I haz the power, 2014-01-22 22:57:22]

Change committed as 13144