diff --git a/.github/PULL_REQUEST_TEMPLATE b/.github/PULL_REQUEST_TEMPLATE index 71336454..e6b7dbf2 100644 --- a/.github/PULL_REQUEST_TEMPLATE +++ b/.github/PULL_REQUEST_TEMPLATE @@ -36,6 +36,7 @@ ## DOCUMENTATION: diff --git a/.github/workflows/python_tests.yaml b/.github/workflows/python_tests.yaml index 3bbc619f..1080f0f3 100644 --- a/.github/workflows/python_tests.yaml +++ b/.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 diff --git a/.pylintrc b/.pylintrc index 3167c2eb..33299b96 100644 --- a/.pylintrc +++ b/.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 diff --git a/tests/WE2E/run_we2e_tests.py b/tests/WE2E/run_we2e_tests.py index 34635e3e..b8851fe1 100755 --- a/tests/WE2E/run_we2e_tests.py +++ b/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 != "python": + 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( diff --git a/tests/regression/README.md b/tests/regression/README.md index 6e888df6..7c0a0dcd 100644 --- a/tests/regression/README.md +++ b/tests/regression/README.md @@ -155,7 +155,7 @@ The default behavior is to pass `--tests all` to the `run_we2e_tests.py` script. To run the METplus diff utility, call the `run_diff.py` script, passing the path to the output directory of the end-to-end tests. By default, the output.baseline directory is used as the baseline. -You can override this by passing the `--baseline` argument. +You can override this by passing the `--baseline_dir` argument. You can also override the location of METplus to use with the `--metplus` argument. The default behavior is to run the diff utility on all files in the dated subdirectories @@ -175,3 +175,13 @@ python3 ./tests/regression/run_diff.py \ ${test_dir} \ --regression_dir ${regression_dir} ``` + +### Diffing a Subset of Tests + +The `--tests` argument can be provided to the `run_diff.py` script +to define a subset of tests to diff. +The format of the argument is the same as the `--tests` argument to the +`tests/WE2E/run_we2e_tests.py` script. +The list of tests defined here determines which subdirectories to pass to the +diff utility. Tests not found in either directory will be reported as a failure. +The default behavior is diff all tests found under `tests/WE2E/test_configs`. diff --git a/tests/regression/regression_common.py b/tests/regression/regression_common.py index 7b8c4bc1..cd42d2d9 100644 --- a/tests/regression/regression_common.py +++ b/tests/regression/regression_common.py @@ -4,5 +4,5 @@ # repository for dtc-vx-workflow git clone WORKFLOW_REPO = "dtcenter/dtc-vx-workflow" -# string to used in diff results to indicate a test was not run -NOT_RUN = "NOT RUN" \ No newline at end of file +# string used in diff results to indicate a test was not run +NOT_RUN = "NOT RUN" diff --git a/tests/regression/run_diff.py b/tests/regression/run_diff.py index a2b759b8..ec324066 100644 --- a/tests/regression/run_diff.py +++ b/tests/regression/run_diff.py @@ -11,11 +11,20 @@ from regression_common import DEFAULT_REGRESSION_DIR, NOT_RUN +# get function from WE2E script to get tests to run + +USH_DIR = Path(__file__).absolute().parents[2] / "ush" +sys.path.insert(0, str(USH_DIR)) + +from python_utils.parse_test_list import get_tests_to_run, get_pretty_list + +# class to store test results - status and any details regarding differences @dataclass class TestResults: status: str = NOT_RUN - details: str = "" + details: str = "No details to report" +# keywords to search in file paths to skip diff if found SKIP_KEYWORDS = [ "vx_wflow.xml", "var_defns.yaml", @@ -40,43 +49,31 @@ def main(): print(f"ERROR: METplus directory does not exist: {args.metplus}") sys.exit(1) - success = True - sys.path.insert(0, str(args.metplus)) from metplus.util import diff_util diff_util.SKIP_KEYWORDS = SKIP_KEYWORDS + tests_to_diff = get_tests_to_run(args.tests, names_only=True) + msg = ( "Running diff tests" - f"\nBASELINE: {Path(args.baseline).resolve()}" + f"\nBASELINE: {Path(args.baseline_dir).resolve()}" f"\nNEW : {Path(args.test_dir).resolve()}" + f"\n\nTests to Diff:\n{get_pretty_list(tests_to_diff)}" + f"\n\nUsing SKIP_KEYWORDS:\n{get_pretty_list(SKIP_KEYWORDS)}\n" ) logging.info(msg) if not args.log_to_terminal: print(msg) - tests_baseline = get_test_paths(args.baseline, not args.diff_inputs) - tests_new = get_test_paths(args.test_dir, not args.diff_inputs) - - all_tests = list(set(tests_baseline).union(tests_new)) - test_results = {x: TestResults() for x in all_tests} - - # set tests that are not found in either baseline or new output to failed - not_in_baseline = [x for x in tests_new if x not in tests_baseline] - not_in_new = [x for x in tests_baseline if x not in tests_new] - for test_name in not_in_baseline: - test_results[test_name].status = 'FAILED (not in baseline)' - success = False - for test_name in not_in_new: - test_results[test_name].status = 'FAILED (not in new output)' - success = False + test_results = init_test_results(tests_to_diff, args) for test_name, test_result in test_results.items(): if test_result.status != NOT_RUN: continue print(f"Diffing {test_name}...") - baseline_test = Path(args.baseline) / test_name + baseline_test = Path(args.baseline_dir) / test_name new_test = Path(args.test_dir) / test_name # create text stream to capture diff output for each test @@ -91,20 +88,23 @@ def main(): test_result.status = 'SUCCEEDED' continue - test_result.status = 'FAILED' - success = False + test_result.status = 'FAILED (diffs found)' + + longest_test_len = len(max(test_results, key=len)) logging.info("SUMMARY:") for test_name, test_result in test_results.items(): - logging.info(f"{test_name.ljust(65)}: {test_result.status}") + logging.info(f"{test_name.ljust(longest_test_len+1)}: {test_result.status}") - logging.info("DETAILS:") + logging.info("\nDETAILS:") for test_name, test_result in test_results.items(): logging.info(f"{'-' * 80}\n{test_name}: {test_result.status}\n{'-' * 80}") logging.info(test_result.details) logging.info(f"{'-' * 80}\n\n") log_msg = '' if not log_file else f"\nSee log file for details: {log_file}" + + success = all(result.status == "SUCCEEDED" for result in test_results.values()) if success: msg = f"SUCCESS: No differences found!{log_msg}" logging.info(msg) @@ -117,6 +117,59 @@ def main(): return success +def init_test_results(tests_to_diff, args): + test_results = {} + + for test_name in tests_to_diff: + baseline_path = Path(args.baseline_dir) / test_name + test_path = Path(args.test_dir) / test_name + + baseline_found = baseline_path.is_dir() + new_found = test_path.is_dir() + + # if test output is found for both baseline and new output + if baseline_found and new_found: + # if diffing inputs, init test results for TEST_NAME + if args.diff_inputs: + # default status is NOT RUN, so diff will be run for this test + test_results[test_name] = TestResults() + continue + + # otherwise init test results for each dated subdirectory + + # get dated subdirectories from both baseline and new output + baseline_subdirs = _get_dated_subdirectories(baseline_path) + test_subdirs = _get_dated_subdirectories(test_path) + + for subdir in baseline_subdirs.union(test_subdirs): + # init test results for TEST_NAME/YYYYMMDDHH + subdir_name = f"{test_name}/{subdir}" + test_results[subdir_name] = TestResults() + + # mark tests as failed if dated subdirectory found in one but not the other + if subdir in baseline_subdirs - test_subdirs: + test_results[subdir_name].status = 'FAILED (not in new output)' + elif subdir in test_subdirs - baseline_subdirs: + test_results[subdir_name].status = 'FAILED (not in baseline output)' + # if dated subdirectory found in both, leave status as NOT RUN to run diff + + continue + + # if test is not found in either baseline or new output, set status to failed + test_results[test_name] = TestResults() + + if not baseline_found and not new_found: + test_results[test_name].status = 'FAILED (not in either output)' + elif baseline_found: + test_results[test_name].status = 'FAILED (not in new output)' + else: + test_results[test_name].status = 'FAILED (not in baseline)' + + return test_results + +def _get_dated_subdirectories(the_path): + return set([x.name for x in the_path.iterdir() if x.is_dir() and x.name.isdigit()]) + def read_args() -> argparse.Namespace: parser = argparse.ArgumentParser(description="Run difference tests") parser.add_argument("test_dir", @@ -125,7 +178,7 @@ def read_args() -> argparse.Namespace: help=f"Directory containing regression test output (default: {DEFAULT_REGRESSION_DIR})") parser.add_argument("--metplus", help="Directory of METplus repo to get diff_util.py (default: regression_dir/METplus)") - parser.add_argument("--baseline", + parser.add_argument("--baseline_dir", help="Directory with baseline data to use for comparison (default: regression_dir/output.baseline)") parser.add_argument("--diff_inputs", action="store_true", help="If set, run the diff utility on each output directory, which includes the input observation files.") @@ -135,13 +188,15 @@ def read_args() -> argparse.Namespace: help="If set, log to the terminal (standard output) instead of a file") parser.add_argument("-d", "--debug", action="store_true", help="Log information about files that were skipped or had no differences") - + parser.add_argument("-t", "--tests", type=str, nargs="*",default=["all"], + help="Defines tests to diff (default: all)." + " Matches format expected by run_we2e_tests.py script.") args = parser.parse_args() if not args.metplus: args.metplus = Path(args.regression_dir) / "METplus" - if not args.baseline: - args.baseline = Path(args.regression_dir) / "output.baseline" + if not args.baseline_dir: + args.baseline_dir = Path(args.regression_dir) / "output.baseline" if not args.log_dir: args.log_dir = args.regression_dir @@ -168,28 +223,6 @@ def setup_logging(args): logging.basicConfig(**log_config) return log_file_path -def get_test_paths(base_path, get_dated=True): - paths = [] - # Loop through the main test directories (e.g., TEST_NAME) - for test_dir in Path(base_path).iterdir(): - if not test_dir.is_dir(): - continue - - has_date_subdirs = False - - if get_dated: - # Look for numeric subdirectories inside the test directory - for sub_dir in test_dir.iterdir(): - if sub_dir.is_dir() and sub_dir.name.isdigit(): - # Store as 'TEST_NAME/202602010000' - paths.append(f"{test_dir.name}/{sub_dir.name}") - has_date_subdirs = True - - # Fallback if get_dated is False or no numeric subdirs were found - if not has_date_subdirs: - paths.append(test_dir.name) - - return paths if __name__ == "__main__": status = main() diff --git a/tests/regression/run_regression.py b/tests/regression/run_regression.py index e57eb04c..2df7df5b 100644 --- a/tests/regression/run_regression.py +++ b/tests/regression/run_regression.py @@ -67,7 +67,7 @@ def read_args() -> argparse.Namespace: help=f"Directory to run regression tests (default: {DEFAULT_REGRESSION_DIR})") parser.add_argument("--clone_https", action="store_true", help="Clone via https instead of ssh", ) - parser.add_argument("--tests", default="all", + parser.add_argument("-t", "--tests", type=str, nargs="*",default=["all"], help="Defines tests to run (default: all)." " Matches format expected by run_we2e_tests.py script.") args = parser.parse_args() @@ -80,7 +80,7 @@ def read_args() -> argparse.Namespace: args.pr = None # tests argument cannot be provided if baseline argument is requested - if args.tests != "all": + if args.tests != ["all"]: print("ERROR: Cannot specify --baseline and a subset of tests with --tests") sys.exit(1) @@ -115,42 +115,43 @@ def setup_repo_dir(args: argparse.Namespace, workflow_repo_dir: Path): if args.pr: merge_commit_id = f"refs/pull/{args.pr}/merge" run_command(f"git -C {workflow_repo_dir} fetch origin {merge_commit_id}") - run_command(f"git -C {workflow_repo_dir} checkout {merge_commit_id}") + run_command(f"git -C {workflow_repo_dir} checkout FETCH_HEAD") elif args.branch: run_command(f"git -C {workflow_repo_dir} checkout {args.branch}") # check out specific commit if specified if args.commit: run_command(f"git -C {workflow_repo_dir} checkout {args.commit}") - # otherwise pull the latest changes if obtaining a branch + # otherwise pull the latest changes if requesting a branch elif args.branch: run_command(f"git -C {workflow_repo_dir} pull origin {args.branch}") def launch_tests(args: argparse.Namespace, workflow_repo_dir: Path, output_path: Path): - print(f"Running WE2E tests in {Path(output_path.parent.name) / output_path.name}: {args.tests}") + pretty_list = "\n".join(str(x) for x in args.tests) + print(f"Running WE2E tests in {Path(output_path.parent.name) / output_path.name}\n{pretty_list}") # Determine paths relative to the workflow repo directory - we2e_test_dir = workflow_repo_dir / "tests" / "WE2E" - test_script = we2e_test_dir / "run_we2e_tests.py" + test_script = workflow_repo_dir / "tests" / "WE2E" / "run_we2e_tests.py" # Commands to set up conda and run the tests cmd = ( f"source {workflow_repo_dir}/setup_conda.sh &&" f" {test_script} --account {args.account} --machine {args.machine}" - f" --tests {args.tests} --expt_basedir {output_path}" + f" --tests {' '.join(args.tests)} --expt_basedir {output_path}" ) + nohup_log = output_path / "nohup.out" print(f"RUNNING: {cmd}") - print(f"CWD: {we2e_test_dir}") + print(f"CWD: {output_path}") print("Launching in background with nohup") - print(f"Follow {we2e_test_dir}/nohup.out for output", flush=True) + print(f"Follow {nohup_log} for output", flush=True) try: # run setup_conda.sh and end-to-end test script - subprocess.Popen(f"nohup bash -c '{cmd}' &", shell=True, cwd=we2e_test_dir) + subprocess.Popen(f"nohup bash -c '{cmd}' &", shell=True, cwd=output_path) except Exception as e: print(f"Failed to launch test command: {e}") diff --git a/tests/test_python/test_python_utils.py b/tests/test_python/test_python_utils.py index 724a34b2..b3e19f06 100644 --- a/tests/test_python/test_python_utils.py +++ b/tests/test_python/test_python_utils.py @@ -13,6 +13,7 @@ import glob import os import shutil +from pathlib import Path import tempfile import unittest @@ -132,14 +133,145 @@ def test_config_parser(self): "regional_workflow", util.get_ini_value(cfg, "regional_workflow", "repo_url") ) + def test_parse_test_list_get_tests_to_run(self): + """ Test parsing of different --tests arguments formats """ + # get all existing tests from tests/WE2E/test_configs directory + test_configs_dir = Path(self.ushdir).parent / "tests" / "WE2E" / "test_configs" + test_categories = [f.name for f in os.scandir(test_configs_dir) if f.is_dir()] + we2e_tests = {} + for test_category in test_categories: + tests = [f.name[7:-5] for f in os.scandir(os.path.join(test_configs_dir, test_category)) + if f.is_file()] + we2e_tests[test_category] = tests + + total_test_num = sum(len(tests) for tests in we2e_tests.values()) + all_tests = [item for tests in we2e_tests.values() for item in tests] + + # sub tests to run to test different values for --tests argument and their expected results + # each test contains: + # * name of the test to identify it if it fails + # * list of arguments to pass to the --tests argument + # * expected number of tests that were found + # * exception that is expected to be raised (set to None if no exception is expected) + # Note: number of expected tests for tests that raise exceptions are set to the expected + # number of valid tests found if logic is revised to ignore/warn instead of error when a + # bad value is provided, enhanced to support multiple test categories or categories in + # file list files, etc. These tests are noted with "(not supported)" in the name. + + test_cases = [ + ( + "all", + ["all"], + total_test_num, None + ), + ( + "single test", + ["MET_verification_winter_wx"], + 1, None + ), + ( + "single test invalid", + ["pizza"], + 0, FileNotFoundError + ), + ( + "single test file name (not supported)", + ["config.MET_verification_winter_wx.yaml"], + 1, FileNotFoundError + ), + ( + "multiple tests", + ["MET_verification_winter_wx", "vx-det_multicyc_fcst-overlap_ncep-hrrr"], + 2, None + ), + ( + "all tests listed out", + all_tests, + total_test_num, None + ), + ( + "multiple tests one invalid", + ["MET_verification_winter_wx", "pudding"], + 1, FileNotFoundError + ), + ( + "subdirectory invalid", + ["olives"], + 0, FileNotFoundError + ), + ( + "2 subdirectories (not supported)", + ["deterministic", "ensemble"], + len(we2e_tests["deterministic"] + we2e_tests["ensemble"]), FileNotFoundError + ), + ] + + # add test cases for each subdirectory + # list of test names and temporary file containing list of test names + for test_cat, tests in we2e_tests.items(): + test_cases.append(( + f"{test_cat} subdir", + [test_cat], + len(tests), None + )) + test_cases.append(( + f"file path - {test_cat} subdir", + [self.create_tmp_test_file(tests)], + len(tests), None + )) + + # name of file path using temporary files + test_cases.append(( + "file path - all tests", + [self.create_tmp_test_file(all_tests)], + total_test_num, None + )) + + # file path that contains an invalid test name + test_cases.append(( + "file path - invalid test name", + [self.create_tmp_test_file(["MET_verification_winter_wx", "pizza"])], + 1, FileNotFoundError + )) + + # file path that contains a test category (not supported) + test_cases.append(( + "file path - category (not supported)", + [self.create_tmp_test_file(["deterministic"])], + len(we2e_tests["deterministic"]), FileNotFoundError + )) + + # test both options for names_only argument to get_tests_to_run function + for names_only in (True, False): + for name, inputs, expected, exception in test_cases: + with self.subTest(msg=name, inputs=inputs, expected=expected): + if exception: + with self.assertRaises(exception): + util.get_tests_to_run(inputs, names_only=names_only) + else: + actual_results = util.get_tests_to_run(inputs, names_only=names_only) + self.assertEqual(len(actual_results), expected) + if not names_only: + for result in actual_results: + self.assertIn(str(test_configs_dir), str(result)) + + def create_tmp_test_file(self, contents: list) -> str: + """Helper to create a temporary file and automatically schedule its deletion.""" + with tempfile.NamedTemporaryFile(mode="w", delete=False) as tmp: + tmp.write("\n".join(contents)) + file_path = tmp.name + + self.addCleanup(os.remove, file_path) + return file_path + def setUp(self): """setUp is where we do preparation for running the unittests. If you need to download files for running test cases, prepare common stuff for all test cases etc, this is the best place to do it""" util.set_env_var("DEBUG", "FALSE") - self.test_dir = os.path.dirname(os.path.abspath(__file__)) - self.ushdir = os.path.join(self.test_dir, "..", "..", "ush") + self.test_dir = Path(__file__).resolve().parent + self.ushdir = self.test_dir.parents[1] / "ush" if __name__ == "__main__": unittest.main() diff --git a/ush/python_utils/__init__.py b/ush/python_utils/__init__.py index a7edc9af..5f4159cb 100644 --- a/ush/python_utils/__init__.py +++ b/ush/python_utils/__init__.py @@ -42,3 +42,4 @@ ) from .metplus_conf_utils import render_metplus_confs, make_var_lists from .eval_metplus_tmpl import eval_metplus_timestr_tmpl, eval_metplus_dt_tmpl +from .parse_test_list import get_tests_to_run, get_pretty_list \ No newline at end of file diff --git a/ush/python_utils/config_parser.py b/ush/python_utils/config_parser.py index 41caf42c..e5ff41a0 100644 --- a/ush/python_utils/config_parser.py +++ b/ush/python_utils/config_parser.py @@ -26,7 +26,6 @@ import xml.etree.ElementTree as ET from xml.dom import minidom -import jinja2 # # Note: yaml may not be available in which case we suppress # the exception, so that we can have other functionality @@ -181,7 +180,7 @@ def extend_yaml(yaml_dict, full_dict=None, parent=None): Updates ``yaml_dict`` in place by rendering any existing Jinja2 templates that exist in a value. """ - + import jinja2 if full_dict is None: full_dict = yaml_dict diff --git a/ush/python_utils/metplus_conf_utils.py b/ush/python_utils/metplus_conf_utils.py index fe86deda..a774dcc4 100644 --- a/ush/python_utils/metplus_conf_utils.py +++ b/ush/python_utils/metplus_conf_utils.py @@ -2,10 +2,6 @@ Common utilities for creating/handling METplus configuration files """ import logging -import os -import sys -from jinja2 import Environment, FileSystemLoader - def make_var_lists(vx_config_dict,field_group): """Renders two multi-line strings representing the list of forecast and observation variables @@ -88,7 +84,7 @@ def render_metplus_confs(cfg,settings,template_fn,vx_leadhr_list,tasks): """Renders metplus conf files from the appropriate template and user settings. If VX_TASKS > 1 and vx_leadhr_list > 1, renders a conf file for each parallel task. Returns the filename(s) of metplus conf files that were rendered""" - + from jinja2 import Environment, FileSystemLoader logger = logging.getLogger(__name__) num_fhrs = len(vx_leadhr_list) diff --git a/ush/python_utils/parse_test_list.py b/ush/python_utils/parse_test_list.py new file mode 100644 index 00000000..1d0b3116 --- /dev/null +++ b/ush/python_utils/parse_test_list.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 + +""" +This file provides utilities for gathering end-to-end test names +Supported formats include: + + a) A test name or list of test names. + b) A subdirectory name under test_configs/ + c) The name of a file (full or relative path) containing a list of test names. + d) "all" to run all tests +""" + +import os +import logging +import glob +from textwrap import dedent +from pathlib import Path + +TESTS_WE2E_DIR = Path(__file__).absolute().parents[2] / "tests" / "WE2E" + +CONFIG_YAML_GLOB = "test_configs/**/config*.yaml" + +ALL_TESTS = glob.glob(CONFIG_YAML_GLOB, recursive=True, root_dir=TESTS_WE2E_DIR) +TEST_DIRS = next(os.walk(os.path.join(TESTS_WE2E_DIR, "test_configs")))[1] + +TEST_FILE_PREFIX = "config." +TEST_FILE_SUFFIX = ".yaml" + +def get_pretty_list(test_list): + return "\n".join(str(x) for x in test_list) + +def get_tests_to_run(args_tests, names_only=False): + tests_to_check = _get_tests_to_check(args_tests) + tests_to_run = _check_tests(tests_to_check) + if names_only: + return _get_test_names(tests_to_run) + return tests_to_run + +def _get_tests_to_check(args_tests): + # 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: + logging.debug(f"User specified a list of tests:\n{args_tests}") + return args_tests + + # 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: + return user_spec_tests + + # 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 = _get_test_names(ALL_TESTS) + logging.debug(f"Will check all tests:\n{tests_to_check}") + return tests_to_check + + if user_spec_tests[0] in TEST_DIRS: + # 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{TEST_DIRS}" + ) + 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, + root_dir=TESTS_WE2E_DIR + ) + return _get_test_names(tests_in_dir) + + # 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 not Path(user_spec_tests[0]).is_file(): + 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. + """ + ) + ) + + # read file to get list of files to check + with open(user_spec_tests[0], encoding="utf-8") as f: + tests_to_check = [x.rstrip() for x in f] + return tests_to_check + +def _get_test_names(test_list) -> list[str]: + test_names = [] + for f in test_list: + 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 and filename.startswith(TEST_FILE_PREFIX) and filename.endswith(TEST_FILE_SUFFIX): + test_names.append(filename[7:-5]) + else: + logging.debug(f"Skipping non-test file {filename}") + + return test_names + +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 + """ + logging.info("Checking that all tests are valid") + + # Check that there are no duplicate test filenames + _check_for_duplicate_tests() + + 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() and 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" + ) + tests_to_run = list(set(tests_to_run)) + return tests_to_run + +def _check_for_duplicate_tests(): + testfilenames = [] + for testfile in ALL_TESTS: + testfile = Path(testfile) + if testfile.name in testfilenames: + duplicates = glob.glob(f"test_configs/**/{testfile.name}", recursive=True, root_dir=TESTS_WE2E_DIR) + 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) + +def _check_test(test: 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 file for input test name + test_config = f"{TEST_FILE_PREFIX}{test.strip()}{TEST_FILE_SUFFIX}" + config = "" + for testfile in ALL_TESTS: + if test_config in testfile: + logging.debug(f"found test {test}, testfile {testfile}") + config = TESTS_WE2E_DIR / Path(testfile) + return config