[develop]: Regression test improvements (#39)

## DESCRIPTION OF CHANGES: 
* Moved logic to parse `--tests` argument of `run_we2e_tests.py` into
python_utils so that it can be used by the `run_diff.py` regression test
script to determine which subdirectories to diff
* Added unit tests for `--tests` argument parsing (identified
unsupported inputs that could be added later)
* Updated regression test scripts to support `--tests` argument to run a
subset of tests and diff a subset of tests (reports failure if requested
test was not generated by either run)
* Updated `run_we2e_tests.py` script so that it can be run from any
directory instead of just the tests/WE2E directory (allows log outputs
to be written to more appropriate location when running via regression
test scripts
* Moved jinja2 imports in python_utils (metplus_conf_utils.py and
config_parser.py) into functions that use them so that the entire
python_utils module can be imported and functions that don't require
jinja2 can be called without the dependency
* Renamed `run_diff.py` argument `--baseline` to `--baseline_dir` to
differentiate from the `run_regression.py argument (one says to run
tests on the baseline commit while the other overrides the location of
the baseline data)
* Fixed bug in `run_regression.py` script where `--tests` argument
cannot take more than 1 input value (list of tests)
* Fixed this PR template that hides a header
* Fixed bug in logic to get merge commit of PR to run regression tests
* add ush to the python path in the pylint config file so PYTHONPATH
doesn't need to be set when calling pylint

Author: georgemccabe

.github/PULL_REQUEST_TEMPLATE

@@ -36,6 +36,7 @@
 <!-- Add any links to PRs (internal or external) that must be merged before this one. For example:
 - #23
 - https://github.com/dtcenter/METplus/pull/3301
+-->
 
 ## DOCUMENTATION:
 <!-- If this PR is contributing new capabilities that need to be documented, please also include updates to the RST files (docs/UsersGuide/source) as supporting material. -->

.github/workflows/python_tests.yaml

@@ -33,7 +33,6 @@ jobs:
       - name: Lint the python code
         run: |
           micromamba activate vx_workflow
-          export PYTHONPATH=$(pwd)/ush
           pylint --version
           pylint --ignore-imports=yes tests/test_python/test_*.py
           pylint ush/generate_wflow.py

.pylintrc

@@ -67,7 +67,7 @@ ignored-modules=
 
 # Python code to execute, usually for sys.path manipulation such as
 # pygtk.require().
-#init-hook=
+init-hook="import sys; sys.path.append('./ush')"
 
 # Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the
 # number of processors available to use, and will cap the count on Windows to
@@ -425,7 +425,8 @@ disable=raw-checker-failed,
         use-symbolic-message-instead,
         logging-fstring-interpolation,
         too-many-locals,
-        similarities
+        similarities,
+        import-outside-toplevel
 
 # Enable the message, report, category or checker with the given id(s). You can
 # either give multiple identifier separated by comma (,) or put this option

tests/WE2E/run_we2e_tests.py

@@ -15,22 +15,23 @@
 from pathlib import Path
 from textwrap import dedent
 
+# add tests/WE2E and ush directories to sys path to find other python files
+USH_DIR = Path(__file__).absolute().parents[2] / "ush"
+sys.path.insert(0, str(USH_DIR))
+
 from monitor_jobs import monitor_jobs, write_monitor_file
 
 from uwtools.api.config import get_yaml_config
 
-sys.path.append("../../ush")
 # pylint: disable=wrong-import-order, wrong-import-position
 from generate_wflow import generate_wflow
-from python_utils import check_python_version
-
+from python_utils import check_python_version, get_tests_to_run, get_pretty_list
 
 
-def run_we2e_tests(homedir, args) -> None:
+def run_we2e_tests(args) -> None:
     """Runs the Workflow End-to-End (WE2E) tests selected by the user
 
     Args:
-        homedir (str): The full path to the top-level application directory
         args    (argparse.Namespace): Command-line arguments
 
     Returns:
@@ -40,116 +41,31 @@ def run_we2e_tests(homedir, args) -> None:
     # Set up logging to write to screen and logfile
     setup_logging(debug=args.debug)
     logging.debug(f"Arguments to run_we2e_tests():\n{args}")
-    # Set some important directories
-    ushdir = Path(homedir, "ush")
 
     # Set some variables based on input arguments
     machine = args.machine.lower()
 
     # Derecho requires long delay between calls to rocotorun due to system-level cacheing of
     # job statuses
-    if machine=="derecho":
-        if args.delay < 60:
-            logging.info("Derecho requires 60 second delay between calls to rocotorun")
-            args.delay=60
-
-    alltests = glob.glob("test_configs/**/config*.yaml", recursive=True)
-    testdirs = next(os.walk("test_configs"))[1]
-    # If args.tests is a list of length more than one, we assume it is a list of test names
-    if len(args.tests) > 1:
-        tests_to_check = args.tests
-        logging.debug(f"User specified a list of tests:\n{tests_to_check}")
-    else:
-        # First see if args.tests is a valid test name
-        user_spec_tests = args.tests
-        logging.debug(f"Checking if {user_spec_tests} is a valid test name")
-        match = check_test(user_spec_tests[0])
-        if match:
-            tests_to_check = user_spec_tests
-        else:
-            # If not a valid test name, check if it is a test suite
-            logging.debug(f"Checking if {user_spec_tests} is a valid test suite")
-            if user_spec_tests[0] == "all":
-                tests_to_check = []
-                for f in alltests:
-                    filename = Path(f).name
-                    # We just want the test name in this list, so cut out the
-                    # "config." prefix and ".yaml" extension
-                    if len(filename) > 12:
-                        if filename[:7] == "config." and filename[-5:] == ".yaml":
-                            tests_to_check.append(filename[7:-5])
-                        else:
-                            logging.debug(f"Skipping non-test file {filename}")
-                    else:
-                        logging.debug(f"Skipping non-test file {filename}")
-                logging.debug(f"Will check all tests:\n{tests_to_check}")
-            elif user_spec_tests[0] in testdirs:
-                # If a subdirectory under test_configs/ is specified, run all
-                # tests in that directory
-                logging.debug(
-                    f"{user_spec_tests[0]} is one of the testing directories:\n{testdirs}"
-                )
-                logging.debug(
-                    f"Will run all tests in test_configs/{user_spec_tests[0]}"
-                )
-                tests_in_dir = glob.glob(
-                    f"test_configs/{user_spec_tests[0]}/config*.yaml", recursive=True
-                )
-                tests_to_check = []
-                for f in tests_in_dir:
-                    filename = Path(f).name
-                    # We just want the test name in this list, so cut out the
-                    # "config." prefix and ".yaml" extension
-                    if len(filename) > 12:
-                        if filename[:7] == "config." and filename[-5:] == ".yaml":
-                            tests_to_check.append(filename[7:-5])
-                        else:
-                            logging.debug(f"Skipping non-test file {filename}")
-                    else:
-                        logging.debug(f"Skipping non-test file {filename}")
-            else:
-                # If we have gotten this far then the only option left for user_spec_tests is a
-                # file containing test names
-                logging.debug(
-                    f"Checking if {user_spec_tests} is a file containing test names"
-                )
-                if Path(user_spec_tests[0]).is_file():
-                    with open(user_spec_tests[0], encoding="utf-8") as f:
-                        tests_to_check = [x.rstrip() for x in f]
-                else:
-                    raise FileNotFoundError(
-                        dedent(
-                            f"""
-                    The specified 'tests' argument '{user_spec_tests}'
-                    does not appear to be a valid test name, a valid test suite, a subdirectory
-                    under test_configs/, or a file containing valid test names.
-
-                    Check your inputs and try again.
-                    """
-                        )
-                    )
-
-    logging.info("Checking that all tests are valid")
-
-    tests_to_run = check_tests(tests_to_check)
-
-    pretty_list = "\n".join(str(x) for x in tests_to_run)
-    logging.info(f"Will run {len(tests_to_run)} tests:\n{pretty_list}")
+    if machine=="derecho" and args.delay < 60:
+        logging.info("Derecho requires 60 second delay between calls to rocotorun")
+        args.delay=60
+
+    tests_to_run = get_tests_to_run(args.tests)
 
-    config_default_file = Path(ushdir, "config_defaults.yaml")
-    logging.debug(f"Loading config defaults file {config_default_file}")
-    config_defaults = get_yaml_config(config_default_file)
+    pretty_list = get_pretty_list(tests_to_run)
+    logging.info(f"Will run {len(tests_to_run)} tests:\n{pretty_list}")
 
-    machine_file = Path(ushdir, "machine", f"{machine}.yaml")
+    machine_file = Path(USH_DIR, "machine", f"{machine}.yaml")
     logging.debug(f"Loading machine defaults file {machine_file}")
     machine_defaults = get_yaml_config(machine_file)
 
+    starttime_string = datetime.now().strftime("%Y%m%d%H%M%S")
+
     monitor_yaml = {}
     for test in tests_to_run:
         # Starting with test yaml template, fill in user-specified and machine- and
         # test-specific options, then write resulting complete config.yaml
-        starttime = datetime.now()
-        starttime_string = starttime.strftime("%Y%m%d%H%M%S")
         test_name = Path(test).name.split(".")[1]
         logging.debug(f"For test {test_name}, constructing config.yaml")
         test_cfg = get_yaml_config(test)
@@ -201,16 +117,16 @@ def run_we2e_tests(homedir, args) -> None:
             "based on specified command-line arguments:\n"
         )
         logging.debug(str(test_cfg))
-        test_cfg.dump(Path(ushdir, "config.yaml"))
+        test_cfg.dump(Path(USH_DIR, "config.yaml"))
 
         logging.info(f"Calling workflow generation function for test {test_name}\n")
         if args.quiet:
             console_handler = logging.getLogger().handlers[1]
             console_handler.setLevel(logging.WARNING)
         expt_dir = generate_wflow(
-            ushdir=str(ushdir),
+            ushdir=str(USH_DIR),
             config="config.yaml",
-            logfile=f"{str(ushdir)}/log.generate_wflow",
+            logfile=f"{str(USH_DIR)}/log.generate_wflow",
             debug=args.debug,
         )
         if args.quiet:
@@ -238,131 +154,44 @@ def run_we2e_tests(homedir, args) -> None:
             # Make WORKFLOW_ID actually mean something
             test_cfg["workflow"].update({"WORKFLOW_ID": workflow_id})
 
-    if args.launch != "cron":
-        monitor_file = f"WE2E_tests_{starttime_string}.yaml"
-        write_monitor_file(monitor_file, monitor_yaml)
-        logging.info("All experiments have been generated;")
-        logging.info(f"Experiment file {monitor_file} created")
-        if args.launch == "python":
-            write_monitor_file(monitor_file, monitor_yaml)
-            logging.debug("calling function that monitors jobs, prints summary")
-            try:
-                monitor_file = monitor_jobs(
-                    monitor_yaml,
-                    monitor_file=monitor_file,
-                    procs=args.procs,
-                    debug=args.debug,
-                    delay=args.delay,
-                )
-            except KeyboardInterrupt:
-                logging.info(
-                    "\n\nUser interrupted monitor script; to resume monitoring jobs run:\n"
-                )
-                rerun_string=f"./monitor_jobs.py -y={monitor_file}"
-                if args.procs>1:
-                    rerun_string+=f" -p={args.procs}"
-                if args.delay!=5:
-                    rerun_string+=f" --delay={args.delay}"
-
-                logging.info(f"{rerun_string}\n")
-        else:
-            logging.info("To automatically run and monitor experiments, use:\n")
-            logging.info(f"./monitor_jobs.py -y={monitor_file}\n")
-    else:
+    if args.launch == "cron":
         logging.info(
             "All experiments have been generated; using cron to submit workflows"
         )
         logging.info("To view running experiments in cron try `crontab -l`")
+        return
 
+    monitor_file = f"WE2E_tests_{starttime_string}.yaml"
+    write_monitor_file(monitor_file, monitor_yaml)
+    logging.info("All experiments have been generated;")
+    logging.info(f"Experiment file {monitor_file} created")
 
-def check_tests(tests: list) -> list:
-    """
-    Checks that all tests in a provided list of tests are valid
-
-    Args:
-        tests (list): List of potentially valid test names
-
-    Returns:
-        tests_to_run: List of configuration files corresponding to test names
-    """
+    if args.launch == "none":
+        logging.info("To automatically run and monitor experiments, use:\n")
+        logging.info(f"./monitor_jobs.py -y={monitor_file}\n")
+        return
 
-    testfiles = glob.glob("test_configs/**/config*.yaml", recursive=True)
-    # Check that there are no duplicate test filenames
-    testfilenames = []
-    for testfile in testfiles:
-        testfile = Path(testfile)
-        if testfile.name in testfilenames:
-            duplicates = glob.glob(f"test_configs/**/{testfile.name}", recursive=True)
-            raise ValueError(
-                dedent(
-                    f"""
-                            Found duplicate test file names:
-                            {duplicates}
-                            Ensure that each test file name under the test_configs/ directory
-                            is unique.
-                            """
-                )
-            )
-        testfilenames.append(testfile.name)
-    tests_to_run = []
-    for test in tests:
-        # Skip blank/empty testnames; this avoids failure if newlines or spaces are included
-        if not test or test.isspace():
-            continue
-        # Skip if string has an octothorpe
-        if "#" in test:
-            logging.debug(
-                f"Assuming line is a comment due to presence of '#' character:\n{test}"
-            )
-            continue
-        match = check_test(test)
-        if not match:
-            raise FileNotFoundError(f"Could not find test {test}")
-        tests_to_run.append(match)
-    # Because some test files are symlinked to other tests, check that we don't
-    # include the same test twice
-    for testfile in tests_to_run.copy():
-        testfile = Path(testfile)
-        if testfile.is_symlink():
-            if testfile.resolve() in tests_to_run:
-                logging.warning(
-                    dedent(
-                        f"""WARNING: test file {testfile} is a symbolic link to a
-                                test file ({testfile.resolve()}) that is also included in
-                                the test list. Only the latter test will be run."""
-                    )
-                )
-                tests_to_run.remove(str(testfile))
-    if len(tests_to_run) != len(set(tests_to_run)):
-        logging.warning(
-            "\nWARNING: Duplicate test names were found in list. "
-            "Removing duplicates and continuing.\n"
+    write_monitor_file(monitor_file, monitor_yaml)
+    logging.debug("calling function that monitors jobs, prints summary")
+    try:
+        monitor_file = monitor_jobs(
+            monitor_yaml,
+            monitor_file=monitor_file,
+            procs=args.procs,
+            debug=args.debug,
+            delay=args.delay,
         )
-        tests_to_run = list(set(tests_to_run))
-    return tests_to_run
-
-
-def check_test(test: str) -> str:
-    """
-    Checks that a string corresponds to a valid test name
-
-    Args:
-        test (str): Potential test name
-
-    Returns:
-        config: Name of the test configuration file (empty string if no test file is found)
-    """
-    # potential test files
-    testfiles = glob.glob("test_configs/**/config*.yaml", recursive=True)
-    # potential test file for input test name
-    test_config = f"config.{test.strip()}.yaml"
-    config = ""
-    for testfile in testfiles:
-        if test_config in testfile:
-            logging.debug(f"found test {test}, testfile {testfile}")
-            config = Path(testfile).absolute()
-    return config
+    except KeyboardInterrupt:
+        logging.info(
+            "\n\nUser interrupted monitor script; to resume monitoring jobs run:\n"
+        )
+        rerun_string=f"./monitor_jobs.py -y={monitor_file}"
+        if args.procs>1:
+            rerun_string+=f" -p={args.procs}"
+        if args.delay!=5:
+            rerun_string+=f" --delay={args.delay}"
 
+        logging.info(f"{rerun_string}\n")
 
 def setup_logging(logfile: str = "log.run_WE2E_tests", debug: bool = False) -> None:
     """
@@ -399,8 +228,6 @@ def setup_logging(logfile: str = "log.run_WE2E_tests", debug: bool = False) -> N
     # Check python version and presence of some non-standard packages
     check_python_version()
 
-    # Get the "Home" directory, two levels above this one
-    top_dir = Path(__file__).absolute().parent.parent.parent
     LOGFILE = "log.run_WE2E_tests"
 
     # Parse arguments
@@ -533,7 +360,7 @@ def setup_logging(logfile: str = "log.run_WE2E_tests", debug: bool = False) -> N
     # Call main function
 
     try:
-        run_we2e_tests(top_dir, user_args)
+        run_we2e_tests(user_args)
     except: #pylint: disable=bare-except
         logging.exception(
             dedent(