Fix review findings: dir failure re-raise, FYPP_GCOV_OPTS init, dep_changed safety, SYSCHECK cleanup

- Re-raise OSError in _prepare_test after directory creation failure
  instead of silently continuing with a broken test directory
- Initialize FYPP_GCOV_OPTS to empty before the MFC_GCov block so
  non-gcov builds don't pass an undefined variable to Fypp
- Push dep_changed now defaults to true on git diff failure, matching
  the safe PR-path behavior
- Remove dead SYSCHECK references from test stubs and fix exec fallback
  replace string to match actual coverage.py imports
- Improve docstrings, comments, and cleanup error handling

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: sbryngelson

.github/workflows/test.yml

@@ -88,7 +88,11 @@ jobs:
               exit 0
             }
           elif [ "${{ github.event_name }}" = "push" ]; then
-            DIFF=$(git diff "$BEFORE".."$AFTER" 2>/dev/null || echo "")
+            DIFF=$(git diff "$BEFORE".."$AFTER" 2>/dev/null) || {
+              echo "git diff failed for push event — defaulting to dep_changed=true for safety."
+              echo "dep_changed=true" >> "$GITHUB_OUTPUT"
+              exit 0
+            }
           else
             DIFF=""
           fi

CMakeLists.txt

@@ -113,6 +113,8 @@ endif()
 # debug builds. These include optimization and debug flags, as well as some that
 # are required for a successful build of MFC.
 
