[Android WebView] Prepare the copyrights scanner to run from presubmit scripts

[Android WebView] Prepare the copyrights scanner to run from presubmit scripts

Make the copyrights scanner to use InputApi instead of accessing system
modules directly.

Convert manual unit tests for the scanner into automatic that now run as
a presubmit check for related changes in android_webview/tools/*

BUG=343104
NOTRY=true

Committed: https://crrev.com/98bfa8802675cebd96be6688305b85a947f5ba2a
Cr-Commit-Position: refs/heads/master@{#300670}

[mnaganov (inactive), 2014-10-20 15:55:14]

mnaganov@chromium.org changed reviewers:
+ mkosiba@chromium.org

[mkosiba (inactive), 2014-10-21 15:41:22]

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
File android_webview/tools/copyright_scanner.py (right):

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner.py:64:
_GeneratedFilesDetector.python_multiline_string_double_re = \
wouldn't it be simpler to have these be instance methods and pass input_api to
the _GeneratedFilesDetector as an argument? I think you'd end up with at most
one instance per script invocation, right?

Also, I think at least theoretically you should keep the invariant that
input_api doesn't 'leak' outside of the callstack, right?

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner.py:177: input_api.os_path.join(root_dir,
file_name), 'rb')
is 'rb' intentional?

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
File android_webview/tools/copyright_scanner_unittest.py (right):

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:47: '// (c) 2014 Google
Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
maybe use something like
 Expectation = collections.namedtuple('Expectation', ['input', 'expected'])
to express this data
  test_data = [
    Expectation(
      '// (c) 2014 Google Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
      '2014 Google Inc.'),
    ...

or just a tuple:
  test_data = [
      ('// (c) 2014 Google Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
      '2014 Google Inc.'),
    ...

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:63: [['GENERATED FILE']],
if you had these as tuples you could use an array comprehension to remove the
duplication

test_data = ['ALL...\n', 'GEN..\n', ..]
test_data = [(data, 'GENERATED FILE') for data in test_data]
or
test_data = [Expectation(input, 'GENERATED FILE') for input in test_data]

[mnaganov (inactive), 2014-10-22 09:27:29]

Thanks for insightful comments, Marcin!

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
File android_webview/tools/copyright_scanner.py (right):

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner.py:64:
_GeneratedFilesDetector.python_multiline_string_double_re = \
On 2014/10/21 15:41:22, mkosiba wrote:
> wouldn't it be simpler to have these be instance methods and pass input_api to
> the _GeneratedFilesDetector as an argument? I think you'd end up with at most
> one instance per script invocation, right?
> 

Done.

But it's different for _CopyrightsScanner, as I spin up a new instance per
scanned file (as it has per-file state). But _CopyrightScanner as a class is
only used once. I could come up with a "factory" class that emits "scanners",
but I think this will add unnecessary complexity.

> Also, I think at least theoretically you should keep the invariant that
> input_api doesn't 'leak' outside of the callstack, right?

It never does. I don't store references to input_api itself.

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner.py:177: input_api.os_path.join(root_dir,
file_name), 'rb')
On 2014/10/21 15:41:22, mkosiba wrote:
> is 'rb' intentional?

Not sure :) Let's stick with 'r', as before.

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
File android_webview/tools/copyright_scanner_unittest.py (right):

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:47: '// (c) 2014 Google
Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
On 2014/10/21 15:41:22, mkosiba wrote:
> maybe use something like
>  Expectation = collections.namedtuple('Expectation', ['input', 'expected'])
> to express this data
>   test_data = [
>     Expectation(
>       '// (c) 2014 Google Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
>       '2014 Google Inc.'),
>     ...
> 
> or just a tuple:
>   test_data = [
>       ('// (c) 2014 Google Inc.\n//\n//  (a) One\n//\n//  (b) Two\n//\n',
>       '2014 Google Inc.'),
>     ...

I decided it's even simpler just to pass a single expected output to
ShouldMatchReferenceOutput. It's easy to add another test function in case when
some input has a different expected output.

https://codereview.chromium.org/667723002/diff/20001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:63: [['GENERATED FILE']],
On 2014/10/21 15:41:22, mkosiba wrote:
> if you had these as tuples you could use an array comprehension to remove the
> duplication
> 
> test_data = ['ALL...\n', 'GEN..\n', ..]
> test_data = [(data, 'GENERATED FILE') for data in test_data]
> or
> test_data = [Expectation(input, 'GENERATED FILE') for input in test_data]

See the previous comment.

[mkosiba (inactive), 2014-10-22 11:16:28]

lgtm

https://codereview.chromium.org/667723002/diff/40001/android_webview/tools/co...
File android_webview/tools/copyright_scanner_unittest.py (right):

https://codereview.chromium.org/667723002/diff/40001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:34: def
ReadTestData(ignore1, ignore2):
you could pull this into your for loop and drop the test_data_pos entirely

for data in test_data:
  self.input_api.ReadFile = lambda _1, _2: data
  actual_output...

[mnaganov (inactive), 2014-10-22 11:55:42]

https://codereview.chromium.org/667723002/diff/40001/android_webview/tools/co...
File android_webview/tools/copyright_scanner_unittest.py (right):

https://codereview.chromium.org/667723002/diff/40001/android_webview/tools/co...
android_webview/tools/copyright_scanner_unittest.py:34: def
ReadTestData(ignore1, ignore2):
On 2014/10/22 11:16:28, mkosiba wrote:
> you could pull this into your for loop and drop the test_data_pos entirely
> 
> for data in test_data:
>   self.input_api.ReadFile = lambda _1, _2: data
>   actual_output...

Yay lambdas! Done. Thank you very much!

[mnaganov (inactive), 2014-10-22 11:56:02]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-10-22 11:57:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667723002/60001

[commit-bot: I haz the power, 2014-10-22 12:01:39]

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

[commit-bot: I haz the power, 2014-10-22 12:01:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[mnaganov (inactive), 2014-10-22 12:47:23]

On 2014/10/22 12:01:39, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
>   win8_chromium_rel on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
>   win_chromium_compile_dbg on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

The failure on chromium_presubmit seems to be a KI: http://crbug.com/369207

Failures on win_* bots seem like a problem in the script that tries to calculate
compile targets.

So landing with NOTRY.

[mnaganov (inactive), 2014-10-22 12:47:45]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-10-22 12:48:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667723002/60001

[commit-bot: I haz the power, 2014-10-22 12:50:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-22 12:50:49]

Patchset 4 (id:??) landed as
https://crrev.com/98bfa8802675cebd96be6688305b85a947f5ba2a
Cr-Commit-Position: refs/heads/master@{#300670}