Chrome URL bugfix + added tests for chrome_app.apis.

Chrome URL bugfix + added tests for chrome_app.apis.

This CL fixes the bug where chrome.google.com URLs show up as APIs. This
is a very specific fix (e.g. chrome.mywebsite.com will still be picked
up as an API) but I think that hardcoding a list of APIs is a bad idea,
and chrome.google.com seems to show up a lot in real-world Chrome Apps.

This CL also adds tests for this bug; there were no existing API
module tests so I've added some of those too.

Resolves https://github.com/chromium/caterpillar/issues/28 .

[Matthew Alger, 2016-02-10 01:52:04]

alger@google.com changed reviewers:
+ mgiuca@chromium.org

[Matthew Alger, 2016-02-10 01:52:04]

There's a change to test data with Unicode filenames, but it only seems to have
affected the report_test.py changes (and a file is hidden, of course). The
unified diff for report_test.py is very simple and should suffice, though.

[Matthew Alger, 2016-02-10 01:52:21]

On 2016/02/10 01:52:04, Matthew Alger wrote:
> There's a change to test data with Unicode filenames, but it only seems to
have
> affected the report_test.py changes (and a file is hidden, of course). The
> unified diff for report_test.py is very simple and should suffice, though.

Specifically, this: https://codereview.chromium.org/1685693004/patch/20001/30003

[Matt Giuca, 2016-02-11 02:17:12]

https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis.py
File src/chrome_app/apis.py (right):

https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis.py#...
src/chrome_app/apis.py:59: (?!google) # chrome.google is not an API
Mmmmmm... I don't like this. As you said in the CL description, this is only
going to catch chrome.google.com, and not any other web domain with chrome in
it.

You also say "hardcoding a list of APIs is a bad idea". Why? The set of APIs is
fixed whereas the set of domains with "chrome" as a subdomain is limitless. So I
think we should hardcode the APIs. (But not in the regex; that would be a
nightmare. Instead, after you apply the regex, check to see if the API name
you've captured is in the list.)

Also: I don't think adding "(?!google)" to this regex is correct. It means
you're going to have potentially weird parses; if you encounter a domain like
"chrome.googlex.com" then you will return "x" as the API name.

https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis_tes...
File src/chrome_app/apis_test.py (right):

https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis_tes...
src/chrome_app/apis_test.py:20: from __future__ import print_function, division,
unicode_literals
This test file doesn't belong in a bugfix CL.

Can you just disable test_chrome_url test (to highlight that there is a bug) and
land the tests first, then fix the bug and un-disable the test at the same time?

[Matthew Alger, 2016-02-11 02:34:29]

On 2016/02/11 02:17:12, Matt Giuca wrote:
> https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis.py
> File src/chrome_app/apis.py (right):
> 
>
https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis.py#...
> src/chrome_app/apis.py:59: (?!google) # chrome.google is not an API
> Mmmmmm... I don't like this. As you said in the CL description, this is only
> going to catch http://chrome.google.com, and not any other web domain with
chrome in
> it.
> 
> You also say "hardcoding a list of APIs is a bad idea". Why? The set of APIs
is
> fixed whereas the set of domains with "chrome" as a subdomain is limitless. So
I
> think we should hardcode the APIs. (But not in the regex; that would be a
> nightmare. Instead, after you apply the regex, check to see if the API name
> you've captured is in the list.)
> 
> Also: I don't think adding "(?!google)" to this regex is correct. It means
> you're going to have potentially weird parses; if you encounter a domain like
> "chrome.googlex.com" then you will return "x" as the API name.
> 
>
https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis_tes...
> File src/chrome_app/apis_test.py (right):
> 
>
https://codereview.chromium.org/1685693004/diff/20001/src/chrome_app/apis_tes...
> src/chrome_app/apis_test.py:20: from __future__ import print_function,
division,
> unicode_literals
> This test file doesn't belong in a bugfix CL.
> 
> Can you just disable test_chrome_url test (to highlight that there is a bug)
and
> land the tests first, then fix the bug and un-disable the test at the same
time?

Will do all of these things in a new CL.
First CL: tests
Second CL: fix