Merge pull request #318 from mabruzzo/pytest-argparse

Improve ergonomics of pytest machinery

Author: mabruzzo

doc/source/tests/pytest_framework.rst

@@ -9,6 +9,9 @@ simulation is run with two versions of ``enzo-e`` and their results are compared
 This is useful for testing problems with no analytical solution or generally
 verifying that results from commonly run simulations don't drift.
 
+It is also useful in for testing problems that do have analytic solutions (the answer test might quantify how close a simulation result is to the analytic expected solution).
+While such tests do exist in the ctest-framework, they often involve more boiler-plate code.
+
 `pytest <https://docs.pytest.org/>`__ is a Python-based framework for detecting
 and running a series of tests within a source code repository. When running
 ``pytest``, the user can provide a directory in which ``pytest`` will look for
@@ -41,6 +44,8 @@ other useful answer testing functionality are located in the source in
 `test/answer_tests/answer_testing.py`. All answer tests are located in the
 other files within the `test/answer_tests` directory.
 
+Some other functionality, that may be reused in other unrelated scripts provided in the Enzo-E repository, are provided in the ``test_utils`` subdirectory.
+
 Running the Answer Test Suite
 -----------------------------
 
@@ -60,16 +65,49 @@ To generate test answers, use the highest numbered gold standard tag.
 Configuring the Answer Test Suite
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-Before the answer tests can be run, a few environment variables must be set to
-configure behavior.
+The behavior of the test can be configured by passing command line arguments to ``pytest`` or by setting environment variables (or by mixing both).
+
+When invoking ``pytest``, the command line flags discussed here should be passed **after** the path to the `test/answer_tests` directory has been provided.
+For the sake of example (the meaning of flags are explained below), one might invoke:
+
+.. code-block:: bash
 
- * ``TEST_RESULTS_DIR``: points to a directory in which answers will be stored
- * ``CHARM_PATH``: points to the directory in which ``charmrun`` is located
- * ``ENZO_PATH``: points to the ``enzo-e`` binary to use.
-   If this is not specified, this defaults to the ``<PATH/TO/ENZO-E/REPO>/build/bin/enzo-e``
- * ``GENERATE_TEST_RESULTS``: "true" to generate test results, "false" to compare with existing results.
- * ``GRACKLE_INPUT_DATA_DIR``: points to the directory where ``Grackle`` input files are installed.
-   If not specified, then all tests involving ``Grackle`` will be skipped.
+   $ pytest test/answer_tests  \
+            --build-dir ./build \
+            --answer-store
+
+The following table lists command line flags, and where applicable, the environment variables that they are interchangable with.
+In cases where both are set, the command line argument is given precedence.
+
+.. list-table:: Configuring pytest behavior
+   :widths: 10 10 30
+   :header-rows: 1
+
+   * - flag
+     - env var
+     - description
+   * - ``--build-dir``
+     - N/A
+     - points to the build-directory where the target enzo-e binary was built (that binary has the path: BUILD_DIR/bin/enzo-e).
+       The path to the charmrun launcher will be inferred from the `BUILD_DIR/CMakeCache.txt` file, but can be overwritten by the ``--charm`` flag or the ``CHARM_PATH`` environment variable.
+       This precedence was chosen in case a user causes a change to relevant cached build-variables, but have not rebuilt Enzo-E (i.e. `CMakeCache.txt` may not be valid for the binary).
+       When this flag isn't specified, the test infrastructure searches for the enzo-e binary at ENZOE_ROOT/build/bin/enzo-e, but doesn't try to infer charmrun's location from `CMakeCache.txt`.
+   * - ``--local-dir``
+     - ``TEST_RESULTS_DIR``
+     - points to a directory in which answers will be stored/loaded
+   * - ``--charm``
+     - ``CHARM_PATH``
+     - points to the directory in which ``charmrun`` is located
+   * - ``--answer-store``
+     - ``GENERATE_TEST_RESULTS``
+     - When the command line flag is specified, test results are generated. Otherwise, results are compared against existing results (unless the environment variable is specified).
+       The environment variable can be be set to ``"true"`` to generate test results or ``"false"`` to compare with existing results.
+   * - ``--grackle-input-data-dir``
+     - ``GRACKLE_INPUT_DATA_DIR``
+     - points to the directory where ``Grackle`` input files are installed.
+       If not specified, then all tests involving ``Grackle`` will be skipped.
+
+Earlier versions of the tests also required the ``"USE_DOUBLE"`` environment variable to be set to ``"true"`` or ``"false"`` to indicate whether the code had been compiled in double or single precision.
 
 .. code-block:: bash
 
