refactor(cli): extract shared parse_and_validate_tc_params_file helper

Address Gemini code review comments on PR #96:

1. Eliminate duplicate file I/O and validation logic between
   validate_tc_params_file (validation.py) and load_tc_params_mapping
   (utils.py). Both functions were independently opening, parsing, and
   structurally validating the same mapping file.

2. Extract parse_and_validate_tc_params_file() into validation.py as the
   single source of truth for mapping-file I/O and structural checks.
   - validate_tc_params_file() now delegates to it and returns the
     resolved Path.
   - load_tc_params_mapping() imports and calls it instead of repeating
     the try/except file-reading block.

3. Add 7 tests for parse_and_validate_tc_params_file in
   test_tc_params_mapping.py, including a cross-function test that proves
   both callers produce identical error messages for the same bad file.

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

Author: rquidute

tests/test_tc_params_mapping.py

@@ -13,7 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-"""Unit tests for load_tc_params_mapping (utils) and validate_tc_params_file (validation)."""
+"""Unit tests for load_tc_params_mapping (utils), validate_tc_params_file, and
+parse_and_validate_tc_params_file (validation)."""
 
 import json
 from pathlib import Path
@@ -22,7 +23,7 @@
 
 from th_cli.exceptions import CLIError
 from th_cli.utils import load_tc_params_mapping
-from th_cli.validation import validate_tc_params_file
+from th_cli.validation import parse_and_validate_tc_params_file, validate_tc_params_file
 
 
 # ---------------------------------------------------------------------------
@@ -36,6 +37,72 @@ def _write_mapping(tmp_path: Path, data: dict, filename: str = "mapping.json") -
     return p
 
 
+# ---------------------------------------------------------------------------
+# parse_and_validate_tc_params_file  (shared helper)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+class TestParseAndValidateTcParamsFile:
+    """Tests for parse_and_validate_tc_params_file — the shared parsing helper.
+
+    Because both validate_tc_params_file and load_tc_params_mapping delegate
+    to this function, its error paths are exercised transitively by those
+    tests too.  These tests focus on the return value and the fact that the
+    helper is the single source of truth.
+    """
+
+    def test_returns_parsed_dict(self, tmp_path: Path) -> None:
+        """Helper returns the full parsed mapping as a dict."""
+        data = {"TC-ACE-1.1": {"int-arg": "PIXIT.ACE.EP:1"}}
+        p = _write_mapping(tmp_path, data)
+        result = parse_and_validate_tc_params_file(str(p))
+        assert result == data
+
+    def test_empty_mapping_returns_empty_dict(self, tmp_path: Path) -> None:
+        """An empty JSON object {} returns an empty dict without error."""
+        p = _write_mapping(tmp_path, {})
+        assert parse_and_validate_tc_params_file(str(p)) == {}
+
+    def test_file_not_found_raises(self, tmp_path: Path) -> None:
+        """Missing file raises CLIError."""
+        with pytest.raises(CLIError, match="File not found"):
+            parse_and_validate_tc_params_file(str(tmp_path / "missing.json"))
+
+    def test_invalid_json_raises(self, tmp_path: Path) -> None:
+        """Broken JSON raises CLIError with line/column info."""
+        p = tmp_path / "bad.json"
+        p.write_text("{bad", encoding="utf-8")
+        with pytest.raises(CLIError, match="Invalid JSON"):
+            parse_and_validate_tc_params_file(str(p))
+
+    def test_array_root_raises(self, tmp_path: Path) -> None:
+        """A JSON array at root raises CLIError."""
+        p = tmp_path / "arr.json"
+        p.write_text("[]", encoding="utf-8")
+        with pytest.raises(CLIError, match="Expected a JSON object at the top level"):
+            parse_and_validate_tc_params_file(str(p))
+
+    def test_non_dict_entry_value_raises(self, tmp_path: Path) -> None:
+        """A non-dict entry value raises CLIError naming the offending TC ID."""
+        p = _write_mapping(tmp_path, {"TC-ACE-1.1": "string-not-dict"})
+        with pytest.raises(CLIError, match="TC-ACE-1.1"):
+            parse_and_validate_tc_params_file(str(p))
+
+    def test_validate_and_load_share_same_errors(self, tmp_path: Path) -> None:
+        """validate_tc_params_file and load_tc_params_mapping raise the same
+        CLIError message for the same bad file — proving they share the helper."""
+        p = _write_mapping(tmp_path, {"TC-ACE-1.1": 42})
+
+        with pytest.raises(CLIError) as exc_validate:
+            validate_tc_params_file(str(p))
+
+        with pytest.raises(CLIError) as exc_load:
+            load_tc_params_mapping(str(p), ["TC-ACE-1.1"])
+
+        assert str(exc_validate.value) == str(exc_load.value)
+
+
 # ---------------------------------------------------------------------------
 # validate_tc_params_file
 # ---------------------------------------------------------------------------

th_cli/utils.py

@@ -29,6 +29,7 @@
 from th_cli.colorize import colorize_dump
 from th_cli.config import find_git_root, get_package_root
 from th_cli.exceptions import CLIError, handle_file_error
