[hawkmoth] Re: [PATCH 3/4] test: transition directive tests to dynamically generated test methods

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

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

Similar to the parser test transition to dynamically generated test

generate*

methods, but with more test separation added. After this, the Sphinx
build is done independently for each test case, and separately for the
actual directive output and the expected output. This removes any
potential C domain interactions between test cases and expected/actual
inputs.

The change does mean the Sphinx build is run roughly 50x times per full
test run, at the current number of test cases. This is somewhat offset
by the ability to run individual directive test cases:

$ test/test_cautodoc.py DirectiveTest.test_example_10_macro

Sounds reasonable to me. It would be nice to skip the extension test if
the parser test failed though as a 2nd failure simply duplicates the
output. It would be a separate improvement though.

However, this relates to something that has been bugging me about these
tests: we could remove test_hawkmoth with no loss of coverage. Frankly,
I think unit tests are useful in some circumstances, but largely
overrated.

I've been working in DO-178 software and in all the unit testing we've
done we haven't found a single bug... Integration is where the big bugs
hide.

My suggestion:

1. Ditch the current parser tests as redundant. If our unit is the whole
   parser or the whole parser with a very shallow Sphinx extension on
   top, might as well test the latter only.
   
2. Add a whole class of integration tests that are actually missing:
   test warning and error propagation. I.e. checking Sphinx's stdout.
   
   This would be relatively useless at the moment, but it would be great
   with warnings about undocumented parameters or against misuse
   (documentation of instanced and named typedefs comes to mind), etc.
   We can dream big here.

Point 2 is clearly a future, separate thing. But what do you think of
point 1? Is there some use case for the other tests I'm missing?


A little bit of extra temp file boilerplate is required to work around a
sphinx-testing issue [1]. Otherwise, the decorated functions could
return the values directly.

[1] https://github.com/sphinx-doc/sphinx-testing/issues/12

Code is OK, but I have some comments below again.

---
 test/test_cautodoc.py | 93 ++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/test/test_cautodoc.py b/test/test_cautodoc.py
index 3fb98af8629d..adbd17c8453e 100755
--- a/test/test_cautodoc.py
+++ b/test/test_cautodoc.py
@@ -4,59 +4,60 @@
 
 import os
 import shutil
+import tempfile
 import unittest
 
 import testenv
 from sphinx_testing import with_app
 
-class DirectiveTest(unittest.TestCase):
-
-    def _setup_src(self, srcdir, testcase_in):
-        testcase_out = testenv.modify_filename(testcase_in, dir=srcdir)
-
-        # use the pre-generated rst as comparison data
-        shutil.copyfile(testenv.modify_filename(testcase_in, ext='stdout'),
-                        testenv.modify_filename(testcase_out, 
ext='expected.rst'))
-
-        # set up an rst file to run the extension
-        shutil.copyfile(testcase_in, testcase_out)
-        options = testenv.get_testcase_options(testcase_in)
-
-        with open(testenv.modify_filename(testcase_out, ext='output.rst'), 
'w') as file:
-            fmt = '.. c:autodoc:: {source}\n'
-            file.write(fmt.format(source=os.path.basename(testcase_out)))
-            for key in options.keys():
-                fmt = '   :{key}: {value}\n'
-                file.write(fmt.format(key=key, value=options[key]))
-
-    def _check_out(self, outdir, testcase_in):
-        testcase_out = testenv.modify_filename(testcase_in, dir=outdir)
-
-        # compare output from the pre-generated rst against the output 
generated
-        # by the extension
+# Use copy_srcdir_to_tmpdir=False and outdir='some-dir' for debugging

Is this comment relevant? I don't understand it.

+@with_app(srcdir=os.path.join(testenv.testdir, 'sphinx'),
+          buildername='text', copy_srcdir_to_tmpdir=True)
+def _sphinx_build_output(input_filename, output_filename, options,
+                         app, status, warning):
+    shutil.copyfile(input_filename,
+                    testenv.modify_filename(input_filename, dir=app.srcdir))
+
+    with open(os.path.join(app.srcdir, 'index.rst'), 'w') as file:
+        fmt = '.. c:autodoc:: {source}\n'
+        file.write(fmt.format(source=os.path.basename(input_filename)))
+        for key in options.keys():
+            fmt = '   :{key}: {value}\n'
+            file.write(fmt.format(key=key, value=options[key]))
+
+    app.build()
+    shutil.copyfile(os.path.join(app.outdir, 'index.txt'), output_filename)
+
+# Use copy_srcdir_to_tmpdir=False and outdir='some-dir' for debugging

Déjà vu.

+@with_app(srcdir=os.path.join(testenv.testdir, 'sphinx'),
+          buildername='text', copy_srcdir_to_tmpdir=True)
+def _sphinx_build_expected(input_filename, output_filename, options,
+                           app, status, warning):
+    shutil.copyfile(testenv.modify_filename(input_filename, ext='stdout'),
+                    os.path.join(app.srcdir, 'index.rst'))
+    app.build()
+    shutil.copyfile(os.path.join(app.outdir, 'index.txt'), output_filename)
+
+def _get_output(input_filename, **options):
+    output_filename = tempfile.mktemp()

Excerpt from my local installation:

mktemp(suffix='', prefix='tmp', dir=None)

    [...]

    THIS FUNCTION IS UNSAFE AND SHOULD NOT BE USED.  The file name may
    refer to a file that did not exist at some point, but by the time
    you get around to creating it, someone else may have beaten you to
    the punch.

There are safe alternatives though. Any interface that returns a file
already open. This is a pain though because you currently open the file
elsewhere.

An imperfect, but nicer compromise might be to create a temporary folder
instead and do everything inside. Other solutions are a lot more
invasive I think.

+    _sphinx_build_output(input_filename, output_filename, options)
+    ret = testenv.read_file(output_filename)
+    os.remove(output_filename)
+
+    return ret
+
+def _get_expected(input_filename, **options):
+    output_filename = tempfile.mktemp()

Ditto.

Regards,
Bruno

+    _sphinx_build_expected(input_filename, output_filename, options)
+    ret = testenv.read_file(output_filename)
+    os.remove(output_filename)
+
+    return ret
 
-        output = testenv.read_file(testenv.modify_filename(testcase_out,
-                                                           ext='output.txt'))
-
-        expected = testenv.read_file(testenv.modify_filename(testcase_out,
-                                                             
ext='expected.txt'))
-
-        self.assertEqual(expected, output)
-
-    # Use copy_srcdir_to_tmpdir=False and outdir='some-dir' for debugging
-    @with_app(srcdir=os.path.join(testenv.testdir, 'sphinx'),
-              buildername='text', copy_srcdir_to_tmpdir=True)
-    def test_directive(self, app, status, warning):
-        testcases = list(testenv.get_testcases(testenv.testdir))
-
-        for f in testcases:
-            self._setup_src(app.srcdir, f)
-
-        app.build()
+class DirectiveTest(unittest.TestCase):
+    pass
 
-        for f in testcases:
-            with self.subTest(source=os.path.basename(f)):
-                self._check_out(app.outdir, os.path.basename(f))
+testenv.assign_test_methods(DirectiveTest, _get_output, _get_expected)
 
 if __name__ == '__main__':
     unittest.main()

Other related posts: