Add self tests for gyp_to_android.

Add self tests for gyp_to_android.

May be overkill when we get the canary up and running, but for now
this will alert us if gyp_to_android fails to create a makefile.

Depends on https://codereview.chromium.org/140503007

BUG=skia:1975
This CL has been merged into https://codereview.chromium.org/140503007 in that CL's patch set 9.

[epoger, 2014-01-21 19:44:06]

looking now

[epoger, 2014-01-21 20:00:51]

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
File platform_tools/android/bin/gyp_to_android.py (right):

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
platform_tools/android/bin/gyp_to_android.py:355: def main():
Please add docstring describing meaning of return value.

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
platform_tools/android/bin/gyp_to_android.py:416: return 0
How would main() ever return anything but 0?

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
File tools/tests/gyp_to_android_tests.py (right):

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:16: script_dir = os.path.dirname(__file__)
all caps for these constants

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:20:
sys.path.append(os.path.join('platform_tools', 'android', 'bin'))
It worries me that tools/ is running the unittests for platform_tools/ .

I think it would make more sense for platform_tools to get its own test_all.py
(copy it in from
https://code.google.com/p/skia/source/browse/trunk/gm/rebaseline_server/test_...
, which knows how to run all *_tests.py for you automatically, assuming that
they extend unittest.TestCase)

I would be OK with this rearrangement being a TODO...

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:24: def AssertFunctionAsserts(f):
Please make the self-test extend unittest.TestCase, like
https://code.google.com/p/skia/source/browse/trunk/gm/rebaseline_server/resul...

Among other things, it gives you
http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises ,
so you don't have to roll your own.

Again, I'm OK with this being a TODO.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:40: vars_dict = gyp_to_android.VarsDict()
Using unittest.TestCase and its assertRaises() will make this function much more
compact, and easier to read.

vars_dict = gyp_to_android.VarsDict()
with self.assertRaises(AssertionError):
  vars_dict['c'] = None

...etc.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:48: # ConvertDefines() should append all
'defines' to 'cflags' with '-D'.
When you convert this to unittest.TestCase, I think this is the start of a
separate test case.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:84: path_to_android_mk =
os.path.join(skia_trunk, 'Android.mk')
I think it's dangerous for unittests to write into the real checkout.  I think
it would be much better to use
http://docs.python.org/2/library/tempfile.html#tempfile.mkdtemp to create a
temporary dir, and make gyp_to_android write into there (if possible).

[scroggo, 2014-01-22 16:36:47]

Elliot, thank you for your detailed comments and helpful pointers, as usual.
Please take a look at patch set 4

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
File platform_tools/android/bin/gyp_to_android.py (right):

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
platform_tools/android/bin/gyp_to_android.py:355: def main():
On 2014/01/21 20:00:51, epoger wrote:
> Please add docstring describing meaning of return value.

Return value removed, but doc string added.

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
platform_tools/android/bin/gyp_to_android.py:416: return 0
On 2014/01/21 20:00:51, epoger wrote:
> How would main() ever return anything but 0?

It would not. Removed.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
File tools/tests/gyp_to_android_tests.py (right):

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:16: script_dir = os.path.dirname(__file__)
On 2014/01/21 20:00:51, epoger wrote:
> all caps for these constants

Done.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:20:
sys.path.append(os.path.join('platform_tools', 'android', 'bin'))
On 2014/01/21 20:00:51, epoger wrote:
> It worries me that tools/ is running the unittests for platform_tools/ .
> 
> I think it would make more sense for platform_tools to get its own test_all.py
> (copy it in from
>
https://code.google.com/p/skia/source/browse/trunk/gm/rebaseline_server/test_...
> , which knows how to run all *_tests.py for you automatically, assuming that
> they extend unittest.TestCase)
> 
> I would be OK with this rearrangement being a TODO...

I've added a TODO. I think it's fine to move it inside this CL, but I'll leave
it here until you're happy with it so it will be easier to see what has changed
from patch set to patch set.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:24: def AssertFunctionAsserts(f):
On 2014/01/21 20:00:51, epoger wrote:
> Please make the self-test extend unittest.TestCase, like
>
https://code.google.com/p/skia/source/browse/trunk/gm/rebaseline_server/resul...
> 
> Among other things, it gives you
> http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises
,
> so you don't have to roll your own.
> 
> Again, I'm OK with this being a TODO.

Done.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:40: vars_dict = gyp_to_android.VarsDict()
On 2014/01/21 20:00:51, epoger wrote:
> Using unittest.TestCase and its assertRaises() will make this function much
more
> compact, and easier to read.
> 
> vars_dict = gyp_to_android.VarsDict()
> with self.assertRaises(AssertionError):
>   vars_dict['c'] = None
> 
> ...etc.

Done.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:48: # ConvertDefines() should append all
'defines' to 'cflags' with '-D'.
On 2014/01/21 20:00:51, epoger wrote:
> When you convert this to unittest.TestCase, I think this is the start of a
> separate test case.

Done.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:84: path_to_android_mk =
os.path.join(skia_trunk, 'Android.mk')
On 2014/01/21 20:00:51, epoger wrote:
> I think it's dangerous for unittests to write into the real checkout.  I think
> it would be much better to use
> http://docs.python.org/2/library/tempfile.html#tempfile.mkdtemp to create a
> temporary dir, and make gyp_to_android write into there (if possible).

Done.

For what it's worth, gyp_to_android calls gyp in a way that gyp writes into
gyp/. I've tried changing it, but without changing gyp itself (which may be
possible), or using python to copy and modify the gyp files, it will write into
gyp/ regardless.

[scroggo, 2014-01-22 19:32:50]

Investigating a bug where the test has to be run from skia root.

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
File platform_tools/android/bin/gyp_to_android.py (right):

https://codereview.chromium.org/142173002/diff/60001/platform_tools/android/b...
platform_tools/android/bin/gyp_to_android.py:416: return 0
On 2014/01/22 16:36:48, scroggo wrote:
> On 2014/01/21 20:00:51, epoger wrote:
> > How would main() ever return anything but 0?
> 
> It would not. Removed.

D'oh! I thought I removed it. Removed in next patch.

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
File tools/tests/gyp_to_android_tests.py (right):

https://codereview.chromium.org/142173002/diff/60001/tools/tests/gyp_to_andro...
tools/tests/gyp_to_android_tests.py:84: path_to_android_mk =
os.path.join(skia_trunk, 'Android.mk')
On 2014/01/22 16:36:48, scroggo wrote:
> On 2014/01/21 20:00:51, epoger wrote:
> > I think it's dangerous for unittests to write into the real checkout.  I
think
> > it would be much better to use
> > http://docs.python.org/2/library/tempfile.html#tempfile.mkdtemp to create a
> > temporary dir, and make gyp_to_android write into there (if possible).
> 
> Done.
> 
> For what it's worth, gyp_to_android calls gyp in a way that gyp writes into
> gyp/. I've tried changing it, but without changing gyp itself (which may be
> possible), or using python to copy and modify the gyp files, it will write
into
> gyp/ regardless.

Actually it wasn't so hard. In patch set 5 (and beyond), I copy the gyp files to
a new folder, so the gypd files are created in that file and later removed.

[scroggo, 2014-01-22 20:05:09]

On 2014/01/22 19:32:50, scroggo wrote:
> Investigating a bug where the test has to be run from skia root.

Fixed in patch set 8.

[epoger, 2014-01-22 21:35:03]

Looking pretty good at patchset 8!  A few final suggestions.

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
File platform_tools/android/bin/android_framework_gyp.py (right):

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
platform_tools/android/bin/android_framework_gyp.py:15: SCRIPT_DIR =
os.path.join(os.getcwd(), os.path.dirname(__file__))
I think this would be clearer, and functionally equivalent:

SCRIPT_DIR = os.path.abspath(os.path.dirname(__file__))

See http://docs.python.org/2/library/os.path.html#os.path.abspath

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
File platform_tools/android/bin/gyp_to_android.py (right):

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
platform_tools/android/bin/gyp_to_android.py:382: tmp_folder =
tempfile.mkdtemp(dir=os.getcwd())
This will work, but maybe set dir=skia_dir so that it won't be dependent on CWD?
 (And then I think you can get rid of the call to os.chdir() above)

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
File tools/tests/gyp_to_android_tests.py (right):

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
tools/tests/gyp_to_android_tests.py:32: def setUp(self):
please add newline before first method in each class

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
tools/tests/gyp_to_android_tests.py:44:
self.assertTrue(len(self.__vars_dict['cflags']) == 0)
Your error messages will be better if you use self.assertEquals(), etc. instead
of always assertTrue().

[scroggo, 2014-01-22 23:33:08]

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
File platform_tools/android/bin/android_framework_gyp.py (right):

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
platform_tools/android/bin/android_framework_gyp.py:15: SCRIPT_DIR =
os.path.join(os.getcwd(), os.path.dirname(__file__))
On 2014/01/22 21:35:03, epoger wrote:
> I think this would be clearer, and functionally equivalent:
> 
> SCRIPT_DIR = os.path.abspath(os.path.dirname(__file__))
> 
> See http://docs.python.org/2/library/os.path.html#os.path.abspath

Done.

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
File platform_tools/android/bin/gyp_to_android.py (right):

https://codereview.chromium.org/142173002/diff/310001/platform_tools/android/...
platform_tools/android/bin/gyp_to_android.py:382: tmp_folder =
tempfile.mkdtemp(dir=os.getcwd())
On 2014/01/22 21:35:03, epoger wrote:
> This will work, but maybe set dir=skia_dir so that it won't be dependent on
CWD?
>  (And then I think you can get rid of the call to os.chdir() above)

Done.

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
File tools/tests/gyp_to_android_tests.py (right):

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
tools/tests/gyp_to_android_tests.py:32: def setUp(self):
On 2014/01/22 21:35:03, epoger wrote:
> please add newline before first method in each class

Done.

https://codereview.chromium.org/142173002/diff/310001/tools/tests/gyp_to_andr...
tools/tests/gyp_to_android_tests.py:44:
self.assertTrue(len(self.__vars_dict['cflags']) == 0)
On 2014/01/22 21:35:03, epoger wrote:
> Your error messages will be better if you use self.assertEquals(), etc.
instead
> of always assertTrue().

Done.

[epoger, 2014-01-23 02:41:41]

LGTM!