update fre.make tests to avoid getcwd, and light pylint cleanup

README.md

@@ -69,7 +69,7 @@ conda activate fre-2026.01.alpha2
 # optional: install or load fre-nctools to gain access to regridding and certain time-averaging routines
 # add to your path like: export PATH=/path/to/your/fre-nctools/build/bin:$PATH
 # or if you have lmod/modules: module load fre-nctools/<version>
-# or compile/install from source: see github.comNOAA-GDFL/FRE-NCTools documentation on compilation/installation
+# or compile/install from source: see github.com/NOAA-GDFL/FRE-NCTools documentation on compilation/installation
 # DO NOT USE noaa-gfdl::fre-nctools==2022.01 at this time, it is being deprecated
 ```
 

fre/make/tests/compilation/test_run_fremake_builds.py

@@ -14,8 +14,9 @@
 
 from fre.make import run_fremake_script
 
-# command options
-YAMLDIR = "fre/make/tests/null_example"
+# command options — use __file__ so tests work from any working directory
+_TEST_DIR = Path(__file__).resolve().parent
+YAMLDIR = str(_TEST_DIR.parent / "null_example")
 YAMLFILE = "null_model.yaml"
 YAMLPATH = f"{YAMLDIR}/{YAMLFILE}"
 PLATFORM = [ "ci.gnu" ]
@@ -28,10 +29,9 @@
 # set up some paths for the tests to build in
 # the TEST_BUILD_DIR env var is used in the null model's platform.yaml
 # so the model root directory path can be changed
-currPath=os.getcwd()
-SERIAL_TEST_PATH=f"{currPath}/fre/make/tests/compilation/serial_build"
-MULTIJOB_TEST_PATH=f"{currPath}/fre/make/tests/compilation/multijob_build"
-CONTAINER_BUILD_TEST_PATH=f"{currPath}/fre/make/tests/compilation/container"
+SERIAL_TEST_PATH = str(_TEST_DIR / "serial_build")
+MULTIJOB_TEST_PATH = str(_TEST_DIR / "multijob_build")
+CONTAINER_BUILD_TEST_PATH = str(_TEST_DIR / "container")
 Path(SERIAL_TEST_PATH).mkdir(parents=True,exist_ok=True)
 Path(MULTIJOB_TEST_PATH).mkdir(parents=True,exist_ok=True)
 Path(CONTAINER_BUILD_TEST_PATH).mkdir(parents=True,exist_ok=True)
@@ -106,7 +106,7 @@ def test_run_fremake_container_build_notransfer():
 def test_run_fremake_cleanup():
     ''' removes directories created by the test and checks to make sure they're gone '''
     dirstrings = ["test_run_fremake_multijob", "test_run_fremake_serial", "test_run_fremake_multitarget"]
-    test_paths = [f"fre/make/tests/{el}/" for el in dirstrings]
+    test_paths = [str(_TEST_DIR.parent / el) for el in dirstrings]
     for tp in test_paths:
         try:
             rmtree(tp)
@@ -118,27 +118,30 @@ def test_run_fremake_cleanup():
 @pytest.mark.skipif(not has_podman, reason="missing podman")
 def test_run_fremake_container_build_fail():
     ''' check createContainer script would fail and exit if one step failed (incorrect Dockerfile name)'''
-    if Path(f"{currPath}/createContainer.sh").exists():
-        os.remove(f"{currPath}/createContainer.sh")
+    if Path("createContainer.sh").exists():
+        os.remove("createContainer.sh")
 
     # Create the createContainer.sh script but do not run
     run_fremake_script.fremake_run(YAMLPATH, CONTAINER_PLATFORM, TARGET,
         nparallel=1, makejobs=1, gitjobs=1, no_parallel_checkout=True,
         no_format_transfer=False, execute=False, verbose=VERBOSE)
-    assert Path(f"{currPath}/createContainer.sh").exists()
+    assert Path("createContainer.sh").exists()
 
     # Alter script to fail
     new_script = []
-    with open(Path(f"{currPath}/createContainer.sh"), "r") as f:
+    with open("createContainer.sh", "r", encoding='utf-8') as f:
         lines = f.readlines()
         for line in lines:
             new_script.append(line.replace("Dockerfile", "Dockerfile-wrong"))
 
-    with open(Path(f"{currPath}/createContainer.sh"), "w") as f2:
+    with open("createContainer.sh", "w", encoding='utf-8') as f2:
         f2.writelines(new_script)
 
     # Run altered script and compare error
-    run = subprocess.run(Path(f"{currPath}/createContainer.sh"), capture_output=True, check=False)
+    # Use a plain string "./createContainer.sh" (not a Path object) so the "./"
+    # prefix is preserved — Path("./x") normalises to PosixPath("x"), losing
+    # the directory separator and causing subprocess to search $PATH.
+    run = subprocess.run("./createContainer.sh", capture_output=True, check=False)
     stderr = run.stderr
 
     #Check that the incorrect line specifically prints in the stderr

fre/make/tests/conftest.py

@@ -0,0 +1,74 @@
+"""
+Shared pytest fixtures for fre.make tests.
+
+Provides common setup/teardown for test output directories and
+environment variables used across the make test suite.
+"""
+
+import shutil
+from pathlib import Path
+
+import pytest
+
+# Test directory paths — use __file__ so tests work from any working directory
+TEST_DIR = Path(__file__).resolve().parent
+
+@pytest.fixture
+def checkout_out(monkeypatch):
+    """
+    Provide a clean checkout output directory and set TEST_BUILD_DIR.
+
+    Ensures the output directory exists and is clean before each test,
+    and cleans up after. Uses monkeypatch to set TEST_BUILD_DIR so it
+    is automatically restored after the test.
+
+    Note: Only cleans the experiment subtree (not the whole OUT dir)
+    because checkout_create creates the full directory structure itself.
+    """
+    out = f"{TEST_DIR}/checkout_out"
+    monkeypatch.setenv("TEST_BUILD_DIR", out)
+
+    # Clean the experiment-specific subtree (not the whole out dir)
+    experiment_dir = Path(f"{out}/fremake_canopy/test")
+    if experiment_dir.exists():
+        shutil.rmtree(experiment_dir)
+
+    yield out
+
+    # Post-test cleanup of experiment subtree
+    if experiment_dir.exists():
+        shutil.rmtree(experiment_dir, ignore_errors=True)
+
+@pytest.fixture
+def compile_out(monkeypatch):
+    """
+    Provide a clean compile output directory and set TEST_BUILD_DIR.
+
+    Note: Recreates the entire OUT dir because compile_create expects
+    the base output directory to already exist.
+    """
+    out = f"{TEST_DIR}/compile_out"
+    monkeypatch.setenv("TEST_BUILD_DIR", out)
+
+    if Path(out).exists():
+        shutil.rmtree(out)
+    Path(out).mkdir(parents=True, exist_ok=True)
+
+    yield out
+
+@pytest.fixture
+def makefile_out(monkeypatch):
+    """
+    Provide a clean makefile output directory and set TEST_BUILD_DIR.
+
+    Note: Recreates the entire OUT dir because makefile_create expects
+    the base output directory to already exist.
+    """
+    out = f"{TEST_DIR}/makefile_out"
+    monkeypatch.setenv("TEST_BUILD_DIR", out)
+
+    if Path(out).exists():
+        shutil.rmtree(out)
+    Path(out).mkdir(parents=True, exist_ok=True)
+
+    yield out

fre/make/tests/test_create_checkout.py

@@ -1,12 +1,13 @@
 """
 Test fre make checkout-script
 """
-from pathlib import Path
 import shutil
+from pathlib import Path
+
 from fre.make import create_checkout_script
 
-# Set example yaml paths, input directory
-TEST_DIR = str(Path("fre/make/tests"))
+# Set example yaml paths, input directory — use __file__ so tests work from any cwd
+TEST_DIR = str(Path(__file__).resolve().parent)
 YAMLFILE = str(Path(f"{TEST_DIR}/null_example/null_model.yaml"))
 
 # Set platform and target
@@ -20,6 +21,9 @@
 # Set expected line that should be in checkout script
 EXPECTED_LINE = "git clone --recursive --jobs=2 https://github.com/NOAA-GFDL/FMS.git -b 2025.04 FMS"
 
+# Common path for checkout script and cloned source
+SRC_DIR = f"{OUT}/fremake_canopy/test/null_model_full/src"
+
 def test_nullyaml_exists():
     """
     Make sure combined yaml file exists
@@ -31,94 +35,84 @@ def test_nullyaml_filled():
     Make sure null.yaml is not an empty file
     """
     with open(YAMLFILE,'r') as f:
-        assert sum(1 for _ in f) >1
+        assert sum(1 for _ in f) > 1
 
-def test_checkout_script_exists(monkeypatch):
+def test_checkout_script_exists(checkout_out):
     """
     Test checkout-script was successful and that file exists.
     Also checks that the default behavior is a parallel checkout.
     """
-    # Set output directory as home for fre make output
-    monkeypatch.setenv("TEST_BUILD_DIR", OUT)
-
-    shutil.rmtree(f"{OUT}/fremake_canopy/test", ignore_errors=True)
     create_checkout_script.checkout_create(YAMLFILE,
                                            PLATFORM,
                                            TARGET,
                                            no_parallel_checkout = False,
                                            njobs = 2,
                                            execute = False,
                                            force_checkout = False)
-    #assert result.exit_code == 0
-    assert Path(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh").exists()
+    assert Path(f"{SRC_DIR}/checkout.sh").exists()
 
     # A parallel checkout is done by default - check for subshells (parallelism in script)
     expected_line = f"({EXPECTED_LINE}) &"
-    with open(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh", 'r') as f1:
+    with open(f"{SRC_DIR}/checkout.sh", 'r') as f1:
         content = f1.read()
     assert expected_line in content
 
-def test_checkout_execute(monkeypatch):
+def test_checkout_execute(checkout_out):
     """
-    check if --execute option works
-    """
-    # Set output directory as home for fre make output
-    monkeypatch.setenv("TEST_BUILD_DIR", OUT)
+    Check if --execute option works.
 
-    shutil.rmtree(f"{OUT}/fremake_canopy/test", ignore_errors=True)
+    Uses no_parallel_checkout to avoid shell-backgrounding race conditions
+    that can cause flaky failures. The parallel checkout syntax is already
+    covered by test_checkout_script_exists.
+    """
     create_checkout_script.checkout_create(YAMLFILE,
                                            PLATFORM,
                                            TARGET,
-                                           no_parallel_checkout = False,
+                                           no_parallel_checkout = True,
                                            njobs = 2,
                                            execute = True,
                                            force_checkout = False)
 
-    # Check for checkout script and for some resulting folders from running the script
-    assert all([Path(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh").exists(),
-                Path(f"{OUT}/fremake_canopy/test/null_model_full/src/FMS").is_dir(),
-                any(Path(f"{OUT}/fremake_canopy/test/null_model_full/src/FMS").iterdir()),
-                Path(f"{OUT}/fremake_canopy/test/null_model_full/src/coupler").is_dir(),
-                any(Path(f"{OUT}/fremake_canopy/test/null_model_full/src/coupler").iterdir())])
-
-def test_checkout_no_parallel_checkout(monkeypatch):
+    # Check for checkout script and for some resulting folders from running the script.
+    # Individual assertions give better diagnostics on failure than assert all([...]).
+    assert Path(f"{SRC_DIR}/checkout.sh").exists(), \
+        "checkout.sh was not created"
+    assert Path(f"{SRC_DIR}/FMS").is_dir(), \
+        "FMS directory was not created by checkout execution"
+    assert any(Path(f"{SRC_DIR}/FMS").iterdir()), \
+        "FMS directory is empty after checkout execution"
+    assert Path(f"{SRC_DIR}/coupler").is_dir(), \
+        "coupler directory was not created by checkout execution"
+    assert any(Path(f"{SRC_DIR}/coupler").iterdir()), \
+        "coupler directory is empty after checkout execution"
+
+def test_checkout_no_parallel_checkout(checkout_out):
     """
-    check if --no_parallel_checkout option works
+    Check if --no_parallel_checkout option works
     """
-    # Set output directory as home for fre make output
-    monkeypatch.setenv("TEST_BUILD_DIR", OUT)
-
-    shutil.rmtree(f"{OUT}/fremake_canopy/test", ignore_errors=True)
     create_checkout_script.checkout_create(YAMLFILE,
                                            PLATFORM,
                                            TARGET,
                                            no_parallel_checkout = True,
                                            njobs = 2,
                                            execute = False,
                                            force_checkout = False)
-    assert Path(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh").exists()
+    assert Path(f"{SRC_DIR}/checkout.sh").exists()
 
     expected_line = EXPECTED_LINE
-    with open(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh", 'r') as f:
+    with open(f"{SRC_DIR}/checkout.sh", 'r') as f:
         content = f.read()
 
-    assert all([expected_line in content,
-                "pids" not in content,
-                "&" not in content])
+    assert expected_line in content
+    assert "pids" not in content
+    assert "&" not in content
 
-def test_bm_checkout_force_checkout(caplog, monkeypatch):
+def test_bm_checkout_force_checkout(caplog, checkout_out):
     """
     Test re-creation of checkout script if --force-checkout is passed.
     """
-    # Set output directory as home for fre make output
-    monkeypatch.setenv("TEST_BUILD_DIR", OUT)
-
-    # Remove if previously created
-    shutil.rmtree(f"{OUT}/fremake_canopy/test", ignore_errors=True)
-
     ## Mock checkout script with some content we can check
-    # Create come checkout script
-    mock_checkout = Path(f"{OUT}/fremake_canopy/test/null_model_full/src")
+    mock_checkout = Path(f"{SRC_DIR}")
     mock_checkout.mkdir(parents = True)
 
     # Write checkout
@@ -141,20 +135,20 @@ def test_bm_checkout_force_checkout(caplog, monkeypatch):
                                            force_checkout = True)
 
     # Check it exists, check output, check content
-    assert all([Path(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh").exists(),
-                "Checkout script PREVIOUSLY created" in caplog.text,
-                "*** REMOVING CHECKOUT SCRIPT ***" in caplog.text,
-                "Checkout script created" in caplog.text])
+    assert Path(f"{SRC_DIR}/checkout.sh").exists()
+    assert "Checkout script PREVIOUSLY created" in caplog.text
+    assert "*** REMOVING CHECKOUT SCRIPT ***" in caplog.text
+    assert "Checkout script created" in caplog.text
 
     # Check one expected line is now populating the re-created checkout script
     expected_line = f"({EXPECTED_LINE}) &"
 
-    with open(f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh", 'r') as f2:
+    with open(f"{SRC_DIR}/checkout.sh", 'r') as f2:
         content = f2.read()
 
-    assert all([expected_line in content,
-                "pids" in content,
-                "mock bare-metal checkout content" not in content])
+    assert expected_line in content
+    assert "pids" in content
+    assert "mock bare-metal checkout content" not in content
 
 def test_container_checkout_force_checkout(caplog):
     """
@@ -165,7 +159,6 @@ def test_container_checkout_force_checkout(caplog):
     shutil.rmtree(f"./tmp/{CONTAINER_PLATFORM[0]}", ignore_errors=True)
 
     ## Mock checkout script with some content we can check
-    # Create come checkout script
     mock_checkout = Path(f"./tmp/{CONTAINER_PLATFORM[0]}")
     mock_checkout.mkdir(parents = True)
 
@@ -187,14 +180,11 @@ def test_container_checkout_force_checkout(caplog):
                                            execute = False,
                                            force_checkout = True)
 
-    # Check it mock checkout script exists
-    # Check output for script removal
-    # Check output for script creation
-    # Check content of re-created script
-    assert all([Path(f"{mock_checkout}/checkout.sh").exists(),
-                "Checkout script PREVIOUSLY created" in caplog.text,
-                "*** REMOVING CHECKOUT SCRIPT ***" in caplog.text,
-                "Checkout script created in ./tmp" in caplog.text])
+    # Check mock checkout script exists and output messages
+    assert Path(f"{mock_checkout}/checkout.sh").exists()
+    assert "Checkout script PREVIOUSLY created" in caplog.text
+    assert "*** REMOVING CHECKOUT SCRIPT ***" in caplog.text
+    assert "Checkout script created in ./tmp" in caplog.text
 
     # Check for an expected line that should be populating the re-created checkout script
     # Check no parenthesis (no parallel checkouts)
@@ -204,8 +194,8 @@ def test_container_checkout_force_checkout(caplog):
     with open(f"./tmp/{CONTAINER_PLATFORM[0]}/checkout.sh", "r") as f2:
         content = f2.read()
 
-    assert all([expected_line in content,
-               "pids" not in content,
-               "mock container checkout content" not in content])
+    assert expected_line in content
+    assert "pids" not in content
+    assert "mock container checkout content" not in content
 
 ##test checkout w/o force but if it already exists