@@ -83,21 +121,16 @@ First, check out the highest numbered gold standard tag and compile ``enzo-e``.
 
 .. code-block:: bash
 
-   $ git checkout gold-standard-1
+   # in the future, you will need to subsitute 004 for a higher number
+   $ git checkout gold-standard-004
    $ ...compile enzo-e
 
-Then, configure the test suite to generate answers by setting 
-GENERATE_TEST_RESULTS to true.
-
-.. code-block:: bash
-
-   $ export GENERATE_TEST_RESULTS=true
-
-Finally, run the test suite by calling ``pytest`` with the answer test directory.
+Then, run the test suite by calling ``pytest`` with the answer test directory (make sure to configure behavior correctly with command-line arguments or environment variables).
+In the following snippet, we assume you are currently at the root of the Enzo-E repository and that you will replace ``<build-dir>`` with the directory where you build enzo-e (this is commonly ``./build``)
 
 .. code-block:: bash
 
-   $ pytest test/answer_tests
+   $ pytest test/answer_tests --local-dir=~/enzoe_tests --build-dir=<build-dir> --answer-store
    ========================== test session starts ===========================
    platform linux -- Python 3.9.13, pytest-7.1.2, pluggy-1.0.0
    rootdir: /home/circleci/enzo-e
@@ -116,19 +149,16 @@ Comparing Test Answers
 
 Once test answers have been generated, the above steps need not be repeated until
 the gold standard tag has been updated. Now, any later version of the code can be
-run with the test suite to check for problems. Set the GENERATE_TEST_RESULTS
-environment variable to false to configure the test suite to compare with existing
-answers.
+run with the test suite to check for problems. To configure the test suite to compare with existing answers, omit the ``--answer-store`` flag and ensure that the ``GENERATE_TEST_RESULTS`` variable is either unset or set to ``"false"``.
 
 .. code-block:: bash
 
    $ git checkout main
    $ ...compile enzo-e
-   $ export GENERATE_TEST_RESULTS=false
-   $ pytest test/answer_tests
+   $ pytest test/answer_tests --local-dir=~/enzoe_tests --build-dir=<build-dir>
 
-Getting More Output from Pytest
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Helpful Tips
+^^^^^^^^^^^^
 
 By default, most output printed by ``enzo-e`` or the test scripts will be swallowed
 by ``pytest``. When tests fail, the Python traceback may be shown, but not much
@@ -139,7 +169,18 @@ variables when this flag is given.
 
 .. code-block:: bash
 
-   $ pytest -s test/answer_tests
+   $ pytest -s test/answer_tests # other args...
+
+When debugging an issue it's sometimes helpful to force pytest to run a subset of tests.
+This can be accomplished with the ``-k`` flag.
+For example, to only run a subset of tests with ``"grackle"`` in the test name, one might execute
+
+.. code-block:: bash
+
+   $ pytest test/answer_tests -k "grackle" # other args...
+
+When investigating a failing test or prototyping a brand-new test, it can sometimes be helpful to run the tests against multiple versions of enzo-e.
+Rather than rebuilding Enzo-E each time you want to do that, you can instead build the different versions of Enzo-E in separate build-directories, and direct ``pytest`` to use the different builds with the ``--build-dir`` flag.
 
 Creating New Answer Tests
 -------------------------
@@ -291,6 +332,17 @@ in single or double precision, and adjust the tolerance on the tests accordingly
    def test_hllc_cloud(self):
        ...
 
