Rewrite blink_gc_plugin test script in Python.

Rewrite blink_gc_plugin test script in Python.

.sh tests don't work on Windows.

BUG=486571
R=thakis@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/a0de0476cbb5f9f8317cc3d43b05300994dd1ae3

[dcheng, 2015-05-13 07:43:00]

dcheng@chromium.org changed reviewers:
+ thakis@chromium.org

[dcheng, 2015-05-13 07:43:00]

I haven't actually enabled this on Windows yet, because I haven't tested it
there.

[Nico, 2015-05-13 17:32:46]

what's with all the extra newlines in the test files? can you make it so that
the new script behaves like the old one?

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/cycle_ptrs.txt (right):

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/cycle_ptrs.txt:8: 
?

[Nico, 2015-05-13 17:43:59]

Thanks!

I'd suggest that you generally try to keep the behavior the same as it was in
the .sh and then change it in a follow-up. Each change should either change
behavior or be a refactoring that preserves behavior ('cause then it's easy to
see if a change was intentional after the fact); this change here is a bit of a
mix of both.

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/test.py (right):

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:13: def run_test(test_base_name, cmd):
add docstring that documents what return value means

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:25: if 'dump-graph' in cmd:
Why check this instead of checking for the existence of the json file like the
sh script does?

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:33: # 'bad' (e.g. it has a cycle).
Hope that the output returned contains the
s/Hope/something else/

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:38: expected = open('%s.txt' %
test_base_name).read()
Maybe this only needs an [:-1] to strip the newline?

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:44: open('%s.txt' % test_base_name,
'w').write(actual)
This overwrites the input, is that intentional?

[dcheng, 2015-05-13 20:59:34]

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/cycle_ptrs.txt (right):

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/cycle_ptrs.txt:8: 
On 2015/05/13 at 17:32:46, Nico wrote:
> ?

I don't know. I guess this is some difference between how bash slurps in output
with $() vs whatever Python is doing in check_output().

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/test.py (right):

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:13: def run_test(test_base_name, cmd):
On 2015/05/13 17:43:58, Nico wrote:
> add docstring that documents what return value means

done

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:25: if 'dump-graph' in cmd:
On 2015/05/13 17:43:58, Nico wrote:
> Why check this instead of checking for the existence of the json file like the
> sh script does?

I guess I feel weird about checking something and then using it later, due to
potential TOCTOU issues. I guess it doesn't matter here, but this does seem
vaguely nicer to me (because we could hypothetically differentiate between
'dump-graph was present but the json was not generated' and 'the results of
processing the json returned nothing'

I don't care strongly, so I just changed it to match the .sh.

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:33: # 'bad' (e.g. it has a cycle).
Hope that the output returned contains the
On 2015/05/13 17:43:58, Nico wrote:
> s/Hope/something else/

Done.

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:38: expected = open('%s.txt' %
test_base_name).read()
On 2015/05/13 at 17:43:58, Nico wrote:
> Maybe this only needs an [:-1] to strip the newline?

I just added rstrip() to hack around this for now. I think we should eventually
just rebaseline the expected results.

https://codereview.chromium.org/1135363003/diff/20001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:44: open('%s.txt' % test_base_name,
'w').write(actual)
On 2015/05/13 17:43:58, Nico wrote:
> This overwrites the input, is that intentional?

I wanted to experiment with something better than:
diff foo.txt*
mv foo.txt-actual foo.txt

But I'm not sure how I feel about the "tests automatically start passing if you
run them more than once" bit.

I've reverted it to the original behavior for now. I guess the best thing to do
would be to provide a --reset-results like Blink's layout test runner.

[Nico, 2015-05-13 21:53:11]

lgtm, thanks!

https://codereview.chromium.org/1135363003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/test.py (right):

https://codereview.chromium.org/1135363003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:112: plugin_path = sys.argv[2] if
len(sys.argv) > 2 else None
optional nit: The old script logged the path to compiler and plugin; I thought
that was kind of useful.

[commit-bot: I haz the power, 2015-05-14 00:16:10]

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

[dcheng, 2015-05-14 00:16:24]

Committed patchset #4 (id:60001) manually as
a0de0476cbb5f9f8317cc3d43b05300994dd1ae3 (presubmit successful).

[dcheng, 2015-05-14 05:25:37]

https://codereview.chromium.org/1135363003/diff/40001/tools/clang/blink_gc_pl...
File tools/clang/blink_gc_plugin/tests/test.py (right):

https://codereview.chromium.org/1135363003/diff/40001/tools/clang/blink_gc_pl...
tools/clang/blink_gc_plugin/tests/test.py:112: plugin_path = sys.argv[2] if
len(sys.argv) > 2 else None
On 2015/05/13 21:53:11, Nico wrote:
> optional nit: The old script logged the path to compiler and plugin; I thought
> that was kind of useful.

Done.