+from th_cli.validation import parse_and_validate_tc_params_file
 
 # Constants
 DEFAULT_FILE_ENCODING = "utf-8"
@@ -420,36 +421,15 @@ def load_tc_params_mapping(
     def _normalise(tc_id: str) -> str:
         return tc_id.replace("-", "_").replace(".", "_").upper()
 
-    try:
-        with open(mapping_path, "r", encoding=DEFAULT_FILE_ENCODING) as f:
-            raw = json.load(f)
-    except FileNotFoundError as e:
-        handle_file_error(e, "TC params mapping file")
-    except json.JSONDecodeError as e:
-        raise CLIError(
-            f"Invalid JSON in TC params mapping file '{mapping_path}': "
-            f"{e.msg} (line {e.lineno}, column {e.colno})"
-        )
-    except OSError as e:
-        raise CLIError(f"Failed to read TC params mapping file '{mapping_path}': {e}")
+    # Delegate file I/O and structural validation to the shared helper so the
+    # file is never parsed twice (validate_tc_params_file already calls it
+    # during pre-flight) and validation logic lives in one place.
+    raw = parse_and_validate_tc_params_file(mapping_path)
 
-    if not isinstance(raw, dict):
-        raise CLIError(
-            f"Invalid TC params mapping file '{mapping_path}': "
-            f"Expected a JSON object at the top level, got {type(raw).__name__}"
-        )
-
-    # Validate that all values are dicts (params dicts), and build a
-    # normalised-key → original-value lookup.
-    normalised_mapping: dict[str, dict[str, Any]] = {}
-    for key, value in raw.items():
-        if not isinstance(value, dict):
-            raise CLIError(
-                f"Invalid TC params mapping file '{mapping_path}': "
-                f"Value for TC ID '{key}' must be a JSON object (dict of parameters), "
-                f"got {type(value).__name__}"
-            )
-        normalised_mapping[_normalise(key)] = value
+    # Build a normalised-key → params lookup.
+    normalised_mapping: dict[str, dict[str, Any]] = {
+        _normalise(key): value for key, value in raw.items()
+    }
 
     # Match each requested test ID against the normalised mapping.
     merged_params: dict[str, Any] = {}

th_cli/validation.py

@@ -113,25 +113,20 @@ def validate_test_ids(test_ids: str) -> list[str]:
     return ids
 
 
-def validate_tc_params_file(file_path: str) -> Path:
-    """Validate that a TC params mapping file exists and has the expected structure.
+def parse_and_validate_tc_params_file(file_path: str) -> dict[str, dict[str, Any]]:
+    """Read, parse, and validate a TC params mapping file, returning its contents.
 
-    Performs two levels of checking:
-
-    1. **File-level**: path exists, is a regular file, and is readable.
-    2. **Format-level**: the file contains valid JSON whose top-level value is
-       a JSON object (dict), and whose values are themselves JSON objects.
-
-    This is a *fast pre-flight* check intended to surface obvious problems
-    before the full run starts.  Deep semantic validation (e.g. checking
-    parameter key names or value formats) is left to
-    ``load_tc_params_mapping`` at load time.
+    This is the single source of truth for mapping-file I/O and structural
+    validation.  Both :func:`validate_tc_params_file` (pre-flight check) and
+    ``load_tc_params_mapping`` (runtime loader) delegate here so the file is
+    never parsed twice and validation logic lives in one place.
 
     Args:
         file_path: Path to the JSON mapping file.
 
     Returns:
-        Resolved ``Path`` object for the validated file.
+        The parsed mapping as a ``dict[str, dict[str, Any]]`` — TC ID keys
+        mapping to their parameter dicts.
 
     Raises:
         CLIError: If the file is missing, not a regular file, contains
@@ -164,7 +159,35 @@ def validate_tc_params_file(file_path: str) -> Path:
             f"Non-dict values found for TC IDs: {', '.join(bad_entries)}"
         )
 
-    return path
+    return data
+
+
+def validate_tc_params_file(file_path: str) -> Path:
+    """Validate that a TC params mapping file exists and has the expected structure.
+
+    Performs two levels of checking:
+
+    1. **File-level**: path exists, is a regular file, and is readable.
+    2. **Format-level**: the file contains valid JSON whose top-level value is
+       a JSON object (dict), and whose values are themselves JSON objects.
+
+    This is a *fast pre-flight* check intended to surface obvious problems
+    before the full run starts.  The actual parsing is delegated to
+    :func:`parse_and_validate_tc_params_file` so the logic is not duplicated
+    with ``load_tc_params_mapping``.
+
+    Args:
+        file_path: Path to the JSON mapping file.
+
+    Returns:
+        Resolved ``Path`` object for the validated file.
+
+    Raises:
+        CLIError: If the file is missing, not a regular file, contains
+            invalid JSON, or does not match the expected top-level structure.
+    """
+    parse_and_validate_tc_params_file(file_path)
+    return Path(file_path).resolve()
 
 
 def validate_hostname(hostname: str) -> str: