Add tests for makefile_writer.py.

Add tests for makefile_writer.py.

Many of the tests performed are comparison tests, where we call the
functions using dummy values and then compare to expectations (also
newly added).

Add a way to rebaseline the test for write_android_mk, which is likely
to change in the future.

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

[scroggo, 2014-03-25 21:42:48]

Finally, some tests, as requested.

[epoger, 2014-03-26 14:29:47]

Looks pretty good, thanks for adding it!  A few comments/questions.

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
File platform_tools/android/tests/makefile_writer_tests.py (right):

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:119: filename =
tempfile.mkstemp()[1]
Looking at http://docs.python.org/2/library/tempfile.html#tempfile.mkstemp , it
seems like you could just use tempfile.mkstemp()[0] as the filehandle rather
than creating a new one with open() below.

I don't know the pros and cons, but if http://www.logilab.org/17873 is to be
believed, you need to close the filehandle returned by tempfile.mkstemp()[0] .

(Although this test is a short-running process, so it should clean up after
itself anyway.)

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:177:
unittest.TextTestRunner(verbosity=2).run(suite)
I think you need to check for errors in TextTestRunner; otherwise, this process
will exit with returncode 0 even if there were failures.

See https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:179: if __name__ ==
'__main__':
Will this new test be run automatically by the housekeeper?

For example, https://github.com/google/skia/blob/master/gm/tests/run.sh and
https://github.com/google/skia/blob/master/platform_tools/android/tests/run_a...
are run by 
https://github.com/google/skia-buildbot/blob/master/master/skia_master_script...

[scroggo, 2014-03-26 16:24:00]

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
File platform_tools/android/tests/makefile_writer_tests.py (right):

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:119: filename =
tempfile.mkstemp()[1]
On 2014/03/26 14:29:48, epoger wrote:
> Looking at http://docs.python.org/2/library/tempfile.html#tempfile.mkstemp ,
it
> seems like you could just use tempfile.mkstemp()[0] as the filehandle rather
> than creating a new one with open() below.
> 
> I don't know the pros and cons, but if http://www.logilab.org/17873 is to be
> believed, you need to close the filehandle returned by tempfile.mkstemp()[0] .
> 
> (Although this test is a short-running process, so it should clean up after
> itself anyway.)

I'll trust your link ;). It sounds like my current code would be fine on linux
(where I'm testing this), but not on Windows. I support good habits either way!
Fixed.

Also, looking back over this made me realize that I did not remove the file
either. There's no need to leave all these temporary files around...

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:177:
unittest.TextTestRunner(verbosity=2).run(suite)
On 2014/03/26 14:29:48, epoger wrote:
> I think you need to check for errors in TextTestRunner; otherwise, this
process
> will exit with returncode 0 even if there were failures.
> 
> See
https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py

platform/android/tests/run_all.py does the check you suggest. The other tests in
this folder do not do the check. Should I add it for all them?

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:179: if __name__ ==
'__main__':
On 2014/03/26 14:29:48, epoger wrote:
> Will this new test be run automatically by the housekeeper?
> 
> For example, https://github.com/google/skia/blob/master/gm/tests/run.sh and
>
https://github.com/google/skia/blob/master/platform_tools/android/tests/run_a...
> are run by 
>
https://github.com/google/skia-buildbot/blob/master/master/skia_master_script...

Yes. It will be run by platform/android/tests/run_all.py. See
https://codereview.chromium.org/159793002

[epoger, 2014-03-26 17:22:46]

LGTM at patchset 3, whether or not you do something about this one remaining
issue

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
File platform_tools/android/tests/makefile_writer_tests.py (right):

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:177:
unittest.TextTestRunner(verbosity=2).run(suite)
On 2014/03/26 16:24:00, scroggo wrote:
> On 2014/03/26 14:29:48, epoger wrote:
> > I think you need to check for errors in TextTestRunner; otherwise, this
> process
> > will exit with returncode 0 even if there were failures.
> > 
> > See
> https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py
> 
> platform/android/tests/run_all.py does the check you suggest. The other tests
in
> this folder do not do the check. Should I add it for all them?

It depends on how each module will be used.

If a module will be executed as __main__ ("python
platform_tools/android/tests/makefile_writer_tests.py" in this case), and you
need it to exit with a nonzero return code on error (i.e. to turn a buildbot
red), yes, the check is needed.

If the module will instead be executed only within the context of a roll-up
Python test suite like
https://github.com/google/skia/blob/master/platform_tools/android/tests/run_a...
, there is no need for the check within this module, because run_all.py performs
that check (looking at the results of the entire test suite).  But in that case,
why bother providing the __main__ entry point?

It seems to me that, for each module, we should either (a) provide a __main__
entry point and return nonzero on error, or (b) not provide a __main__ entry
point at all.

[scroggo, 2014-03-26 19:18:11]

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
File platform_tools/android/tests/makefile_writer_tests.py (right):

https://codereview.chromium.org/205383006/diff/1/platform_tools/android/tests...
platform_tools/android/tests/makefile_writer_tests.py:177:
unittest.TextTestRunner(verbosity=2).run(suite)
On 2014/03/26 17:22:46, epoger wrote:
> On 2014/03/26 16:24:00, scroggo wrote:
> > On 2014/03/26 14:29:48, epoger wrote:
> > > I think you need to check for errors in TextTestRunner; otherwise, this
> > process
> > > will exit with returncode 0 even if there were failures.
> > > 
> > > See
> > https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py
> > 
> > platform/android/tests/run_all.py does the check you suggest. The other
tests
> in
> > this folder do not do the check. Should I add it for all them?
> 
> It depends on how each module will be used.
> 
> If a module will be executed as __main__ ("python
> platform_tools/android/tests/makefile_writer_tests.py" in this case), and you
> need it to exit with a nonzero return code on error (i.e. to turn a buildbot
> red), yes, the check is needed.
> 
> If the module will instead be executed only within the context of a roll-up
> Python test suite like
>
https://github.com/google/skia/blob/master/platform_tools/android/tests/run_a...
> , there is no need for the check within this module, because run_all.py
performs
> that check (looking at the results of the entire test suite).  But in that
case,
> why bother providing the __main__ entry point?
> 
> It seems to me that, for each module, we should either (a) provide a __main__
> entry point and return nonzero on error, or (b) not provide a __main__ entry
> point at all.

I find the __main__ to be convenient for testing purposes (and in this file for
rebaselining), but the intent is not for the bot to call this script directly.

Since I'm leaving in the __main__, I'll add the check for the results.

[scroggo, 2014-03-26 19:18:16]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-03-26 19:18:21]

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

[commit-bot: I haz the power, 2014-03-26 19:46:57]

Change committed as 13957