[hawkmoth] Re: [PATCH 2/4] test: transition parser tests to dynamically generated test methods

  • From: Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx>
  • To: Jani Nikula <jani@xxxxxxxxxx>
  • Date: Sun, 27 Jan 2019 00:40:17 +0100

I think you're needlessly making commit titles really long ;)

"test: dynamically generate parser tests"

On 17:34:18 2019-01-26, Jani Nikula wrote:

It's impossible to have expected failures or other unittest decorators
at subtest granularity. They only work at the test method level. On the
other hand we don't want to be manually adding test methods when all of
the tests are defined in terms of input files and expected results.

Generate test methods dynamically from the input files, and assign to
the test class. Running code at import time to do this is less than
stellar, but it needs to be done early to have unittest test discovery
find the methods.

The alternative would be to add a load_tests protocol function [1], but
that seems like more boilerplate. Can be added later as needed.

Finally, one massive upside to this is the ability to run individual
named testcases. For example, to test enum.c and typedef-enum.c, use:

$ test/test_hawkmoth.py ParserTest.test_enum ParserTest.test_typedef_enum

[1] https://docs.python.org/3/library/unittest.html#load-tests-protocol

I'm still not as familiar with this unittest thing as I should, but it
looks OK to me. I do have one comment below.

See my comments on the next patch before pushing this into master
though.

---
 test/test_hawkmoth.py | 26 +++++++-------------------
 test/testenv.py       | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/test/test_hawkmoth.py b/test/test_hawkmoth.py
index 1fe02efc004d..75eebbe35eef 100755
--- a/test/test_hawkmoth.py
+++ b/test/test_hawkmoth.py
@@ -8,28 +8,16 @@ import unittest
 import testenv
 from hawkmoth import hawkmoth
 
-class ParserTest(unittest.TestCase):
-    def _run_test(self, input_filename):
-        # sanity check
-        self.assertTrue(os.path.isfile(input_filename))
-
-        options = testenv.get_testcase_options(input_filename)
-        output = hawkmoth.parse_to_string(input_filename, False, **options)
-        expected = testenv.read_file(input_filename, ext='stdout')
+def _get_output(input_filename, **options):
+    return hawkmoth.parse_to_string(input_filename, False, **options)
 
-        self.assertEqual(expected, output)
+def _get_expected(input_filename, **options):
+    return testenv.read_file(input_filename, ext='stdout')
 
-    def _run_dir(self, path):
-        # sanity check
-        self.assertTrue(os.path.isdir(path))
-
-        with self.subTest(path=path):
-            for f in testenv.get_testcases(path):
-                with self.subTest(source=os.path.basename(f)):
-                    self._run_test(f)
+class ParserTest(unittest.TestCase):
+    pass
 
-    def test_parser(self):
-        self._run_dir(testenv.testdir)
+testenv.assign_test_methods(ParserTest, _get_output, _get_expected)
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/test/testenv.py b/test/testenv.py
index f026aead8c07..9ffcc4fbd8c1 100644
--- a/test/testenv.py
+++ b/test/testenv.py
@@ -3,6 +3,7 @@
 
 import sys
 import os
+import unittest
 
 testext = '.c'
 testdir = os.path.dirname(os.path.abspath(__file__))
@@ -10,6 +11,16 @@ rootdir = os.path.dirname(testdir)
 
 sys.path.insert(0, rootdir)
 
+def _testcase_name(testcase):
+    """Convert a testcase filename into a test case identifier."""
+    name = os.path.splitext(os.path.basename(testcase))[0]
+    name = name.replace('-', '_')
+    name = 'test_{name}'.format(name=name)
+
+    assert name.isidentifier()
+
+    return name
+
 def get_testcases(path):
     for f in sorted(os.listdir(path)):
         if f.endswith(testext):
@@ -52,3 +63,24 @@ def read_file(filename, **kwargs):
         expected = file.read()
 
     return expected
+
+def _test_generator(get_output, get_expected, input_filename, **options):
+    """Return a function that compares output/expected results on 
input_filename."""
+    def test(self):
+        # sanity check
+        self.assertTrue(os.path.isfile(input_filename))

I know this was the old behaviour as well, but I'd rather drop this.

1. This is not the subject of our tests so using the unittest assertion
   feels out of place. Raising an exception would probably be more
   appropriate, although I don't know if that makes any difference in
   the end result.

2. It's a redundant and useless check anyway since the names are
   obtained with os.listdir(path) and this doesn't guarantee the files
   will still be there once we try to open them in get_output or
   get_expected, which, by the way, don't even open the same file.

Cheers,
Bruno

+
+        output = get_output(input_filename, **options)
+        expected = get_expected(input_filename, **options)
+
+        self.assertEqual(expected, output)
+
+    return test
+
+def assign_test_methods(cls, get_output, get_expected):
+    """Assign test case functions to the given class."""
+    for f in get_testcases(testdir):
+        options = get_testcase_options(f)
+        method = _test_generator(get_output, get_expected, f, **options)
+
+        setattr(cls, _testcase_name(f), method)

Other related posts: