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 = []
@@ -374,6 +376,7 @@ def _prepare_test(case, root_dir: str) -> dict:  # pylint: disable=unused-argume
     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':
@@ -466,6 +469,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 +573,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"