+set(FYPP_GCOV_OPTS "")
+
 if (CMAKE_Fortran_COMPILER_ID STREQUAL "GNU")
     add_compile_options(
         $<$<COMPILE_LANGUAGE:Fortran>:-ffree-line-length-none>

toolchain/mfc/cli/commands.py

@@ -460,7 +460,7 @@
         ),
         Argument(
             name="build-coverage-cache",
-            help="Run all tests with gcov instrumentation to build the file-level coverage cache. Requires a prior --gcov build: ./mfc.sh build --gcov -j 8",
+            help="Run all tests with gcov instrumentation to build the file-level coverage cache. Pass --gcov to enable coverage instrumentation in the internal build step.",
             action=ArgAction.STORE_TRUE,
             default=False,
             dest="build_coverage_cache",

toolchain/mfc/test/coverage.py

@@ -8,9 +8,8 @@
 covered file set. Only tests that touch at least one changed file run.
 
 Workflow:
-    ./mfc.sh build --gcov -j 8              # one-time: build with coverage
-    ./mfc.sh test --build-coverage-cache    # one-time: populate the cache
-    ./mfc.sh test --only-changes -j 8       # fast: run only affected tests
+    ./mfc.sh test --build-coverage-cache --gcov -j 8  # one-time: populate the cache
+    ./mfc.sh test --only-changes -j 8                 # fast: run only affected tests
 """
 
 import io
@@ -234,6 +233,9 @@ def _collect_single_test_coverage(  # pylint: disable=too-many-locals
     """
     build_subdir = os.path.join(test_gcda, "build")
     if not os.path.isdir(build_subdir):
+        # No .gcda files produced — test may not have run or GCOV_PREFIX
+        # was misconfigured.  Return empty list; the sanity check at the end
+        # of build_coverage_cache will catch systemic failures.
         return uuid, []
 
     gcno_copies = []
@@ -367,13 +369,17 @@ def _prepare_test(case, root_dir: str) -> dict:  # pylint: disable=unused-argume
     Prepare a test for direct execution: create directory, generate .inp
     files, and resolve binary paths.  All Python/toolchain overhead happens
     here (single-threaded) so the parallel phase is pure subprocess calls.
+
+    Operates on a shallow copy of case.params to avoid mutating the
+    original case object.
     """
     try:
         case.delete_output()
         case.create_directory()
     except OSError as exc:
         cons.print(f"[yellow]Warning: Failed to prepare test directory for "
                    f"{case.get_uuid()}: {exc}[/yellow]")
+        raise
 
     # Lagrange bubble tests need input files generated before running.
     if case.params.get("bubbles_lagrange", 'F') == 'T':
@@ -383,35 +389,39 @@ def _prepare_test(case, root_dir: str) -> dict:  # pylint: disable=unused-argume
             cons.print(f"[yellow]Warning: Failed to generate Lagrange bubble input "
                        f"for {case.get_uuid()}: {exc}[/yellow]")
 
+    # Work on a copy so we don't permanently mutate the case object.
+    params = dict(case.params)
+
     # Apply post_process output params so simulation writes data files that
     # post_process reads.  Mirrors the generated case.py logic that normally
     # runs via ./mfc.sh run (see POST_PROCESS_OUTPUT_PARAMS in case.py).
-    case.params.update(get_post_process_mods(case.params))
+    params.update(get_post_process_mods(params))
 
     # Run only one timestep: we only need to know which source files are
     # *touched*, not verify correctness.  A single step exercises the key
     # code paths across all three executables while preventing heavy 3D tests
     # from timing out under gcov instrumentation (~10x slowdown).
-    case.params['t_step_stop'] = 1
+    params['t_step_stop'] = 1
 
     # Adaptive-dt tests: post_process computes n_save = int(t_stop/t_save)+1
     # and iterates over that many save indices.  But with small t_step_stop
     # the simulation produces far fewer saves.  Clamp t_stop so post_process
     # only reads saves that actually exist.
-    if case.params.get('cfl_adap_dt', 'F') == 'T':
-        t_save = float(case.params.get('t_save', 1.0))
-        case.params['t_stop'] = t_save  # n_save = 2: indices 0 and 1
+    if params.get('cfl_adap_dt', 'F') == 'T':
+        t_save = float(params.get('t_save', 1.0))
+        params['t_stop'] = t_save  # n_save = 2: indices 0 and 1
 
     # Heavy 3D tests: remove vorticity output (omega_wrt + fd_order) for
     # 3D QBMM tests.  Normal test execution never runs post_process (only
     # PRE_PROCESS + SIMULATION, never POST_PROCESS), so post_process on
     # heavy 3D configs is untested.  Vorticity FD computation on large grids
     # with many QBMM variables causes post_process to crash (exit code 2).
-    if (int(case.params.get('p', 0)) > 0 and
-            case.params.get('qbmm', 'F') == 'T'):
+    if (int(params.get('p', 0)) > 0 and
+            params.get('qbmm', 'F') == 'T'):
         for key in POST_PROCESS_3D_PARAMS:
-            case.params.pop(key, None)
+            params.pop(key, None)
 
+    case.params = params
     test_dir = case.get_dirpath()
     input_file = case.to_input_file()
 
@@ -466,6 +476,8 @@ def build_coverage_cache(  # pylint: disable=too-many-locals,too-many-statements
     strip = _compute_gcov_prefix_strip(root_dir)
 
     if n_jobs is None:
+        # Caller should pass n_jobs explicitly on SLURM systems;
+        # os.cpu_count() may exceed the SLURM allocation.
         n_jobs = max(os.cpu_count() or 1, 1)
     # Cap Phase 2 test parallelism: each test spawns gcov-instrumented MPI
     # processes (~2-5 GB each under gcov).  Too many concurrent tests cause OOM.
@@ -568,7 +580,11 @@ def build_coverage_cache(  # pylint: disable=too-many-locals,too-many-statements
                 if completed % 50 == 0 or completed == len(test_results):
                     cons.print(f"  [{completed:3d}/{len(test_results):3d}] tests processed")
     finally:
-        shutil.rmtree(gcda_dir, ignore_errors=True)
+        try:
+            shutil.rmtree(gcda_dir)
+        except OSError as exc:
+            cons.print(f"[yellow]Warning: Failed to clean up temp directory "
+                       f"{gcda_dir}: {exc}[/yellow]")
 
     # Sanity check: at least some tests should have non-empty coverage.
     tests_with_coverage = sum(1 for v in cache.values() if v)

toolchain/mfc/test/test_coverage_unit.py

@@ -68,7 +68,6 @@ class _FakeMFCException(Exception):
 _build_stub.PRE_PROCESS = "pre_process"
 _build_stub.SIMULATION = "simulation"
 _build_stub.POST_PROCESS = "post_process"
-_build_stub.SYSCHECK = "syscheck"
 
 _case_stub = sys.modules.get("toolchain.mfc.test.case", _make_stub("toolchain.mfc.test.case"))
 _case_stub.input_bubbles_lagrange = lambda case: None
@@ -123,7 +122,6 @@ class _FakeMFCException(Exception):
         "PRE_PROCESS": "pre_process",
         "SIMULATION": "simulation",
         "POST_PROCESS": "post_process",
-        "SYSCHECK": "syscheck",
     }
     with open(_COVERAGE_PATH, encoding="utf-8") as _f:
         _src = _f.read()
@@ -133,7 +131,7 @@ class _FakeMFCException(Exception):
         .replace("from ..printer import cons", "cons = _globals['cons']")
         .replace("from .. import common", "")
         .replace("from ..common import MFCException", "MFCException = _globals['MFCException']")
-        .replace("from ..build import PRE_PROCESS, SIMULATION, POST_PROCESS, SYSCHECK", "")
+        .replace("from ..build import PRE_PROCESS, SIMULATION, POST_PROCESS", "")
         .replace("from .case import (input_bubbles_lagrange, get_post_process_mods,\n"
                  "                    POST_PROCESS_3D_PARAMS)",
                  "input_bubbles_lagrange = lambda case: None\n"