+.. note::
+
+   The above code is primarily for the sake of example.
+   In practice, we now automatically detect the code's precision from the enzo-e executable.
+
+Alternatively, additional configuration options can be configured through new command-line flags, which are introduced and parsed by the `conftest.py` file in the `answer_test` directory.
+This is generally more robust than adding environment variables (since the flags are more easily discovered and are more explicit).
+But, in practice it's made slightly more complicated by the fact that flags are parsed with pytest hooks.
+Flags added in this way work best with ``pytest`` fixtures, while our tests mostly leverage features from `Python's unittest module <https://docs.python.org/3/library/unittest.html>`_.
+
+
 Caveats
 ^^^^^^^
 

test/answer_tests/answer_testing.py

@@ -1,69 +1,66 @@
 import copy
+from dataclasses import dataclass
 import numpy as np
 import os
 import pytest
 import shutil
-import signal
-import subprocess
-import sys
 import tempfile
-import time
+from typing import Optional
 import yt
 
 from numpy.testing import assert_array_equal
 from unittest import TestCase
 from yt.funcs import ensure_dir
 from yt.testing import assert_rel_equal
 
+from test_utils.enzoe_driver import EnzoEDriver
+
 _base_file = os.path.basename(__file__)
 
-# If GENERATE_TEST_RESULTS="true", just generate test results.
-generate_results = os.environ.get("GENERATE_TEST_RESULTS", "false").lower() == "true"
-yt.mylog.info(f"{_base_file}: generate_results = {generate_results}")
-
-_results_dir = os.environ.get("TEST_RESULTS_DIR", "~/enzoe_test_results")
-test_results_dir = os.path.abspath(os.path.expanduser(_results_dir))
-yt.mylog.info(f"{_base_file}: test_results_dir = {test_results_dir}")
-if generate_results:
-    ensure_dir(test_results_dir)
-else:
-    if not os.path.exists(test_results_dir):
+@dataclass(frozen = True)
+class TestOptions:
+    enzoe_driver: EnzoEDriver
+    uses_double_prec: bool
+    generate_results: bool
+    test_results_dir: str
+    grackle_input_data_dir : Optional[str]
+
+_CACHED_OPTS = None
+
+def set_cached_opts(**kwargs):
+    global _CACHED_OPTS
+    if _CACHED_OPTS is not None:
+        raise RuntimeError("Can't call set_cached_opts more than once")
+
+    _CACHED_OPTS = TestOptions(**kwargs)
+    yt.mylog.info(
+        f"{_base_file}: generate_results = {_CACHED_OPTS.generate_results}")
+
+    yt.mylog.info(
+        f"{_base_file}: test_results_dir = {_CACHED_OPTS.test_results_dir}")
+    if _CACHED_OPTS.generate_results:
+        ensure_dir(_CACHED_OPTS.test_results_dir)
+    elif not os.path.exists(_CACHED_OPTS.test_results_dir):
         raise RuntimeError(
-            f"Test results directory not found: {test_results_dir}.")
-
-# Set the path to charmrun
-_charm_path = os.environ.get("CHARM_PATH", "")
-if not _charm_path:
-    raise RuntimeError(
-        f"Specify path to charm with CHARM_PATH environment variable.")
-charmrun_path = os.path.join(_charm_path, "charmrun")
-yt.mylog.info(f"{_base_file}: charmrun_path = {charmrun_path}")
-if not os.path.exists(charmrun_path):
-    raise RuntimeError(
-        f"No charmrun executable found in {_charm_path}.")
+            f"Test results dir not found: {_CACHED_OPTS.test_results_dir}.")
 
-src_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))
+    if ((_CACHED_OPTS.grackle_input_data_dir is not None) and
+        (not os.path.exists(_CACHED_OPTS.grackle_input_data_dir))):
+        raise RuntimeError(
+            "grackle input data dir not found: "
+            f"{_CACHED_OPTS.grackle_input_data_dir}")
 
-# Set the path to the enzo-e binary
-_enzo_path = os.environ.get("ENZO_PATH", "")
-if _enzo_path:
-    enzo_path = os.path.abspath(_enzo_path)
-else:
-    enzo_path = os.path.join(src_path, "build/bin/enzo-e")
-yt.mylog.info(f"{_base_file}: enzo_path = {enzo_path}")
-if not os.path.exists(enzo_path):
-    raise RuntimeError(
-        f"No enzo-e executable found in {enzo_path}.")
+    yt.mylog.info(
+        f"{_base_file}: use_double = {_CACHED_OPTS.uses_double_prec}")
 
