Add tests for makefile_writer.py.

platform_tools/android/tests/makefile_writer_tests.py

Index: platform_tools/android/tests/makefile_writer_tests.py
diff --git a/platform_tools/android/tests/makefile_writer_tests.py b/platform_tools/android/tests/makefile_writer_tests.py
new file mode 100644
index 0000000000000000000000000000000000000000..552e9dd38751f6b22c89609747721fae06ce9cbd
--- /dev/null
+++ b/platform_tools/android/tests/makefile_writer_tests.py
@@ -0,0 +1,190 @@
+#!/usr/bin/python
+
+# Copyright 2014 Google Inc.
+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""
+Test makefile_writer.py
+"""
+
+import argparse
+import os
+import shutil
+import sys
+import tempfile
+import test_variables
+import unittest
+
+sys.path.append(test_variables.GYP_GEN_DIR)
+
+import makefile_writer
+import vars_dict_lib
+
+EXPECTATIONS_DIR = os.path.join(os.path.dirname(__file__), 'expectations')
+MAKEFILE_NAME = 'Android.mk'
+
+def generate_dummy_vars_dict(name):
+  """Create a VarsDict and fill it with dummy entries.
+
+  Args:
+      name: string to be appended to each entry, if not None.
+
+  Returns:
+      A VarsDict with dummy entries.
+  """
+  vars_dict = vars_dict_lib.VarsDict()
+  for key in vars_dict.keys():
+    entry = key.lower()
+    if name:
+      entry += '_' + name
+    vars_dict[key].add(entry)
+  return vars_dict
+
+
+def generate_dummy_vars_dict_data(name, condition):
+  """Create a dummy VarsDictData.
+
+  Create a dummy VarsDictData, using the name for both the contained
+  VarsDict and the VarsDictData
+
+  Args:
+      name: name used by both the returned VarsDictData and its contained
+          VarsDict.
+      condition: condition used by the returned VarsDictData.
+
+  Returns:
+      A VarsDictData with dummy values, using the passed in info.
+  """
+  vars_dict = generate_dummy_vars_dict(name)
+
+  return makefile_writer.VarsDictData(vars_dict=vars_dict, name=name,
+                                      condition=condition)
+
+
+def generate_dummy_makefile(target_dir):
+  """Create a dummy makefile to demonstrate how it works.
+
+  Use dummy values unrelated to any gyp files. Its output should remain the
+  same unless/until makefile_writer.write_android_mk changes.
+
+  Args:
+      target_dir: directory in which to write the resulting Android.mk
+  """
+  common_vars_dict = generate_dummy_vars_dict(None)
+
+  deviation_params = [('foo', 'COND'), ('bar', None)]
+  deviations = [generate_dummy_vars_dict_data(name, condition)
+                for (name, condition) in deviation_params]
+
+  makefile_writer.write_android_mk(target_dir=target_dir,
+                                   common=common_vars_dict,
+                                   deviations_from_common=deviations)
+
+
+class MakefileWriterTest(unittest.TestCase):
+
+  def test_write_group_empty(self):
+    f = tempfile.TemporaryFile()
+    assert f.tell() == 0
+    for empty in (None, []):
+      for truth in (True, False):
+        makefile_writer.write_group(f, 'name', empty, truth)
+        self.assertEqual(f.tell(), 0)
+    f.close()
+
+  def __compare_files(self, actual_name, expectation_name, msg=None):
+    """Check that two files are identical.
+
+    Assert line by line that the files match.
+
+    Args:
+        actual_name: Full path to the test file.
+        expectation_name: Basename of the expectations file within which
+            to compare. The file is expected to be in
+            platform_tools/android/tests/expectations.
+        msg: Message to pass to assertEqual.
+    Raises:
+        AssertionError: If the files do not match.
+    """
+    with open(actual_name, 'r') as result:
+      with open(os.path.join(EXPECTATIONS_DIR,
+                             expectation_name)) as expectation:
+        for line in result:
+          self.assertEqual(line, expectation.readline(), msg)
+
+  def test_write_group(self):
+    animals = ('dog', 'cat', 'mouse', 'elephant')
+    filename = tempfile.mkstemp()[1]

[epoger, 2014/03/26 14:29:48]

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.)

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

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...

+    with open(filename, 'w') as f:
+      makefile_writer.write_group(f, 'animals', animals, False)
+    # Now confirm that it matches expectations
+    self.__compare_files(filename, 'animals.txt')
+
+    with open(filename, 'w') as f:
+      makefile_writer.write_group(f, 'animals_append', animals, True)
+    # Now confirm that it matches expectations
+    self.__compare_files(filename, 'animals_append.txt')
+
+  def test_write_local_vars(self):
+    vars_dict = generate_dummy_vars_dict(None)
+
+    # Call variations of write_local_vars.
+    for append in [ True, False ]:
+      for name in [ None, 'arm', 'foo' ]:
+        # Now write to a temporary file.
+        outfile = tempfile.mkstemp()[1]
+        with open(outfile, 'w') as f:
+          makefile_writer.write_local_vars(f, vars_dict, append, name)
+
+        # Compare to the expected file.
+        filename = 'write_local_vars'
+        if append:
+          filename += '_append'
+        else:
+          filename += '_no_append'
+        if name:
+          filename += '_' + name
+        else:
+          filename += '_no_name'
+        self.__compare_files(outfile, filename)
+
+        # KNOWN_TARGETS is always a part of the input VarsDict, but it should
+        # not be written to the resulting file.
+        # Note that this assumes none of our dummy entries is 'KNOWN_TARGETS'.
+        known_targets_name = 'KNOWN_TARGETS'
+        self.assertEqual(len(vars_dict[known_targets_name]), 1)
+
+        with open(outfile, 'r') as f:
+          self.assertNotIn(known_targets_name, f.read())
+
+  def test_write_android_mk(self):
+    outdir = tempfile.mkdtemp()
+    generate_dummy_makefile(outdir)
+
+    self.__compare_files(os.path.join(outdir, MAKEFILE_NAME), MAKEFILE_NAME,
+                         'If you\'ve modified makefile_writer.py, run ' +
+                         '"makefile_writer_tests.py --rebaseline" ' +
+                         'to rebaseline')
+
+    shutil.rmtree(outdir)
+
+
+def main():
+  loader = unittest.TestLoader()
+  suite = loader.loadTestsFromTestCase(MakefileWriterTest)
+  unittest.TextTestRunner(verbosity=2).run(suite)

[epoger, 2014/03/26 14:29:48]

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

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

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?

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

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:12]

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.

+
+if __name__ == '__main__':

[epoger, 2014/03/26 14:29:48]

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]

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

+  parser = argparse.ArgumentParser()
+  parser.add_argument('-r', '--rebaseline',
+                      help='Rebaseline expectation for Android.mk',
+                      action='store_true')
+  args = parser.parse_args()
+
+  if args.rebaseline:
+    generate_dummy_makefile(EXPECTATIONS_DIR)
+  else:
+    main()
+