-input_dir = "input"
+def cached_opts():
+    if _CACHED_OPTS is None:
+        raise RuntimeError("set_cached_opts was never called")
+    return _CACHED_OPTS
 
-# check for data path to grackle
-_grackle_input_data_dir = os.environ.get("GRACKLE_INPUT_DATA_DIR", None)
-if ((_grackle_input_data_dir is not None) and
-    (not os.path.exists(_grackle_input_data_dir))):
-    raise RuntimeError("GRACKLE_INPUT_DATA_DIR points to a non-existent "
-                       f"directory: {_grackle_input_data_dir}")
+src_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))
 
+input_dir = "input"
 
 _grackle_tagged_tests = set()
 
@@ -76,9 +73,15 @@ def uses_grackle(cls):
     """
     _grackle_tagged_tests.add(cls.__name__)
 
+    has_grackle = cached_opts().enzoe_driver.query_has_grackle()
+    has_grackle_inputs = cached_opts().grackle_input_data_dir is not None
+
+    skip_reason = "Enzo-E is not built with Grackle"
+    if has_grackle and (not has_grackle_inputs):
+        skip_reason = "the grackle input data dir was not specified"
+
     wrapper_factory = pytest.mark.skipif(
-        _grackle_input_data_dir is None,
-        reason = "GRACKLE_INPUT_DATA_DIR is not defined"
+        (not has_grackle) or (not has_grackle_inputs), reason = skip_reason
     )
     return wrapper_factory(cls)
 
@@ -94,40 +97,22 @@ def setup_symlinks(self):
 
         if self.__class__.__name__ in _grackle_tagged_tests:
             # make symlinks to each grackle input file
-            with os.scandir(_grackle_input_data_dir) as it:
+            with os.scandir(cached_opts().grackle_input_data_dir) as it:
                 for entry in it:
                     if not entry.is_file():
                         continue
                     os.symlink(entry.path,os.path.join(self.tmpdir, entry.name))
 
-    def run_simulation(self):
-        pfile = os.path.join(input_dir, self.parameter_file)
-        command = f"{charmrun_path} ++local +p{self.ncpus} {enzo_path} {pfile}"
-        proc = subprocess.Popen(
-            command, shell=True, close_fds=True,
-            preexec_fn=os.setsid)
-
-        stime = time.time()
-        while proc.poll() is None:
-            if time.time() - stime > self.max_runtime:
-                os.killpg(proc.pid, signal.SIGUSR1)
-                raise RuntimeError(
-                    f"Simulation {self.__class__.__name__} exceeded max runtime of "
-                    f"{self.max_runtime} seconds.")
-            time.sleep(1)
-
-        if proc.returncode != 0:
-            raise RuntimeError(
-                f"Simulation {self.__class__.__name__} exited with nonzero return "
-                f"code {proc.returncode}.")
-
     def setUp(self):
         self.curdir = os.getcwd()
         self.tmpdir = tempfile.mkdtemp()
 
         self.setup_symlinks()
         os.chdir(self.tmpdir)
-        self.run_simulation()
+        cached_opts().enzoe_driver.run(
+            parameter_fname = os.path.join(input_dir, self.parameter_file),
+            max_runtime = self.max_runtime, ncpus = self.ncpus,
+            sim_name = f"Simulation {self.__class__.__name__}")
 
     def tearDown(self):
         os.chdir(self.curdir)
@@ -145,18 +130,19 @@ def real_answer_test(func):
         def wrapper(*args):
             # name the file after the function
             filename = "%s.h5" % func.__name__
-            result_filename = os.path.join(test_results_dir, filename)
+            result_filename = os.path.join(cached_opts().test_results_dir,
+                                           filename)
 
             # check that answers exist
-            if not generate_results:
+            if not cached_opts().generate_results:
                 assert os.path.exists(result_filename), \
                   "Result file, %s, not found!" % result_filename
 
             data = func(*args)
             fn = yt.save_as_dataset({}, filename=filename, data=data)
 
             # if generating, move files to results dir
-            if generate_results:
+            if cached_opts().generate_results:
                 shutil.move(filename, result_filename)
             # if comparing, run the comparison
             else: