From 09f0c076bebf8ef06d971e9bc8b342a4aa0ddd69 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sun, 22 Mar 2026 22:29:44 +0100 Subject: [PATCH 01/28] fix(pyats): prevent credential exposure in PyATS archives (#689) Disable EnvironmentDebugPlugin which was writing environment variables (including passwords) to env.txt files in PyATS result archives. Add E2E regression test that scans all output artifacts (including ZIP contents) for a sentinel password value to catch future credential leaks. --- .../pyats_core/execution/subprocess_runner.py | 4 ++ tests/e2e/conftest.py | 8 +++- tests/e2e/test_e2e_scenarios.py | 41 ++++++++++++++++++- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py index 173d6e65..003092f0 100644 --- a/nac_test/pyats_core/execution/subprocess_runner.py +++ b/nac_test/pyats_core/execution/subprocess_runner.py @@ -141,6 +141,8 @@ async def execute_job( enabled: True module: nac_test.pyats_core.progress.plugin order: 1.0 + EnvironmentDebugPlugin: + enabled: False """) with tempfile.NamedTemporaryFile( @@ -249,6 +251,8 @@ async def execute_job_with_testbed( enabled: True module: nac_test.pyats_core.progress.plugin order: 1.0 + EnvironmentDebugPlugin: + enabled: False """) with tempfile.NamedTemporaryFile( diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index da978c7f..09ea8772 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -42,6 +42,10 @@ ) from tests.e2e.mocks.mock_server import MockAPIServer +# Sentinel value for credential exposure detection (#689) +# All test passwords use this value so we can detect if credentials leak into artifacts +TEST_CREDENTIAL_SENTINEL = "CRED_SENTINEL_MUST_NOT_APPEAR_IN_ARTIFACTS" + @dataclass class E2EResults: @@ -224,10 +228,10 @@ def _run_e2e_scenario( else: class_mocker.setenv(f"{arch}_URL", "http://dry-run.invalid") class_mocker.setenv(f"{arch}_USERNAME", "mock_user") - class_mocker.setenv(f"{arch}_PASSWORD", "mock_pass") + class_mocker.setenv(f"{arch}_PASSWORD", TEST_CREDENTIAL_SENTINEL) # IOSXE credentials needed for D2D tests (device access) class_mocker.setenv("IOSXE_USERNAME", "mock_user") - class_mocker.setenv("IOSXE_PASSWORD", "mock_pass") + class_mocker.setenv("IOSXE_PASSWORD", TEST_CREDENTIAL_SENTINEL) if extra_env_vars: for key, value in extra_env_vars.items(): diff --git a/tests/e2e/test_e2e_scenarios.py b/tests/e2e/test_e2e_scenarios.py index 03178f7a..6da15ca4 100644 --- a/tests/e2e/test_e2e_scenarios.py +++ b/tests/e2e/test_e2e_scenarios.py @@ -20,6 +20,7 @@ import logging import re import xml.etree.ElementTree as ET +import zipfile import pytest @@ -39,7 +40,7 @@ ) from nac_test.robot.reporting.robot_output_parser import RobotResultParser from tests.conftest import assert_is_link_to -from tests.e2e.conftest import E2EResults +from tests.e2e.conftest import TEST_CREDENTIAL_SENTINEL, E2EResults from tests.e2e.html_helpers import ( assert_combined_stats, assert_report_stats, @@ -950,6 +951,44 @@ def test_stdout_combined_summary_has_visual_spacing( f"Content after separator starts with: {repr(after_closing[:20])}" ) + # ------------------------------------------------------------------------- + # Security Tests (#689 - Credential Exposure Prevention) + # ------------------------------------------------------------------------- + + def test_no_credentials_in_output_artifacts(self, results: E2EResults) -> None: + """Verify that credentials don't appear in any output artifacts. + + Regression test for #689 - EnvironmentDebugPlugin was exposing env vars + including passwords in env.txt files within PyATS archives. + """ + offending_files: list[str] = [] + + for file_path in results.output_dir.rglob("*"): + if not file_path.is_file(): + continue + + if file_path.suffix.lower() == ".zip": + try: + with zipfile.ZipFile(file_path, "r") as zf: + for name in zf.namelist(): + content = zf.read(name).decode("utf-8", errors="ignore") + if TEST_CREDENTIAL_SENTINEL in content: + offending_files.append(f"{file_path}!{name}") + except zipfile.BadZipFile: + pass # Not a valid zip file, skip + else: + # Check all files as text - errors="ignore" safely handles binary + # files by dropping undecodable bytes while preserving readable text. + # No try/except needed: we control output_dir, files won't disappear. + content = file_path.read_text(errors="ignore") + if TEST_CREDENTIAL_SENTINEL in content: + offending_files.append(str(file_path)) + + assert not offending_files, ( + "Credential sentinel found in output artifacts:\n" + + "\n".join(f" - {f}" for f in offending_files) + ) + # ============================================================================= # SUCCESS SCENARIO TESTS From dac1aa11acb15c7f95d54ba1bc8caaf878e21819 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Mon, 23 Mar 2026 06:28:52 +0100 Subject: [PATCH 02/28] fix(security): auto-cleanup merged_data_model_test_variables.yaml (#677) Automatically delete merged_data_model_test_variables.yaml after test execution to prevent potential credential exposure in shared environments. Changes: - Add CleanupManager singleton for registering files to delete on exit - Register merged data file for cleanup immediately after creation - Remove duplicate data merging in RobotWriter (CLI already merges) - Simplify RobotWriter to accept merged_data_path instead of data_paths - Handle cleanup on normal exit (atexit) and SIGTERM signals The merged data file contains sensitive test variables that should not persist after test execution completes. Closes #677 --- README.md | 12 +- dev-docs/PRD_AND_ARCHITECTURE.md | 16 +- nac_test/cli/main.py | 59 ++--- nac_test/combined_orchestrator.py | 6 - nac_test/core/constants.py | 1 + nac_test/data_merger.py | 2 +- .../execution/device/device_executor.py | 4 +- nac_test/pyats_core/orchestrator.py | 5 +- nac_test/robot/orchestrator.py | 19 +- nac_test/robot/robot_writer.py | 20 +- nac_test/utils/cleanup.py | 150 ++++++++++++ tests/e2e/fixtures/success/data.yaml | 1 + .../test_controller_detection_integration.py | 1 - .../test_orchestrator_controller_detection.py | 5 - .../test_orchestrator_controller_param.py | 4 - tests/pyats_core/test_orchestrator_dry_run.py | 4 - tests/pyats_core/test_orchestrator_env_var.py | 1 - tests/unit/robot/test_orchestrator.py | 57 +---- .../test_combined_orchestrator_controller.py | 8 - tests/unit/test_combined_orchestrator_flow.py | 8 - .../test_combined_orchestrator_python311.py | 1 - tests/unit/utils/test_cleanup.py | 230 ++++++++++++++++++ 22 files changed, 438 insertions(+), 176 deletions(-) create mode 100644 tests/unit/utils/test_cleanup.py diff --git a/README.md b/README.md index 147aff4a..b517d120 100644 --- a/README.md +++ b/README.md @@ -58,8 +58,6 @@ $ nac-test --help │ --verbose Enables verbose output for nac-test, │ │ Robot and PyATS execution. | │ [env var: NAC_TEST_VERBOSE] │ -│ --merged-data-file… -m TEXT Filename for merged data model. │ -│ [default: merged_data_model_test...] │ │ --loglevel -l [DEBUG|...] Log level. [default: WARNING] │ │ --version Display version number. │ │ --help Show this message and exit. │ @@ -372,17 +370,9 @@ Before test execution, `nac-test` merges all YAML data files into a single data 1. All files from `--data` paths are recursively loaded 2. YAML structures are deep-merged (later files override earlier ones) -3. The merged result is written to the output directory +3. The merged result is written to the output directory as `merged_data_model_test_variables.yaml` 4. Both Robot and PyATS tests reference this merged data -### Custom Filename - -By default, the merged file is named `merged_data_model_test_variables.yaml`. You can customize this: - -```bash -nac-test -d ./data -t ./tests -o ./output -m my_custom_data.yaml -``` - ### Accessing the Merged Data The merged data model is available to: diff --git a/dev-docs/PRD_AND_ARCHITECTURE.md b/dev-docs/PRD_AND_ARCHITECTURE.md index 8b215a69..8154c28e 100644 --- a/dev-docs/PRD_AND_ARCHITECTURE.md +++ b/dev-docs/PRD_AND_ARCHITECTURE.md @@ -842,20 +842,6 @@ nac-test provides 13 CLI flags covering data input, template configuration, exec nac-test -d config/ -t templates/ --tests custom_tests/ -o output/ ``` -**`-m, --merged-data-filename`** (OPTIONAL) -- **Type**: `str` -- **Default**: `"merged_data_model_test_variables.yaml"` -- **Environment Variable**: None (CLI only) -- **Purpose**: Custom filename for merged data model YAML (SOT shared across frameworks) -- **Behavior**: - - Written to `{output_dir}/{merged_data_filename}` - - Passed to CombinedOrchestrator constructor - - Both PyATS and Robot read from this shared file -- **Example**: - ```bash - nac-test -d config/ -t templates/ -o output/ -m merged_data_prod.yaml - ``` - ##### Execution Control Flags **`-i, --include`** (OPTIONAL) @@ -1296,7 +1282,7 @@ exit() # Checks error_handler.fired for exit code #### Environment Variable Support -Every CLI flag (except `--version` and `--merged-data-filename`) supports environment variable configuration for CI/CD and containerized environments. +Every CLI flag (except `--version`) supports environment variable configuration for CI/CD and containerized environments. **Environment Variable Mapping Table**: diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index e1aba840..e3d1f0fb 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -4,9 +4,11 @@ """CLI entry point for nac-test.""" import logging +import os +import stat from datetime import datetime from pathlib import Path -from typing import Annotated +from typing import Annotated, NoReturn import typer @@ -17,11 +19,13 @@ from nac_test.combined_orchestrator import CombinedOrchestrator from nac_test.core.constants import ( DEBUG_MODE, + DEFAULT_MERGED_DATA_FILENAME, EXIT_ERROR, EXIT_INTERRUPTED, EXIT_INVALID_ARGS, ) from nac_test.data_merger import DataMerger +from nac_test.utils.cleanup import get_cleanup_manager from nac_test.utils.formatting import format_duration from nac_test.utils.logging import ( DEFAULT_LOGLEVEL, @@ -158,16 +162,6 @@ def version_callback(value: bool) -> None: ] -MergedDataFilename = Annotated[ - str, - typer.Option( - "-m", - "--merged-data-filename", - help="Filename for the merged data model YAML file.", - ), -] - - RenderOnly = Annotated[ bool, typer.Option( @@ -310,7 +304,6 @@ def main( version: Version = False, # noqa: ARG001 diagnostic: Diagnostic = False, # noqa: ARG001 verbose: Verbose = False, - merged_data_filename: MergedDataFilename = "merged_data_model_test_variables.yaml", ) -> None: """A CLI tool to render and execute Robot Framework and PyATS tests using Jinja templating. @@ -372,18 +365,31 @@ def main( typer.echo("\n\n📄 Merging data model files...") merged_data = DataMerger.merge_data_files(data) - DataMerger.write_merged_data_model(merged_data, output, merged_data_filename) + DataMerger.write_merged_data_model( + merged_data, output, DEFAULT_MERGED_DATA_FILENAME + ) + + merged_data_path = output / DEFAULT_MERGED_DATA_FILENAME + if os.name != "nt": + os.chmod(merged_data_path, stat.S_IRUSR | stat.S_IWUSR) + + # Register merged data file for cleanup on exit/signal (SIGTERM/SIGINT) + cleanup_manager = get_cleanup_manager() + cleanup_manager.register(merged_data_path) duration = (datetime.now() - start_time).total_seconds() typer.echo(f"✅ Data model merging completed ({format_duration(duration)})") + def exit_with_cleanup(code: int) -> NoReturn: + cleanup_manager.cleanup_now() + raise typer.Exit(code) + # CombinedOrchestrator - handles both dev and production modes (uses pre-created merged data) orchestrator = CombinedOrchestrator( data_paths=data, templates_dir=templates, custom_testbed_path=testbed, output_dir=output, - merged_data_filename=merged_data_filename, filters_path=filters, tests_path=tests, include_tags=include, @@ -406,22 +412,19 @@ def main( try: stats = orchestrator.run_tests() except KeyboardInterrupt: - # Handle Ctrl+C interruption gracefully typer.echo( typer.style( "\n⚠️ Test execution was interrupted by user (Ctrl+C)", fg=typer.colors.YELLOW, ) ) - # Exit with code 253 following Robot Framework convention - raise typer.Exit(EXIT_INTERRUPTED) from None + exit_with_cleanup(EXIT_INTERRUPTED) except Exception as e: - # Infrastructure errors (template rendering, controller detection, etc.) typer.echo(f"Error during execution: {e}") - # Progressive disclosure: clean output for customers, full context for developers if DEBUG_MODE: - raise typer.Exit(EXIT_ERROR) from e # Developer: full exception context - raise typer.Exit(EXIT_ERROR) from None # Customer: clean output + cleanup_manager.cleanup_now() + raise typer.Exit(EXIT_ERROR) from e + exit_with_cleanup(EXIT_ERROR) # Display total runtime before exit runtime_end = datetime.now() @@ -436,9 +439,9 @@ def main( typer.echo(f"\n❌ Template rendering failed: {reason}", err=True) else: typer.echo("\n❌ Template rendering failed", err=True) - raise typer.Exit(stats.exit_code) + exit_with_cleanup(stats.exit_code) typer.echo("\n✅ Templates rendered successfully (render-only mode)") - raise typer.Exit(0) + exit_with_cleanup(0) if stats.pre_flight_failure is not None: pf = stats.pre_flight_failure @@ -446,22 +449,22 @@ def main( f"\n❌ Pre-flight failure ({pf.failure_type.display_name})", err=True, ) - raise typer.Exit(stats.exit_code) + exit_with_cleanup(stats.exit_code) if stats.has_errors: error_list = "; ".join(stats.errors) typer.echo(f"\n❌ Execution errors: {error_list}", err=True) - raise typer.Exit(stats.exit_code) + exit_with_cleanup(stats.exit_code) if stats.has_failures: typer.echo( f"\n❌ Tests failed: {stats.failed} out of {stats.total} tests", err=True ) - raise typer.Exit(stats.exit_code) + exit_with_cleanup(stats.exit_code) if stats.is_empty: typer.echo("\n⚠️ No tests were executed", err=True) - raise typer.Exit(stats.exit_code) + exit_with_cleanup(stats.exit_code) typer.echo(f"\n✅ All tests passed: {stats.passed} out of {stats.total} tests") - raise typer.Exit(0) + exit_with_cleanup(0) diff --git a/nac_test/combined_orchestrator.py b/nac_test/combined_orchestrator.py index 415c0922..8ccedfaf 100644 --- a/nac_test/combined_orchestrator.py +++ b/nac_test/combined_orchestrator.py @@ -69,7 +69,6 @@ def __init__( data_paths: list[Path], templates_dir: Path, output_dir: Path, - merged_data_filename: str, filters_path: Path | None = None, tests_path: Path | None = None, include_tags: list[str] | None = None, @@ -92,7 +91,6 @@ def __init__( data_paths: List of paths to data model YAML files templates_dir: Directory containing test templates and PyATS test files output_dir: Base directory for test output - merged_data_filename: Name of the merged data model file filters_path: Path to Jinja filters (Robot only) tests_path: Path to Jinja tests (Robot only) include_tags: Tags to include (Robot only) @@ -112,7 +110,6 @@ def __init__( self.data_paths = data_paths self.templates_dir = Path(templates_dir) self.output_dir = Path(output_dir) - self.merged_data_filename = merged_data_filename # Robot-specific parameters self.filters_path = filters_path @@ -205,7 +202,6 @@ def run_tests(self) -> CombinedResults: data_paths=self.data_paths, test_dir=self.templates_dir, output_dir=self.output_dir, - merged_data_filename=self.merged_data_filename, minimal_reports=self.minimal_reports, custom_testbed_path=self.custom_testbed_path, controller_type=self.controller_type, @@ -225,10 +221,8 @@ def run_tests(self) -> CombinedResults: typer.echo(f"\n🤖 Running Robot Framework tests{mode_suffix}...\n") robot_orchestrator = RobotOrchestrator( - data_paths=self.data_paths, templates_dir=self.templates_dir, output_dir=self.output_dir, - merged_data_filename=self.merged_data_filename, filters_path=self.filters_path, tests_path=self.tests_path, include_tags=self.include_tags, diff --git a/nac_test/core/constants.py b/nac_test/core/constants.py index a51d916a..640b2916 100644 --- a/nac_test/core/constants.py +++ b/nac_test/core/constants.py @@ -84,6 +84,7 @@ REPORT_HTML: str = "report.html" XUNIT_XML: str = "xunit.xml" ORDERING_FILENAME: str = "ordering.txt" +DEFAULT_MERGED_DATA_FILENAME: str = "merged_data_model_test_variables.yaml" SUMMARY_SEPARATOR_WIDTH: int = 70 # HTTP status code range boundaries diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index 157def6c..fec999ef 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -36,7 +36,7 @@ def merge_data_files(data_paths: list[Path]) -> dict[str, Any]: def write_merged_data_model( data: dict[str, Any], output_directory: Path, - filename: str = "merged_data_model_test_variables.yaml", + filename: str, ) -> None: """Write merged data model to YAML file. diff --git a/nac_test/pyats_core/execution/device/device_executor.py b/nac_test/pyats_core/execution/device/device_executor.py index eb138aaa..426cc21b 100644 --- a/nac_test/pyats_core/execution/device/device_executor.py +++ b/nac_test/pyats_core/execution/device/device_executor.py @@ -11,6 +11,7 @@ from pathlib import Path from typing import Any +from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME from nac_test.pyats_core.execution.job_generator import JobGenerator from nac_test.pyats_core.execution.subprocess_runner import SubprocessRunner @@ -104,8 +105,7 @@ async def run_device_job_with_semaphore( "HOSTNAME": hostname, "DEVICE_INFO": json.dumps(device), "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH": str( - self.base_output_dir - / "merged_data_model_test_variables.yaml" + self.base_output_dir / DEFAULT_MERGED_DATA_FILENAME ), "NAC_TEST_TYPE": "d2d", } diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py index 2f2ba6aa..d32d20fb 100644 --- a/nac_test/pyats_core/orchestrator.py +++ b/nac_test/pyats_core/orchestrator.py @@ -15,6 +15,7 @@ from nac_test.core.constants import ( DEBUG_MODE, + DEFAULT_MERGED_DATA_FILENAME, DRY_RUN_REASON, EXIT_ERROR, PYATS_RESULTS_DIRNAME, @@ -66,7 +67,6 @@ def __init__( data_paths: list[Path], test_dir: Path, output_dir: Path, - merged_data_filename: str, minimal_reports: bool = False, custom_testbed_path: Path | None = None, controller_type: str | None = None, @@ -80,7 +80,6 @@ def __init__( data_paths: List of paths to data model YAML files test_dir: Directory containing PyATS test files output_dir: Base output directory (orchestrator creates pyats_results subdirectory) - merged_data_filename: Name of the merged data model file minimal_reports: Only include command outputs for failed/errored tests in reports custom_testbed_path: Path to custom PyATS testbed YAML for device overrides controller_type: The detected controller type (e.g., "ACI", "SDWAN", "CC"). @@ -97,7 +96,7 @@ def __init__( self.output_dir = ( self.base_output_dir / PYATS_RESULTS_DIRNAME ) # PyATS works in its own subdirectory - self.merged_data_filename = merged_data_filename + self.merged_data_filename = DEFAULT_MERGED_DATA_FILENAME self.minimal_reports = minimal_reports self.custom_testbed_path = custom_testbed_path self.dry_run = dry_run diff --git a/nac_test/robot/orchestrator.py b/nac_test/robot/orchestrator.py index d88e9591..a958f53a 100644 --- a/nac_test/robot/orchestrator.py +++ b/nac_test/robot/orchestrator.py @@ -14,6 +14,7 @@ import typer from nac_test.core.constants import ( + DEFAULT_MERGED_DATA_FILENAME, DISABLE_TESTLEVELSPLIT, EXIT_DATA_ERROR, EXIT_ERROR, @@ -46,10 +47,8 @@ class RobotOrchestrator: def __init__( self, - data_paths: list[Path], templates_dir: Path, output_dir: Path, - merged_data_filename: str, filters_path: Path | None = None, tests_path: Path | None = None, include_tags: list[str] | None = None, @@ -64,10 +63,8 @@ def __init__( """Initialize the Robot Framework orchestrator. Args: - data_paths: List of paths to data model YAML files templates_dir: Directory containing Robot template files output_dir: Base output directory (orchestrator uses its robot_results subdirectory) - merged_data_filename: Name of the merged data model file filters_path: Optional path to filter files tests_path: Optional path to test files include_tags: Optional list of tags to include @@ -79,11 +76,10 @@ def __init__( loglevel: Log level verbose: Enable verbose mode - enables verbose output for pabot """ - self.data_paths = data_paths self.templates_dir = Path(templates_dir) self.base_output_dir = Path(output_dir) self.output_dir = self.base_output_dir / ROBOT_RESULTS_DIRNAME - self.merged_data_filename = merged_data_filename + self.merged_data_path = self.base_output_dir / DEFAULT_MERGED_DATA_FILENAME # Robot-specific parameters self.filters_path = filters_path @@ -105,7 +101,7 @@ def __init__( # Initialize Robot Framework components (reuse existing implementations) self.robot_writer = RobotWriter( - data_paths=self.data_paths, + merged_data_path=self.merged_data_path, filters_path=self.filters_path, tests_path=self.tests_path, include_tags=self.include_tags, @@ -145,14 +141,7 @@ def run_tests(self) -> TestResults: self.templates_dir, self.output_dir, ordering_file=self.ordering_file ) - # Phase 2: Create merged data model in Robot working directory - # Note: Robot tests expect the merged data file in their working directory - logger.info("Creating merged data model for Robot tests") - self.robot_writer.write_merged_data_model( - self.output_dir, self.merged_data_filename - ) - - # Phase 3: Test execution (unless render-only mode) + # Phase 2: Test execution (unless render-only mode) if not self.render_only: typer.echo("🤖 Executing Robot Framework tests...\n\n") robot_loglevel = "DEBUG" if self.loglevel == LogLevel.DEBUG else None diff --git a/nac_test/robot/robot_writer.py b/nac_test/robot/robot_writer.py index f0178634..80bbf7a2 100644 --- a/nac_test/robot/robot_writer.py +++ b/nac_test/robot/robot_writer.py @@ -61,13 +61,13 @@ def start_test(self, test: Any) -> None: class RobotWriter: def __init__( self, - data_paths: list[Path], + merged_data_path: Path, filters_path: Path | None, tests_path: Path | None, include_tags: list[str] | None = None, exclude_tags: list[str] | None = None, ) -> None: - self.data = DataMerger.merge_data_files(data_paths) + self.data = DataMerger.load_yaml_file(merged_data_path) # Convert OrderedDict to dict once during initialization instead of per-template # This eliminates expensive JSON round-trip serialization for each template render self.template_data = self._convert_data_model_for_templates(self.data) @@ -543,19 +543,3 @@ def write( else: # ensure we clean out a leftover ordering file if we don't want testlevelsplit ordering_file.unlink(missing_ok=True) - - def write_merged_data_model( - self, - output_directory: Path, - filename: str = "merged_data_model_test_variables.yaml", - ) -> None: - """Writes the merged data model to a specified YAML file. - - This method takes the internal, merged data dictionary (`self.data`) - and writes it to a YAML file in the specified output directory. - - Args: - output_directory: The directory where the YAML file will be saved. - filename: The name of the output YAML file. - """ - DataMerger.write_merged_data_model(self.data, output_directory, filename) diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index adfbf09d..1932f002 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -3,16 +3,166 @@ """Cleanup utilities for nac-test framework.""" +import atexit import logging import shutil +import signal +import sys +import threading import time from pathlib import Path +from types import FrameType +from typing import Any from nac_test.pyats_core.discovery.test_type_resolver import VALID_TEST_TYPES logger = logging.getLogger(__name__) +class CleanupManager: + """Thread-safe manager for registering files to be cleaned up on exit. + + Ensures registered files are removed on: + - Normal program exit (atexit) + - SIGTERM signal (e.g., docker stop, kill) + - SIGINT signal (Ctrl+C / KeyboardInterrupt) + + Note: SIGKILL cannot be caught - files may remain if process is killed with -9. + + Usage: + cleanup_manager = CleanupManager() + cleanup_manager.register(Path("/tmp/sensitive_file.yaml")) + # File will be automatically removed on exit + + Thread Safety: + All operations are thread-safe via internal locking. + """ + + _instance: "CleanupManager | None" = None + _instance_lock = threading.Lock() + _initialized: bool = False + + def __new__(cls) -> "CleanupManager": + """Singleton pattern - only one cleanup manager per process.""" + with cls._instance_lock: + if cls._instance is None: + cls._instance = super().__new__(cls) + cls._instance._initialized = False + return cls._instance + + def __init__(self) -> None: + """Initialize the cleanup manager (only runs once due to singleton).""" + if self._initialized: + return + + self._files: set[Path] = set() + self._lock = threading.Lock() + self._original_sigterm: Any = None + self._original_sigint: Any = None + self._cleanup_done = False + + # Register atexit handler + atexit.register(self._cleanup) + + # Install signal handlers (Unix only - Windows doesn't support SIGTERM properly) + if sys.platform != "win32": + self._install_signal_handlers() + + self._initialized = True + logger.debug("CleanupManager initialized") + + def _install_signal_handlers(self) -> None: + """Install signal handlers for SIGTERM and SIGINT.""" + try: + self._original_sigterm = signal.signal(signal.SIGTERM, self._signal_handler) + self._original_sigint = signal.signal(signal.SIGINT, self._signal_handler) + logger.debug("Signal handlers installed for SIGTERM and SIGINT") + except (ValueError, OSError) as e: + # Can fail if not in main thread or signal not supported + logger.debug(f"Could not install signal handlers: {e}") + + def _signal_handler(self, signum: int, frame: FrameType | None) -> None: + """Handle SIGTERM/SIGINT by cleaning up and re-raising.""" + sig_name = signal.Signals(signum).name + logger.debug(f"Received {sig_name}, performing cleanup") + + # Perform cleanup + self._cleanup() + + # Re-raise the signal with original handler or default behavior + if signum == signal.SIGTERM: + original = self._original_sigterm + else: + original = self._original_sigint + + # Restore original handler and re-raise + signal.signal(signum, original if original else signal.SIG_DFL) + + # For SIGINT, raise KeyboardInterrupt to allow normal exception handling + if signum == signal.SIGINT: + raise KeyboardInterrupt + + # For SIGTERM, re-send the signal + signal.raise_signal(signum) + + def register(self, path: Path) -> None: + """Register a file path for cleanup on exit. + + Args: + path: Path to file that should be removed on exit. + Directories are not supported (use shutil.rmtree directly). + """ + with self._lock: + resolved = path.resolve() + self._files.add(resolved) + logger.debug(f"Registered for cleanup: {resolved}") + + def unregister(self, path: Path) -> None: + """Unregister a file path from cleanup. + + Args: + path: Path to file that should no longer be cleaned up. + """ + with self._lock: + resolved = path.resolve() + self._files.discard(resolved) + logger.debug(f"Unregistered from cleanup: {resolved}") + + def _cleanup(self) -> None: + """Remove all registered files. Safe to call multiple times.""" + with self._lock: + if self._cleanup_done: + return + self._cleanup_done = True + + for path in self._files: + try: + if path.exists(): + path.unlink() + logger.debug(f"Cleaned up: {path}") + except Exception as e: + logger.warning(f"Failed to clean up {path}: {e}") + + self._files.clear() + + def cleanup_now(self) -> None: + """Manually trigger cleanup (e.g., before explicit exit). + + After calling this, registered files are cleared and won't be + cleaned up again on exit. + """ + self._cleanup() + + +def get_cleanup_manager() -> CleanupManager: + """Get the singleton CleanupManager instance. + + Returns: + The global CleanupManager instance. + """ + return CleanupManager() + + def cleanup_pyats_runtime(workspace_path: Path | None = None) -> None: """Clean up PyATS runtime directories before test execution. diff --git a/tests/e2e/fixtures/success/data.yaml b/tests/e2e/fixtures/success/data.yaml index 4db39c46..4495a92c 100644 --- a/tests/e2e/fixtures/success/data.yaml +++ b/tests/e2e/fixtures/success/data.yaml @@ -1,6 +1,7 @@ sdwan: management_ip_variable: vpn0_ip + test_credential: !env TEST_CREDENTIAL_SENTINEL sites: - name: SITE_100 site_id: "100" diff --git a/tests/integration/test_controller_detection_integration.py b/tests/integration/test_controller_detection_integration.py index 215d7bbc..a74adc19 100644 --- a/tests/integration/test_controller_detection_integration.py +++ b/tests/integration/test_controller_detection_integration.py @@ -53,7 +53,6 @@ def test_method(self): data_paths=[tmp_path / "data.yaml"], test_dir=test_dir, output_dir=output_dir, - merged_data_filename="merged_data.yaml", ) # Verify controller type was detected correctly diff --git a/tests/pyats_core/test_orchestrator_controller_detection.py b/tests/pyats_core/test_orchestrator_controller_detection.py index 57b600a8..0b0d5002 100644 --- a/tests/pyats_core/test_orchestrator_controller_detection.py +++ b/tests/pyats_core/test_orchestrator_controller_detection.py @@ -24,7 +24,6 @@ def test_orchestrator_detects_controller_on_init( data_paths=[pyats_test_dirs.output_dir.parent / "data.yaml"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) assert orchestrator.controller_type == "ACI" @@ -38,7 +37,6 @@ def test_orchestrator_exits_on_detection_failure( data_paths=[pyats_test_dirs.output_dir.parent / "data.yaml"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) # Verify it exits with EXIT_ERROR (255) for infrastructure errors @@ -56,7 +54,6 @@ def test_orchestrator_handles_multiple_controllers_error( data_paths=[pyats_test_dirs.output_dir.parent / "data.yaml"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) # Verify it exits with EXIT_ERROR (255) for infrastructure errors @@ -70,7 +67,6 @@ def test_validate_environment_uses_detected_controller( data_paths=[pyats_test_dirs.output_dir.parent / "data.yaml"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) assert orchestrator.controller_type == "SDWAN" @@ -94,7 +90,6 @@ def test_orchestrator_no_longer_uses_controller_type_env_var( data_paths=[pyats_test_dirs.output_dir.parent / "data.yaml"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) assert orchestrator.controller_type == "ACI" diff --git a/tests/pyats_core/test_orchestrator_controller_param.py b/tests/pyats_core/test_orchestrator_controller_param.py index d8a63cef..d9083d4a 100644 --- a/tests/pyats_core/test_orchestrator_controller_param.py +++ b/tests/pyats_core/test_orchestrator_controller_param.py @@ -24,7 +24,6 @@ def test_orchestrator_uses_provided_controller_type( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", controller_type="SDWAN", ) @@ -39,7 +38,6 @@ def test_orchestrator_falls_back_to_detection_when_none( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", controller_type=None, ) @@ -53,7 +51,6 @@ def test_orchestrator_defaults_to_detection( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", ) assert orchestrator.controller_type == "CC" @@ -66,7 +63,6 @@ def test_validate_environment_uses_provided_controller( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", controller_type="ACI", ) diff --git a/tests/pyats_core/test_orchestrator_dry_run.py b/tests/pyats_core/test_orchestrator_dry_run.py index 03f6f02d..6fb4091e 100644 --- a/tests/pyats_core/test_orchestrator_dry_run.py +++ b/tests/pyats_core/test_orchestrator_dry_run.py @@ -32,7 +32,6 @@ def test_dry_run_prints_summary_and_skips_execution( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", dry_run=True, ) @@ -80,7 +79,6 @@ def test_dry_run_returns_not_run_results_for_api_and_d2d( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", dry_run=True, ) @@ -114,7 +112,6 @@ def test_dry_run_does_not_execute_tests( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", dry_run=True, ) @@ -145,7 +142,6 @@ def test_dry_run_empty_test_lists( data_paths=[pyats_test_dirs.output_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", dry_run=True, ) diff --git a/tests/pyats_core/test_orchestrator_env_var.py b/tests/pyats_core/test_orchestrator_env_var.py index ceb3f80e..45685055 100644 --- a/tests/pyats_core/test_orchestrator_env_var.py +++ b/tests/pyats_core/test_orchestrator_env_var.py @@ -26,7 +26,6 @@ def test_orchestrator_respects_nac_test_pyats_processes_env_var( data_paths=[pyats_test_dirs.test_dir.parent / "data"], test_dir=pyats_test_dirs.test_dir, output_dir=pyats_test_dirs.output_dir, - merged_data_filename="merged.yaml", controller_type="ACI", ) diff --git a/tests/unit/robot/test_orchestrator.py b/tests/unit/robot/test_orchestrator.py index 5df3a412..751fa6c5 100644 --- a/tests/unit/robot/test_orchestrator.py +++ b/tests/unit/robot/test_orchestrator.py @@ -2,8 +2,6 @@ # Copyright (c) 2025 Daniel Schmidt # mypy: disable-error-code="no-untyped-def,method-assign" -# SPDX-License-Identifier: MPL-2.0 -# Copyright (c) 2025 Daniel Schmidt """Unit tests for Robot Framework orchestrator.""" @@ -13,6 +11,7 @@ import pytest from nac_test.core.constants import ( + DEFAULT_MERGED_DATA_FILENAME, LOG_HTML, ORDERING_FILENAME, OUTPUT_XML, @@ -29,20 +28,15 @@ @pytest.fixture def temp_output_dir(tmp_path: Path) -> Path: - """Create a temporary output directory for tests.""" + """Create a temporary output directory with merged data file for tests.""" output_dir = tmp_path / "output" output_dir.mkdir() + # Create merged data file that RobotWriter expects to read + merged_data_file = output_dir / DEFAULT_MERGED_DATA_FILENAME + merged_data_file.write_text("test: data") return output_dir -@pytest.fixture -def mock_data_paths(tmp_path: Path) -> list[Path]: - """Create mock data paths.""" - data_file = tmp_path / "data.yaml" - data_file.write_text("test: data") - return [data_file] - - @pytest.fixture def mock_templates_dir(tmp_path: Path) -> Path: """Create mock templates directory.""" @@ -52,15 +46,11 @@ def mock_templates_dir(tmp_path: Path) -> Path: @pytest.fixture -def orchestrator( - mock_data_paths, mock_templates_dir, temp_output_dir -) -> RobotOrchestrator: +def orchestrator(mock_templates_dir, temp_output_dir) -> RobotOrchestrator: """Create a RobotOrchestrator instance for testing.""" return RobotOrchestrator( - data_paths=mock_data_paths, templates_dir=mock_templates_dir, output_dir=temp_output_dir, - merged_data_filename="merged_data.yaml", loglevel=DEFAULT_LOGLEVEL, ) @@ -69,28 +59,28 @@ class TestRobotOrchestrator: """Test suite for RobotOrchestrator.""" def test_initialization( - self, orchestrator, temp_output_dir, mock_data_paths, mock_templates_dir + self, orchestrator, temp_output_dir, mock_templates_dir ) -> None: """Test orchestrator initialization.""" assert orchestrator.base_output_dir == temp_output_dir assert orchestrator.output_dir == temp_output_dir / ROBOT_RESULTS_DIRNAME assert orchestrator.ordering_file == orchestrator.output_dir / ORDERING_FILENAME - assert orchestrator.data_paths == mock_data_paths assert orchestrator.templates_dir == mock_templates_dir - assert orchestrator.merged_data_filename == "merged_data.yaml" + assert ( + orchestrator.merged_data_path + == temp_output_dir / DEFAULT_MERGED_DATA_FILENAME + ) assert orchestrator.render_only is False assert orchestrator.dry_run is False assert orchestrator.loglevel == DEFAULT_LOGLEVEL def test_initialization_with_optional_params( - self, mock_data_paths, mock_templates_dir, temp_output_dir + self, mock_templates_dir, temp_output_dir ) -> None: """Test orchestrator initialization with optional parameters.""" orchestrator = RobotOrchestrator( - data_paths=mock_data_paths, templates_dir=mock_templates_dir, output_dir=temp_output_dir, - merged_data_filename="merged.yaml", include_tags=["smoke", "regression"], exclude_tags=["wip"], render_only=True, @@ -194,20 +184,13 @@ def test_run_tests_render_only_mode( """Test run_tests in render-only mode.""" orchestrator.render_only = True - # Mock RobotWriter instance methods directly on the orchestrator's writer orchestrator.robot_writer.write = MagicMock() - orchestrator.robot_writer.write_merged_data_model = MagicMock() stats = orchestrator.run_tests() - # Verify template rendering was called orchestrator.robot_writer.write.assert_called_once() - orchestrator.robot_writer.write_merged_data_model.assert_called_once() - - # Verify pabot was NOT called mock_pabot.assert_not_called() - # Verify render-only mode returns not_run() result with SKIPPED state assert stats.was_not_run is True assert stats.reason == "render-only mode" @@ -217,14 +200,10 @@ def test_run_tests_full_execution( self, mock_generator, mock_pabot, orchestrator, temp_output_dir ) -> None: """Test run_tests executes full test lifecycle.""" - # Mock RobotWriter instance methods directly orchestrator.robot_writer.write = MagicMock() - orchestrator.robot_writer.write_merged_data_model = MagicMock() - # Mock pabot success mock_pabot.return_value = 0 - # Mock report generator - now returns tuple (path, stats) mock_generator_instance = MagicMock() mock_stats = TestResults(passed=1, failed=0, skipped=0) mock_generator_instance.generate_summary_report.return_value = ( @@ -233,7 +212,6 @@ def test_run_tests_full_execution( ) mock_generator.return_value = mock_generator_instance - # Create mock Robot output files in robot_results/ (pabot 5.2+ writes directly there) robot_results_dir = temp_output_dir / ROBOT_RESULTS_DIRNAME robot_results_dir.mkdir() for filename in [LOG_HTML, OUTPUT_XML, REPORT_HTML, XUNIT_XML]: @@ -241,13 +219,10 @@ def test_run_tests_full_execution( stats = orchestrator.run_tests() - # Verify all phases executed orchestrator.robot_writer.write.assert_called_once() - orchestrator.robot_writer.write_merged_data_model.assert_called_once() mock_pabot.assert_called_once() mock_generator_instance.generate_summary_report.assert_called_once() - # Verify statistics returned assert stats.total == 1 assert stats.passed == 1 assert stats.failed == 0 @@ -258,14 +233,10 @@ def test_run_tests_handles_pabot_error_252( self, mock_pabot: MagicMock, orchestrator: RobotOrchestrator ) -> None: """Test run_tests returns TestResults.from_error on pabot exit code 252 (invalid arguments).""" - # Mock RobotWriter instance methods orchestrator.robot_writer.write = MagicMock() - orchestrator.robot_writer.write_merged_data_model = MagicMock() - # Mock pabot failure with exit code 252 mock_pabot.return_value = 252 - # Should return TestResults with error state result = orchestrator.run_tests() assert isinstance(result, TestResults) assert result.has_error @@ -349,7 +320,6 @@ def test_verbose_flag_passed_to_pabot( self, mock_generator, mock_pabot, - mock_data_paths, mock_templates_dir, temp_output_dir, verbose, @@ -359,15 +329,12 @@ def test_verbose_flag_passed_to_pabot( ) -> None: """Test that verbose and loglevel are correctly passed to run_pabot.""" orchestrator = RobotOrchestrator( - data_paths=mock_data_paths, templates_dir=mock_templates_dir, output_dir=temp_output_dir, - merged_data_filename="merged.yaml", verbose=verbose, loglevel=loglevel, ) orchestrator.robot_writer.write = MagicMock() - orchestrator.robot_writer.write_merged_data_model = MagicMock() mock_pabot.return_value = 0 mock_generator_instance = MagicMock() diff --git a/tests/unit/test_combined_orchestrator_controller.py b/tests/unit/test_combined_orchestrator_controller.py index cd040ada..9b4c14b7 100644 --- a/tests/unit/test_combined_orchestrator_controller.py +++ b/tests/unit/test_combined_orchestrator_controller.py @@ -51,7 +51,6 @@ def test_controller_type_is_none_after_init( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", ) # Controller detection is now deferred to run_tests() @@ -76,7 +75,6 @@ def test_controller_detected_during_run_tests( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=True, ) @@ -125,7 +123,6 @@ def test_detection_failure_continues_with_preflight_failure( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=True, ) @@ -190,7 +187,6 @@ def test_combined_orchestrator_passes_controller_to_pyats( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=True, # Run PyATS only mode ) @@ -235,7 +231,6 @@ def test_combined_orchestrator_passes_controller_to_pyats( data_paths=[data_dir], test_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", minimal_reports=False, custom_testbed_path=None, controller_type="SDWAN", @@ -291,7 +286,6 @@ def test_render_only_mode_does_not_instantiate_pyats_orchestrator( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", render_only=True, # Critical: render-only mode ) @@ -362,7 +356,6 @@ def test_combined_orchestrator_production_mode_passes_controller( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", ) # Controller type should be None after init (deferred to run_tests) @@ -404,7 +397,6 @@ def test_combined_orchestrator_production_mode_passes_controller( data_paths=[data_dir], test_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", minimal_reports=False, custom_testbed_path=None, controller_type="CC", diff --git a/tests/unit/test_combined_orchestrator_flow.py b/tests/unit/test_combined_orchestrator_flow.py index 947f8ec8..8908cc02 100644 --- a/tests/unit/test_combined_orchestrator_flow.py +++ b/tests/unit/test_combined_orchestrator_flow.py @@ -75,7 +75,6 @@ def orchestrator(self, tmp_path: Path) -> CombinedOrchestrator: data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", ) @pytest.fixture @@ -294,7 +293,6 @@ def test_dev_pyats_only_skips_robot( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=True, ) @@ -354,7 +352,6 @@ def test_dev_robot_only_skips_pyats( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_robot_only=True, ) @@ -416,7 +413,6 @@ def test_dev_pyats_only_generates_dashboard( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=True, ) @@ -477,7 +473,6 @@ def test_render_only_skips_dashboard_generation( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", render_only=True, ) @@ -522,7 +517,6 @@ def test_render_only_converts_robot_exception_to_results( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", render_only=True, ) @@ -565,7 +559,6 @@ def test_non_render_only_catches_robot_exception( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", render_only=False, # Not render-only ) @@ -753,7 +746,6 @@ def test_print_execution_summary_not_called_in_render_only( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", render_only=True, ) diff --git a/tests/unit/test_combined_orchestrator_python311.py b/tests/unit/test_combined_orchestrator_python311.py index c22e52b5..9536b73b 100644 --- a/tests/unit/test_combined_orchestrator_python311.py +++ b/tests/unit/test_combined_orchestrator_python311.py @@ -39,7 +39,6 @@ def _make_orchestrator( data_paths=[data_dir], templates_dir=templates_dir, output_dir=output_dir, - merged_data_filename="merged.yaml", dev_pyats_only=dev_pyats_only, ) diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py new file mode 100644 index 00000000..84cc1e67 --- /dev/null +++ b/tests/unit/utils/test_cleanup.py @@ -0,0 +1,230 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2025 Daniel Schmidt + +"""Unit tests for CleanupManager.""" + +from __future__ import annotations + +import signal +import threading +from collections.abc import Generator +from pathlib import Path +from unittest.mock import patch + +import pytest + +from nac_test.core.constants import IS_WINDOWS +from nac_test.utils.cleanup import CleanupManager, get_cleanup_manager + + +@pytest.fixture +def fresh_cleanup_manager() -> Generator[CleanupManager, None, None]: + """Create a fresh CleanupManager instance isolated from the global singleton.""" + with ( + patch.object(CleanupManager, "_instance", None), + patch.object(CleanupManager, "_initialized", False), + patch("atexit.register"), + ): + cm = CleanupManager() + cm._files.clear() + cm._cleanup_done = False + yield cm + + +class TestCleanupManagerSingleton: + """Tests for CleanupManager singleton behavior.""" + + def test_multiple_calls_return_same_instance( + self, fresh_cleanup_manager: CleanupManager + ) -> None: + cm2 = CleanupManager() + assert fresh_cleanup_manager is cm2 + + def test_get_cleanup_manager_returns_singleton( + self, fresh_cleanup_manager: CleanupManager + ) -> None: + assert get_cleanup_manager() is fresh_cleanup_manager + + +class TestCleanupManagerRegistration: + """Tests for file registration and unregistration.""" + + def test_register_adds_path( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + + assert test_file.resolve() in fresh_cleanup_manager._files + + def test_register_multiple_files( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + file1 = tmp_path / "file1.txt" + file2 = tmp_path / "file2.txt" + file1.touch() + file2.touch() + + fresh_cleanup_manager.register(file1) + fresh_cleanup_manager.register(file2) + + assert len(fresh_cleanup_manager._files) == 2 + + def test_unregister_removes_path( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + fresh_cleanup_manager.unregister(test_file) + + assert test_file.resolve() not in fresh_cleanup_manager._files + + def test_unregister_nonexistent_path_is_safe( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + nonexistent = tmp_path / "nonexistent.txt" + fresh_cleanup_manager.unregister(nonexistent) # Should not raise + + +class TestCleanupManagerCleanup: + """Tests for cleanup behavior.""" + + def test_cleanup_removes_registered_files( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "sensitive_data.yaml" + test_file.write_text("secret: password123") + + fresh_cleanup_manager.register(test_file) + fresh_cleanup_manager.cleanup_now() + + assert not test_file.exists() + + def test_cleanup_is_idempotent( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.cleanup_now() # Second call should be safe + + assert fresh_cleanup_manager._cleanup_done + + def test_cleanup_handles_already_deleted_file( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + test_file.unlink() # Delete before cleanup + + fresh_cleanup_manager.cleanup_now() # Should not raise + + def test_cleanup_continues_after_single_file_error( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + file1 = tmp_path / "file1.txt" + file2 = tmp_path / "file2.txt" + file1.touch() + file2.touch() + + fresh_cleanup_manager.register(file1) + fresh_cleanup_manager.register(file2) + + # Make file1 a directory so unlink fails + file1.unlink() + file1.mkdir() + + fresh_cleanup_manager.cleanup_now() + + # file2 should still be cleaned up despite file1 error + assert not file2.exists() + # Cleanup completed + assert fresh_cleanup_manager._cleanup_done + + +class TestCleanupManagerThreadSafety: + """Tests for thread-safe behavior.""" + + def test_concurrent_registration( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + num_threads = 10 + files_per_thread = 10 + + def register_files(thread_id: int) -> None: + for i in range(files_per_thread): + f = tmp_path / f"thread{thread_id}_file{i}.txt" + f.touch() + fresh_cleanup_manager.register(f) + + threads = [ + threading.Thread(target=register_files, args=(i,)) + for i in range(num_threads) + ] + + for t in threads: + t.start() + for t in threads: + t.join() + + assert len(fresh_cleanup_manager._files) == num_threads * files_per_thread + + +@pytest.mark.skipif(IS_WINDOWS, reason="Signal tests not supported on Windows") +class TestCleanupManagerSignalHandlers: + """Tests for signal handler behavior (Unix only).""" + + def test_sigint_cleans_up_and_raises_keyboard_interrupt( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + fresh_cleanup_manager.register(test_file) + + with patch("signal.signal"), pytest.raises(KeyboardInterrupt): + fresh_cleanup_manager._signal_handler(signal.SIGINT, None) + + assert not test_file.exists() + + def test_sigterm_cleans_up_and_reraises_signal( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + test_file = tmp_path / "test.txt" + test_file.touch() + fresh_cleanup_manager.register(test_file) + + with patch("signal.signal"), patch("signal.raise_signal") as mock_raise: + fresh_cleanup_manager._signal_handler(signal.SIGTERM, None) + mock_raise.assert_called_once_with(signal.SIGTERM) + + assert not test_file.exists() + + +class TestCleanupManagerIntegration: + """Integration tests using the real singleton.""" + + def test_multiple_registrations_share_state(self, tmp_path: Path) -> None: + cm = get_cleanup_manager() + initial_count = len(cm._files) + + file1 = tmp_path / "module_a.txt" + file2 = tmp_path / "module_b.txt" + file1.touch() + file2.touch() + + cm.register(file1) + cm.register(file2) + + assert len(cm._files) == initial_count + 2 + + # Clean up test files from singleton + cm.unregister(file1) + cm.unregister(file2) From 69191babf5a7ef4d9ed8c5117c14fa07bd3b2a58 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 25 Mar 2026 21:04:33 +0100 Subject: [PATCH 03/28] refactor: pass merged_data dict through Robot path, simplify DataMerger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eliminates the redundant YAML re-read in RobotWriter (fixes #704). Previously, main.py merged and wrote the data model to disk, then RobotWriter re-read and re-parsed it — costing ~51 s and ~400 MB peak RAM on large data models. The already-loaded dict is now passed through CombinedOrchestrator → RobotOrchestrator → RobotWriter directly. DataMerger is simplified in line with the removal of --merged-data-filename in #677: the filename parameter is removed from write_merged_data_model() (hardcoded to DEFAULT_MERGED_DATA_FILENAME), and load_yaml_file() is removed entirely as it had no remaining callers. RobotWriter._convert_data_model_for_templates() is deleted: the JSON round-trip was intended to convert CommentedMap to plain dicts for Jinja2, but CommentedMap is a dict subclass and is fully compatible natively. The redundant self.template_data alias is collapsed into self.data. Tests: - Add tests/unit/test_data_merger.py covering merge content correctness and write/roundtrip behaviour, replacing coverage lost when the commented-out integration tests were cleaned up - Add tests/unit/robot/test_robot_writer.py covering constructor semantics and render_template context selection - Move data_merge fixtures from tests/integration/fixtures/ to tests/unit/fixtures/ where they belong --- nac_test/cli/main.py | 5 +- nac_test/combined_orchestrator.py | 7 + nac_test/data_merger.py | 23 +-- nac_test/robot/orchestrator.py | 10 +- nac_test/robot/robot_writer.py | 53 +------ tests/integration/test_cli_rendering.py | 89 ----------- .../fixtures/data_merge/defaults.yaml | 0 .../fixtures/data_merge/file1.yaml | 0 .../fixtures/data_merge/file2.yaml | 0 .../fixtures/data_merge/result.yaml | 0 tests/unit/robot/test_orchestrator.py | 24 +-- tests/unit/robot/test_robot_writer.py | 148 ++++++++++++++++++ tests/unit/test_data_merger.py | 123 +++++++++++++++ 13 files changed, 313 insertions(+), 169 deletions(-) rename tests/{integration => unit}/fixtures/data_merge/defaults.yaml (100%) rename tests/{integration => unit}/fixtures/data_merge/file1.yaml (100%) rename tests/{integration => unit}/fixtures/data_merge/file2.yaml (100%) rename tests/{integration => unit}/fixtures/data_merge/result.yaml (100%) create mode 100644 tests/unit/robot/test_robot_writer.py create mode 100644 tests/unit/test_data_merger.py diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 5ad9bd07..221afd92 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -387,9 +387,7 @@ def main( typer.echo("\n\n📄 Merging data model files...") merged_data = DataMerger.merge_data_files(data) - DataMerger.write_merged_data_model( - merged_data, output, DEFAULT_MERGED_DATA_FILENAME - ) + DataMerger.write_merged_data_model(merged_data, output) merged_data_path = output / DEFAULT_MERGED_DATA_FILENAME if os.name != "nt": @@ -426,6 +424,7 @@ def exit_with_cleanup(code: int) -> NoReturn: dev_pyats_only=pyats, dev_robot_only=robot, verbose=verbose, + merged_data=merged_data, ) # Track total runtime for benchmarking diff --git a/nac_test/combined_orchestrator.py b/nac_test/combined_orchestrator.py index 76fc130d..cfdfb74f 100644 --- a/nac_test/combined_orchestrator.py +++ b/nac_test/combined_orchestrator.py @@ -6,6 +6,7 @@ import logging import os from pathlib import Path +from typing import Any import typer @@ -89,6 +90,7 @@ def __init__( processes: int | None = None, extra_args: ValidatedRobotArgs | None = None, verbose: bool = False, + merged_data: dict[str, Any] | None = None, ): """Initialize the combined orchestrator. @@ -111,6 +113,7 @@ def __init__( dev_pyats_only: Development mode - run only PyATS tests (skip Robot) dev_robot_only: Development mode - run only Robot Framework tests (skip PyATS) verbose: Enable verbose mode - keeps archive files, enables verbose output + merged_data: Already-loaded merged data model dict (avoids re-reading from disk) """ self.data_paths = data_paths self.templates_dir = Path(templates_dir) @@ -136,6 +139,9 @@ def __init__( self.dev_pyats_only = dev_pyats_only self.dev_robot_only = dev_robot_only self.verbose = verbose + self.merged_data: dict[str, Any] = ( + merged_data if merged_data is not None else {} + ) # Controller type — detected lazily in run_tests() when PyATS tests are present self.controller_type: ControllerTypeKey | None = None @@ -241,6 +247,7 @@ def run_tests(self) -> CombinedResults: extra_args=self.extra_args, loglevel=self.loglevel, verbose=self.verbose, + merged_data=self.merged_data, ) try: robot_results = robot_orchestrator.run_tests() diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index fec999ef..40a89cd4 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -9,6 +9,8 @@ from nac_yaml import yaml +from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME + logger = logging.getLogger(__name__) @@ -36,29 +38,16 @@ def merge_data_files(data_paths: list[Path]) -> dict[str, Any]: def write_merged_data_model( data: dict[str, Any], output_directory: Path, - filename: str, ) -> None: """Write merged data model to YAML file. + The output filename is always DEFAULT_MERGED_DATA_FILENAME — the single + fixed location used by all consumers (Robot, PyATS subprocesses, cleanup). + Args: data: The merged data dictionary to write output_directory: Directory where the YAML file will be saved - filename: Name of the output YAML file """ - full_output_path = output_directory / filename + full_output_path = output_directory / DEFAULT_MERGED_DATA_FILENAME logger.info("Writing merged data model to %s", full_output_path) yaml.write_yaml_file(data, full_output_path) - - @staticmethod - def load_yaml_file(file_path: Path) -> dict[str, Any]: - """Load a single YAML file from the provided path. - - Args: - file_path: Path to the YAML file to load - - Returns: - Loaded dictionary from the YAML file - """ - logger.info("Loading yaml file from %s", file_path) - data = yaml.load_yaml_files([file_path]) - return data if data is not None else {} diff --git a/nac_test/robot/orchestrator.py b/nac_test/robot/orchestrator.py index f9a26121..1e951df0 100644 --- a/nac_test/robot/orchestrator.py +++ b/nac_test/robot/orchestrator.py @@ -10,11 +10,11 @@ import logging from pathlib import Path +from typing import Any import typer from nac_test.core.constants import ( - DEFAULT_MERGED_DATA_FILENAME, DISABLE_TESTLEVELSPLIT, EXIT_DATA_ERROR, EXIT_ERROR, @@ -59,6 +59,7 @@ def __init__( extra_args: ValidatedRobotArgs | None = None, loglevel: LogLevel = DEFAULT_LOGLEVEL, verbose: bool = False, + merged_data: dict[str, Any] | None = None, ): """Initialize the Robot Framework orchestrator. @@ -75,11 +76,11 @@ def __init__( extra_args: Additional Robot Framework arguments to pass to pabot loglevel: Log level verbose: Enable verbose mode - enables verbose output for pabot + merged_data: Already-loaded merged data model dict (avoids re-reading from disk) """ self.templates_dir = Path(templates_dir) self.base_output_dir = Path(output_dir) self.output_dir = self.base_output_dir / ROBOT_RESULTS_DIRNAME - self.merged_data_path = self.base_output_dir / DEFAULT_MERGED_DATA_FILENAME # Robot-specific parameters self.filters_path = filters_path @@ -92,6 +93,9 @@ def __init__( self.extra_args = extra_args self.loglevel = loglevel self.verbose = verbose + self.merged_data: dict[str, Any] = ( + merged_data if merged_data is not None else {} + ) # Determine if ordering file should be used for test-level parallelization if not DISABLE_TESTLEVELSPLIT: @@ -101,7 +105,7 @@ def __init__( # Initialize Robot Framework components (reuse existing implementations) self.robot_writer = RobotWriter( - merged_data_path=self.merged_data_path, + merged_data=self.merged_data, filters_path=self.filters_path, tests_path=self.tests_path, include_tags=self.include_tags, diff --git a/nac_test/robot/robot_writer.py b/nac_test/robot/robot_writer.py index 80bbf7a2..45c4c72d 100644 --- a/nac_test/robot/robot_writer.py +++ b/nac_test/robot/robot_writer.py @@ -3,7 +3,6 @@ import copy import importlib.util -import json import logging import os import pathlib @@ -11,7 +10,7 @@ import shutil import sys from pathlib import Path -from typing import Any, cast +from typing import Any from jinja2 import ( ChainableUndefined, @@ -22,8 +21,6 @@ from robot.api import SuiteVisitor, TestSuite from robot.utils import is_truthy -from nac_test.data_merger import DataMerger - logger = logging.getLogger(__name__) @@ -61,16 +58,13 @@ def start_test(self, test: Any) -> None: class RobotWriter: def __init__( self, - merged_data_path: Path, + merged_data: dict[str, Any], filters_path: Path | None, tests_path: Path | None, include_tags: list[str] | None = None, exclude_tags: list[str] | None = None, ) -> None: - self.data = DataMerger.load_yaml_file(merged_data_path) - # Convert OrderedDict to dict once during initialization instead of per-template - # This eliminates expensive JSON round-trip serialization for each template render - self.template_data = self._convert_data_model_for_templates(self.data) + self.data = merged_data self.filters: dict[str, Any] = {} self.include_tags = include_tags or [] self.exclude_tags = exclude_tags or [] @@ -105,36 +99,6 @@ def __init__( self.tests[mod.Test.name] = mod.Test self.ordering_entries: list[str] = [] - def _convert_data_model_for_templates(self, data: dict[str, Any]) -> dict[str, Any]: - """Convert nested OrderedDict to dict to avoid duplicate dict keys. - - This method performs the same conversion that was previously done per-template, - but centralizes it during initialization for performance efficiency. - - Args: - data: Raw data model from DataMerger (may contain OrderedDict structures) - - Returns: - Converted data model safe for Jinja template rendering - - Note: - JSON round-trip approach is used as it's safe for all serializable data - and handles nested OrderedDict structures reliably. - """ - logger.debug( - "Converting data model for template rendering (one-time conversion)" - ) - try: - # JSON round-trip to convert nested OrderedDict to dict - # This preserves all data while fixing duplicate key issues (e.g., 'tag' fields) - converted_data = cast(dict[str, Any], json.loads(json.dumps(data))) - logger.debug("Data model conversion completed successfully") - return converted_data - except Exception as e: - logger.warning(f"Data model conversion failed: {e}. Using original data.") - # Fallback to original data if conversion fails - return data - def render_template( self, template_path: Path, @@ -152,13 +116,8 @@ def render_template( pathlib.Path(os.path.dirname(output_path)).mkdir(parents=True, exist_ok=True) template = env.get_template(template_path.as_posix()) - # Use custom_data if provided (for chunked templates), otherwise use pre-converted template_data - if custom_data is not None: - # Convert custom_data for template rendering (same as initialization conversion) - template_data = self._convert_data_model_for_templates(custom_data) - else: - # Use pre-converted data model (converted once during initialization) - template_data = self.template_data + # Use custom_data if provided (for chunked templates), otherwise use template_data + template_data = custom_data if custom_data is not None else self.data result = template.render(template_data, **kwargs) # remove extra empty lines @@ -384,7 +343,7 @@ def write( next_template = True path = params[2].split(".") attr = params[3] - elem: Any = self.template_data + elem: Any = self.data for p in path: try: elem = elem.get(p, {}) diff --git a/tests/integration/test_cli_rendering.py b/tests/integration/test_cli_rendering.py index bb59366c..c0761115 100644 --- a/tests/integration/test_cli_rendering.py +++ b/tests/integration/test_cli_rendering.py @@ -8,7 +8,6 @@ chunked rendering, and merged data model output. """ -import filecmp from pathlib import Path import pytest @@ -263,94 +262,6 @@ def test_chunked_list_rendering_produces_expected_content(tmp_path: Path) -> Non ) -def test_merged_data_model_creates_default_filename(tmp_path: Path) -> None: - """Test that merged data model creates file with default filename. - - Verifies that the CLI creates a merged data model YAML file with - the default filename when no custom filename is specified. - - Args: - tmp_path: Pytest fixture providing a temporary directory. - """ - runner = CliRunner() - data_path = "tests/integration/fixtures/data_merge/" - templates_path = "tests/integration/fixtures/templates/" - expected_filename = "merged_data_model_test_variables.yaml" - output_model_path = tmp_path / expected_filename - data_dir = Path(data_path) - expected_model_path = data_dir / "result.yaml" - - base_args = [ - "-d", - str(data_dir / "file1.yaml"), - "-d", - str(data_dir / "file2.yaml"), - "-t", - templates_path, - "-o", - str(tmp_path), - "--render-only", - ] - - result = runner.invoke(nac_test.cli.main.app, base_args) - assert result.exit_code == 0, ( - f"Merged data model creation should succeed, got exit code " - f"{result.exit_code}: {result.output}" - ) - assert output_model_path.exists(), ( - f"Default merged data model file should exist at {output_model_path}" - ) - assert filecmp.cmp(output_model_path, expected_model_path, shallow=False), ( - f"Merged data model content should match expected content from " - f"{expected_model_path}" - ) - - -def test_merged_data_model_creates_custom_filename(tmp_path: Path) -> None: - """Test that merged data model creates file with custom filename. - - Verifies that the CLI creates a merged data model YAML file with - a custom filename when --merged-data-filename is specified. - - Args: - tmp_path: Pytest fixture providing a temporary directory. - """ - runner = CliRunner() - data_path = "tests/integration/fixtures/data_merge/" - templates_path = "tests/integration/fixtures/templates/" - expected_filename = "custom.yaml" - output_model_path = tmp_path / expected_filename - data_dir = Path(data_path) - expected_model_path = data_dir / "result.yaml" - - base_args = [ - "-d", - str(data_dir / "file1.yaml"), - "-d", - str(data_dir / "file2.yaml"), - "-t", - templates_path, - "-o", - str(tmp_path), - "--render-only", - "--merged-data-filename", - "custom.yaml", - ] - - result = runner.invoke(nac_test.cli.main.app, base_args) - assert result.exit_code == 0, ( - f"Merged data model with custom filename should succeed, got exit code " - f"{result.exit_code}: {result.output}" - ) - assert output_model_path.exists(), ( - f"Custom merged data model file should exist at {output_model_path}" - ) - assert filecmp.cmp(output_model_path, expected_model_path, shallow=False), ( - f"Custom merged data model content should match expected content from " - f"{expected_model_path}" - ) - - def test_render_only_without_controller_credentials(tmp_path: Path) -> None: """Render-only mode works without controller environment variables. All other tests in this module implicitly also test this, but this diff --git a/tests/integration/fixtures/data_merge/defaults.yaml b/tests/unit/fixtures/data_merge/defaults.yaml similarity index 100% rename from tests/integration/fixtures/data_merge/defaults.yaml rename to tests/unit/fixtures/data_merge/defaults.yaml diff --git a/tests/integration/fixtures/data_merge/file1.yaml b/tests/unit/fixtures/data_merge/file1.yaml similarity index 100% rename from tests/integration/fixtures/data_merge/file1.yaml rename to tests/unit/fixtures/data_merge/file1.yaml diff --git a/tests/integration/fixtures/data_merge/file2.yaml b/tests/unit/fixtures/data_merge/file2.yaml similarity index 100% rename from tests/integration/fixtures/data_merge/file2.yaml rename to tests/unit/fixtures/data_merge/file2.yaml diff --git a/tests/integration/fixtures/data_merge/result.yaml b/tests/unit/fixtures/data_merge/result.yaml similarity index 100% rename from tests/integration/fixtures/data_merge/result.yaml rename to tests/unit/fixtures/data_merge/result.yaml diff --git a/tests/unit/robot/test_orchestrator.py b/tests/unit/robot/test_orchestrator.py index 97e3f92b..68ee1be0 100644 --- a/tests/unit/robot/test_orchestrator.py +++ b/tests/unit/robot/test_orchestrator.py @@ -6,12 +6,12 @@ """Unit tests for Robot Framework orchestrator.""" from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest from nac_test.core.constants import ( - DEFAULT_MERGED_DATA_FILENAME, EXIT_DATA_ERROR, LOG_HTML, ORDERING_FILENAME, @@ -29,15 +29,18 @@ @pytest.fixture def temp_output_dir(tmp_path: Path) -> Path: - """Create a temporary output directory with merged data file for tests.""" + """Create a temporary output directory for tests.""" output_dir = tmp_path / "output" output_dir.mkdir() - # Create merged data file that RobotWriter expects to read - merged_data_file = output_dir / DEFAULT_MERGED_DATA_FILENAME - merged_data_file.write_text("test: data") return output_dir +@pytest.fixture +def merged_data() -> dict[str, Any]: + """Minimal merged data dict passed to RobotOrchestrator/RobotWriter.""" + return {"test": "data"} + + @pytest.fixture def mock_templates_dir(tmp_path: Path) -> Path: """Create mock templates directory.""" @@ -47,12 +50,13 @@ def mock_templates_dir(tmp_path: Path) -> Path: @pytest.fixture -def orchestrator(mock_templates_dir, temp_output_dir) -> RobotOrchestrator: +def orchestrator(mock_templates_dir, temp_output_dir, merged_data) -> RobotOrchestrator: """Create a RobotOrchestrator instance for testing.""" return RobotOrchestrator( templates_dir=mock_templates_dir, output_dir=temp_output_dir, loglevel=DEFAULT_LOGLEVEL, + merged_data=merged_data, ) @@ -67,10 +71,7 @@ def test_initialization( assert orchestrator.output_dir == temp_output_dir / ROBOT_RESULTS_DIRNAME assert orchestrator.ordering_file == orchestrator.output_dir / ORDERING_FILENAME assert orchestrator.templates_dir == mock_templates_dir - assert ( - orchestrator.merged_data_path - == temp_output_dir / DEFAULT_MERGED_DATA_FILENAME - ) + assert orchestrator.merged_data == {"test": "data"} assert orchestrator.render_only is False assert orchestrator.dry_run is False assert orchestrator.loglevel == DEFAULT_LOGLEVEL @@ -91,6 +92,7 @@ def test_initialization_with_optional_params( processes=4, extra_args=ValidatedRobotArgs(args=["--exitonfailure"], robot_opts={}), loglevel=LogLevel.DEBUG, + merged_data={"test": "data"}, ) assert orchestrator.include_tags == ["smoke", "regression"] @@ -340,6 +342,7 @@ def test_verbose_flag_passed_to_pabot( output_dir=temp_output_dir, verbose=verbose, loglevel=loglevel, + merged_data={"test": "data"}, ) orchestrator.robot_writer.write = MagicMock() mock_pabot.return_value = 0 @@ -393,6 +396,7 @@ def test_include_exclude_tags_passed_to_pabot( output_dir=temp_output_dir, include_tags=include_tags, exclude_tags=exclude_tags, + merged_data={"test": "data"}, ) orchestrator.robot_writer.write = MagicMock() mock_pabot.return_value = 0 diff --git a/tests/unit/robot/test_robot_writer.py b/tests/unit/robot/test_robot_writer.py new file mode 100644 index 00000000..45e7ae78 --- /dev/null +++ b/tests/unit/robot/test_robot_writer.py @@ -0,0 +1,148 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2025 Daniel Schmidt + +# mypy: disable-error-code="no-untyped-def" + +"""Unit tests for RobotWriter. + +Covers: +- Constructor: merged_data dict is stored directly (no file I/O, no conversion) +- render_template: uses self.data as template context by default +- render_template: custom_data overrides self.data when provided +- render_template: output file and parent directories are created +""" + +from pathlib import Path + +import pytest +from jinja2 import Environment, FileSystemLoader + +from nac_test.robot.robot_writer import RobotWriter + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def templates_dir(tmp_path: Path) -> Path: + """Temporary directory with a minimal Jinja2 robot template.""" + (tmp_path / "simple.robot").write_text( + "*** Test Cases ***\nTest {{ device }}\n Log ok\n" + ) + return tmp_path + + +@pytest.fixture +def jinja_env(templates_dir: Path) -> Environment: + """Jinja2 environment rooted at templates_dir.""" + return Environment(loader=FileSystemLoader(str(templates_dir))) # nosec B701 + + +@pytest.fixture +def writer() -> RobotWriter: + """RobotWriter with a simple data dict matching the simple.robot template.""" + return RobotWriter( + merged_data={"device": "Router1", "vlan": 100}, + filters_path=None, + tests_path=None, + ) + + +# --------------------------------------------------------------------------- +# Constructor tests +# --------------------------------------------------------------------------- + + +class TestRobotWriterInit: + """Tests for RobotWriter.__init__().""" + + def test_stores_merged_data_directly(self) -> None: + """Constructor stores the dict by reference — no copy or type conversion.""" + data = {"host": "sw1", "count": 5} + w = RobotWriter(merged_data=data, filters_path=None, tests_path=None) + assert w.data is data + + def test_accepts_empty_dict(self) -> None: + """Constructor accepts an empty dict without raising.""" + w = RobotWriter(merged_data={}, filters_path=None, tests_path=None) + assert w.data == {} + + def test_tags_default_to_empty_lists(self) -> None: + """include_tags and exclude_tags default to empty lists when omitted.""" + w = RobotWriter(merged_data={}, filters_path=None, tests_path=None) + assert w.include_tags == [] + assert w.exclude_tags == [] + + @pytest.mark.parametrize( + ("include_tags", "exclude_tags"), + [ + (["smoke"], []), + ([], ["slow"]), + (["smoke"], ["slow"]), + (["smoke", "regression"], ["slow", "wip"]), + ], + ids=["include_only", "exclude_only", "both", "multiple_each"], + ) + def test_tags_stored_correctly(self, include_tags, exclude_tags) -> None: + """Explicit include/exclude tags are stored without modification.""" + w = RobotWriter( + merged_data={}, + filters_path=None, + tests_path=None, + include_tags=include_tags, + exclude_tags=exclude_tags, + ) + assert w.include_tags == include_tags + assert w.exclude_tags == exclude_tags + + +# --------------------------------------------------------------------------- +# render_template tests +# --------------------------------------------------------------------------- + + +class TestRobotWriterRenderTemplate: + """Tests for RobotWriter.render_template().""" + + def test_renders_using_self_data( + self, writer: RobotWriter, jinja_env: Environment, tmp_path: Path + ) -> None: + """render_template uses self.data as context when no custom_data is given.""" + output = tmp_path / "out.robot" + writer.render_template( + template_path=Path("simple.robot"), + output_path=output, + env=jinja_env, + ) + content = output.read_text() + assert "Router1" in content + assert "{{" not in content + + def test_custom_data_overrides_self_data( + self, writer: RobotWriter, jinja_env: Environment, tmp_path: Path + ) -> None: + """custom_data replaces self.data entirely as the template context.""" + output = tmp_path / "out.robot" + writer.render_template( + template_path=Path("simple.robot"), + output_path=output, + env=jinja_env, + custom_data={"device": "Switch99"}, + ) + content = output.read_text() + assert "Switch99" in content + assert "Router1" not in content + + def test_creates_output_file_and_parent_directories( + self, writer: RobotWriter, jinja_env: Environment, tmp_path: Path + ) -> None: + """render_template creates the output file and any missing parent directories.""" + output = tmp_path / "a" / "b" / "out.robot" + writer.render_template( + template_path=Path("simple.robot"), + output_path=output, + env=jinja_env, + ) + assert output.exists() + assert output.parent.is_dir() diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py new file mode 100644 index 00000000..fd67a6b9 --- /dev/null +++ b/tests/unit/test_data_merger.py @@ -0,0 +1,123 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2025 Daniel Schmidt + +"""Unit tests for DataMerger. + +Covers: +- merge_data_files: content correctness, empty input, single file +- write_merged_data_model: output filename, YAML roundtrip +""" + +from pathlib import Path + +from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME +from nac_test.data_merger import DataMerger + +FIXTURES_DIR = Path("tests/unit/fixtures/data_merge") + + +class TestMergeDataFiles: + """Tests for DataMerger.merge_data_files().""" + + def test_merge_produces_correct_scalar_values(self) -> None: + """Scalar attributes from both files are present in the merged result.""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + assert result["root"]["attr1"] == "value1" + assert result["root"]["attr2"] == "value2" + + def test_merge_concatenates_primitive_lists(self) -> None: + """Primitive lists are concatenated (not de-duplicated) across files.""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + assert result["root"]["primitive_list"] == ["item1", "item1", "item1"] + + def test_merge_merges_dict_lists_by_name_key(self) -> None: + """Dict-list entries with the same name key are merged (not duplicated).""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + dict_list = result["root"]["dict_list"] + assert len(dict_list) == 1 + assert dict_list[0]["name"] == "abc" + assert dict_list[0]["extra"] == "def" + + def test_merge_combines_extra_fields_across_files(self) -> None: + """Extra fields from both files are merged into the same named entry.""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + entry = result["root"]["dict_list_extra"][0] + assert entry["name"] == "abc" + assert entry["extra1"] == "def" + assert entry["extra2"] == "ghi" + + def test_merge_preserves_nested_defaults(self) -> None: + """Nested scalar values present only in the first file are preserved.""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + assert result["defaults"]["apic"]["version"] == 6.0 + + def test_merge_returns_dict(self) -> None: + """Return type is always a dict.""" + result = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + assert isinstance(result, dict) + + def test_merge_single_file_returns_its_content(self) -> None: + """A single-file list returns the content of that file unchanged.""" + result = DataMerger.merge_data_files([FIXTURES_DIR / "file1.yaml"]) + assert result["root"]["attr1"] == "value1" + assert "attr2" not in result.get("root", {}) + + def test_merge_empty_list_returns_empty_dict(self) -> None: + """An empty path list returns an empty dict rather than raising.""" + result = DataMerger.merge_data_files([]) + assert result == {} + + +class TestWriteMergedDataModel: + """Tests for DataMerger.write_merged_data_model().""" + + def test_writes_to_default_filename(self, tmp_path: Path) -> None: + """Output file is always named DEFAULT_MERGED_DATA_FILENAME.""" + DataMerger.write_merged_data_model({"key": "value"}, tmp_path) + expected = tmp_path / DEFAULT_MERGED_DATA_FILENAME + assert expected.exists(), f"Expected {expected} to exist" + + def test_writes_no_extra_files(self, tmp_path: Path) -> None: + """Exactly one file is created in the output directory.""" + DataMerger.write_merged_data_model({"key": "value"}, tmp_path) + files = list(tmp_path.iterdir()) + assert len(files) == 1 + + def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: + """Data written to YAML can be read back with the same structure.""" + from nac_yaml import yaml + + original = {"host": "router1", "vlan": 100, "tags": ["a", "b"]} + DataMerger.write_merged_data_model(original, tmp_path) + output_path = tmp_path / DEFAULT_MERGED_DATA_FILENAME + reloaded = yaml.load_yaml_files([output_path]) + assert reloaded["host"] == "router1" + assert reloaded["vlan"] == 100 + assert list(reloaded["tags"]) == ["a", "b"] + + def test_roundtrip_preserves_merged_fixture_content(self, tmp_path: Path) -> None: + """Full merge result can be written and reloaded without data loss.""" + from nac_yaml import yaml + + merged = DataMerger.merge_data_files( + [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] + ) + DataMerger.write_merged_data_model(merged, tmp_path) + output_path = tmp_path / DEFAULT_MERGED_DATA_FILENAME + reloaded = yaml.load_yaml_files([output_path]) + assert reloaded["root"]["attr1"] == "value1" + assert reloaded["root"]["attr2"] == "value2" + assert reloaded["root"]["primitive_list"] == ["item1", "item1", "item1"] + assert reloaded["root"]["dict_list"][0]["extra"] == "def" From 82d2333c59a84415a7a800763207e33331f18f51 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 08:22:49 +0100 Subject: [PATCH 04/28] fix: correct signal handler restoration for SIG_DFL (falsy value 0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit signal.SIG_DFL has the integer value 0, which is falsy. The previous `original if original else signal.SIG_DFL` expression would silently treat a legitimate SIG_DFL original disposition as "not set" and still return SIG_DFL — technically a no-op here, but masking the real issue: any None (unrecorded handler) and SIG_DFL were indistinguishable. Switch to `original if original is not None else signal.SIG_DFL` so the intent is explicit and the two cases are correctly distinguished. Add parametrized tests covering SIG_DFL (falsy), SIG_IGN (truthy), and None (fallback) to pin the correct behaviour. --- nac_test/utils/cleanup.py | 2 +- tests/unit/utils/test_cleanup.py | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index 4433f9e3..69ff9112 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -96,7 +96,7 @@ def _signal_handler(self, signum: int, frame: FrameType | None) -> None: original = self._original_sigint # Restore original handler and re-raise - signal.signal(signum, original if original else signal.SIG_DFL) + signal.signal(signum, original if original is not None else signal.SIG_DFL) # For SIGINT, raise KeyboardInterrupt to allow normal exception handling if signum == signal.SIGINT: diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index 84cc1e67..c77bafea 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -9,6 +9,7 @@ import threading from collections.abc import Generator from pathlib import Path +from typing import Any from unittest.mock import patch import pytest @@ -207,6 +208,44 @@ def test_sigterm_cleans_up_and_reraises_signal( assert not test_file.exists() + @pytest.mark.parametrize( + ("signum", "original_handler", "expected_restored"), + [ + # SIG_DFL (value 0) is falsy — must use `is not None`, not truthiness check + (signal.SIGTERM, signal.SIG_DFL, signal.SIG_DFL), + # SIG_IGN (value 1) is truthy but should still be restored correctly + (signal.SIGTERM, signal.SIG_IGN, signal.SIG_IGN), + # None means no prior handler was recorded — fall back to SIG_DFL + (signal.SIGTERM, None, signal.SIG_DFL), + ], + ids=["SIG_DFL_restored", "SIG_IGN_restored", "None_falls_back_to_SIG_DFL"], + ) + def test_signal_handler_restores_original_handler( + self, + fresh_cleanup_manager: CleanupManager, + signum: int, + original_handler: Any, + expected_restored: Any, + ) -> None: + """Handler restores the original signal disposition, including falsy SIG_DFL.""" + if signum == signal.SIGTERM: + fresh_cleanup_manager._original_sigterm = original_handler + else: + fresh_cleanup_manager._original_sigint = original_handler + + restored: list[Any] = [] + + def capture_signal(sig: int, handler: Any) -> None: + restored.append((sig, handler)) + + with ( + patch("signal.signal", side_effect=capture_signal), + patch("signal.raise_signal"), + ): + fresh_cleanup_manager._signal_handler(signum, None) + + assert restored == [(signum, expected_restored)] + class TestCleanupManagerIntegration: """Integration tests using the real singleton.""" From 2de08bae6bd22d31536bb841cd4be277bd333138 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 08:42:13 +0100 Subject: [PATCH 05/28] refactor: centralise merged data file path via DataMerger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidate all knowledge of where the merged data model file lives into DataMerger, eliminating independent path reconstruction across callers. - Rename DEFAULT_MERGED_DATA_FILENAME → MERGED_DATA_FILENAME; the DEFAULT_ prefix implied variability that no longer exists since the --merged-data-filename CLI option was removed - Add MERGED_DATA_FILE_MODE = 0o600 constant (owner read/write only) - Add DataMerger.merged_data_path() as the single query point for the file location; callers no longer construct output_dir / MERGED_DATA_FILENAME - write_merged_data_model() now returns the Path it wrote, owns the chmod call, and takes no filename argument — "write this sensitive file securely" is a single responsibility - Move merged_data_path injection into DeviceExecutor constructor so the orchestrator (which already holds the path) passes it down rather than DeviceExecutor re-deriving it from base_output_dir; removes the DataMerger import from device_executor.py entirely --- nac_test/cli/main.py | 9 +---- nac_test/core/constants.py | 4 ++- nac_test/data_merger.py | 36 ++++++++++++++++--- .../execution/device/device_executor.py | 6 ++-- nac_test/pyats_core/orchestrator.py | 9 ++--- .../test_device_executor_env_propagation.py | 2 ++ tests/unit/test_data_merger.py | 31 ++++++++-------- 7 files changed, 61 insertions(+), 36 deletions(-) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 221afd92..84d50dad 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -4,8 +4,6 @@ """CLI entry point for nac-test.""" import logging -import os -import stat from datetime import datetime from pathlib import Path from typing import Annotated, NoReturn @@ -22,7 +20,6 @@ from nac_test.combined_orchestrator import CombinedOrchestrator from nac_test.core.constants import ( DEBUG_MODE, - DEFAULT_MERGED_DATA_FILENAME, EXIT_DATA_ERROR, EXIT_ERROR, EXIT_INTERRUPTED, @@ -387,11 +384,7 @@ def main( typer.echo("\n\n📄 Merging data model files...") merged_data = DataMerger.merge_data_files(data) - DataMerger.write_merged_data_model(merged_data, output) - - merged_data_path = output / DEFAULT_MERGED_DATA_FILENAME - if os.name != "nt": - os.chmod(merged_data_path, stat.S_IRUSR | stat.S_IWUSR) + merged_data_path = DataMerger.write_merged_data_model(merged_data, output) # Register merged data file for cleanup on exit/signal (SIGTERM/SIGINT) cleanup_manager = get_cleanup_manager() diff --git a/nac_test/core/constants.py b/nac_test/core/constants.py index 640b2916..ddc0c334 100644 --- a/nac_test/core/constants.py +++ b/nac_test/core/constants.py @@ -84,7 +84,9 @@ REPORT_HTML: str = "report.html" XUNIT_XML: str = "xunit.xml" ORDERING_FILENAME: str = "ordering.txt" -DEFAULT_MERGED_DATA_FILENAME: str = "merged_data_model_test_variables.yaml" +MERGED_DATA_FILENAME: str = "merged_data_model_test_variables.yaml" +# Owner read/write only — prevents other users from reading sensitive merged data +MERGED_DATA_FILE_MODE: int = 0o600 SUMMARY_SEPARATOR_WIDTH: int = 70 # HTTP status code range boundaries diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index 40a89cd4..07efe84a 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -4,12 +4,17 @@ """Shared data merging utilities for both Robot and PyATS test execution.""" import logging +import os from pathlib import Path from typing import Any from nac_yaml import yaml -from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME +from nac_test.core.constants import ( + IS_WINDOWS, + MERGED_DATA_FILE_MODE, + MERGED_DATA_FILENAME, +) logger = logging.getLogger(__name__) @@ -34,20 +39,41 @@ def merge_data_files(data_paths: list[Path]) -> dict[str, Any]: # Ensure we always return a dict, even if yaml returns None return data if isinstance(data, dict) else {} + @staticmethod + def merged_data_path(output_directory: Path) -> Path: + """Return the path where the merged data model file will be written. + + Single source of truth for locating the merged data file — use this + instead of constructing the path manually from the constant. + + Args: + output_directory: Base output directory passed to write_merged_data_model() + + Returns: + Full path to the merged data model YAML file + """ + return output_directory / MERGED_DATA_FILENAME + @staticmethod def write_merged_data_model( data: dict[str, Any], output_directory: Path, - ) -> None: + ) -> Path: """Write merged data model to YAML file. - The output filename is always DEFAULT_MERGED_DATA_FILENAME — the single - fixed location used by all consumers (Robot, PyATS subprocesses, cleanup). + The output filename is always MERGED_DATA_FILENAME — the single fixed + location used by all consumers (Robot, PyATS subprocesses, cleanup). Args: data: The merged data dictionary to write output_directory: Directory where the YAML file will be saved + + Returns: + Path to the written file (use this instead of reconstructing the path) """ - full_output_path = output_directory / DEFAULT_MERGED_DATA_FILENAME + full_output_path = DataMerger.merged_data_path(output_directory) logger.info("Writing merged data model to %s", full_output_path) yaml.write_yaml_file(data, full_output_path) + if not IS_WINDOWS: + os.chmod(full_output_path, MERGED_DATA_FILE_MODE) + return full_output_path diff --git a/nac_test/pyats_core/execution/device/device_executor.py b/nac_test/pyats_core/execution/device/device_executor.py index 5cf6a5e1..065a2995 100644 --- a/nac_test/pyats_core/execution/device/device_executor.py +++ b/nac_test/pyats_core/execution/device/device_executor.py @@ -11,7 +11,6 @@ from pathlib import Path from typing import Any -from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME from nac_test.pyats_core.constants import ENV_TEST_DIR from nac_test.pyats_core.execution.job_generator import JobGenerator from nac_test.pyats_core.execution.subprocess_runner import SubprocessRunner @@ -31,6 +30,7 @@ def __init__( test_status: dict[str, Any], test_dir: Path, base_output_dir: Path, + merged_data_path: Path, custom_testbed_path: Path | None = None, ): """Initialize device executor. @@ -41,6 +41,7 @@ def __init__( test_status: Dictionary for tracking test status test_dir: Directory containing PyATS test files (user-specified) base_output_dir: Base output directory for test results + merged_data_path: Path to the merged data model YAML file custom_testbed_path: Optional path to custom PyATS testbed YAML """ self.job_generator = job_generator @@ -48,6 +49,7 @@ def __init__( self.test_status = test_status self.test_dir = test_dir self.base_output_dir = base_output_dir + self.merged_data_path = merged_data_path self.custom_testbed_path = custom_testbed_path async def run_device_job_with_semaphore( @@ -106,7 +108,7 @@ async def run_device_job_with_semaphore( "HOSTNAME": hostname, "DEVICE_INFO": json.dumps(device), "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH": str( - self.base_output_dir / DEFAULT_MERGED_DATA_FILENAME + self.merged_data_path ), "NAC_TEST_TYPE": "d2d", ENV_TEST_DIR: str(self.test_dir), diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py index 3e046541..76b42afb 100644 --- a/nac_test/pyats_core/orchestrator.py +++ b/nac_test/pyats_core/orchestrator.py @@ -15,7 +15,6 @@ from nac_test.core.constants import ( DEBUG_MODE, - DEFAULT_MERGED_DATA_FILENAME, DRY_RUN_REASON, EXIT_ERROR, PYATS_RESULTS_DIRNAME, @@ -23,6 +22,7 @@ SUMMARY_SEPARATOR_WIDTH, ) from nac_test.core.types import PyATSResults, TestResults +from nac_test.data_merger import DataMerger from nac_test.pyats_core.broker.connection_broker import ConnectionBroker from nac_test.pyats_core.constants import ( DEFAULT_CPU_MULTIPLIER, @@ -97,7 +97,7 @@ def __init__( self.output_dir = ( self.base_output_dir / PYATS_RESULTS_DIRNAME ) # PyATS works in its own subdirectory - self.merged_data_filename = DEFAULT_MERGED_DATA_FILENAME + self.merged_data_path = DataMerger.merged_data_path(self.base_output_dir) self.minimal_reports = minimal_reports self.custom_testbed_path = custom_testbed_path self.dry_run = dry_run @@ -139,7 +139,7 @@ def __init__( # Initialize discovery components self.test_discovery = TestDiscovery(self.test_dir) self.device_inventory_discovery = DeviceInventoryDiscovery( - self.base_output_dir / self.merged_data_filename + self.merged_data_path ) # Initialize execution components @@ -309,7 +309,7 @@ async def _execute_api_tests_standard(self, test_files: list[Path]) -> Path | No # The merged data file is created by main.py at the base output level. # Pass absolute path so the child process (with cwd set) can locate it. env["MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH"] = str( - (self.base_output_dir / self.merged_data_filename).absolute() + self.merged_data_path.absolute() ) # Set NAC_TEST_TYPE to differentiate API vs D2D test types for separate temp directories # This prevents race conditions where both test types write JSONL files to the same location @@ -429,6 +429,7 @@ async def _execute_device_tests_with_broker( self.d2d_test_status, # Use d2d_test_status for device tests self.test_dir, self.base_output_dir, + self.merged_data_path, self.custom_testbed_path, ) diff --git a/tests/pyats_core/test_device_executor_env_propagation.py b/tests/pyats_core/test_device_executor_env_propagation.py index 59dc2b45..f622f510 100644 --- a/tests/pyats_core/test_device_executor_env_propagation.py +++ b/tests/pyats_core/test_device_executor_env_propagation.py @@ -8,6 +8,7 @@ from typing import Any from unittest.mock import AsyncMock, MagicMock, patch +from nac_test.data_merger import DataMerger from nac_test.pyats_core.constants import ENV_TEST_DIR from nac_test.pyats_core.execution.device.device_executor import DeviceExecutor from nac_test.pyats_core.execution.device.testbed_generator import TestbedGenerator @@ -51,6 +52,7 @@ async def capture_execute_job_with_testbed( test_status={}, test_dir=pyats_test_dirs.test_dir, base_output_dir=pyats_test_dirs.output_dir, + merged_data_path=DataMerger.merged_data_path(pyats_test_dirs.output_dir), ) device: dict[str, Any] = { diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index fd67a6b9..3cbd8d6e 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -10,7 +10,8 @@ from pathlib import Path -from nac_test.core.constants import DEFAULT_MERGED_DATA_FILENAME +from nac_yaml import yaml + from nac_test.data_merger import DataMerger FIXTURES_DIR = Path("tests/unit/fixtures/data_merge") @@ -83,25 +84,26 @@ def test_merge_empty_list_returns_empty_dict(self) -> None: class TestWriteMergedDataModel: """Tests for DataMerger.write_merged_data_model().""" - def test_writes_to_default_filename(self, tmp_path: Path) -> None: - """Output file is always named DEFAULT_MERGED_DATA_FILENAME.""" - DataMerger.write_merged_data_model({"key": "value"}, tmp_path) - expected = tmp_path / DEFAULT_MERGED_DATA_FILENAME - assert expected.exists(), f"Expected {expected} to exist" + def test_returns_path_to_written_file(self, tmp_path: Path) -> None: + """write_merged_data_model returns the path of the file it created.""" + returned = DataMerger.write_merged_data_model({"key": "value"}, tmp_path) + assert returned == DataMerger.merged_data_path(tmp_path) + assert returned.exists() def test_writes_no_extra_files(self, tmp_path: Path) -> None: """Exactly one file is created in the output directory.""" DataMerger.write_merged_data_model({"key": "value"}, tmp_path) - files = list(tmp_path.iterdir()) - assert len(files) == 1 + assert len(list(tmp_path.iterdir())) == 1 + + def test_merged_data_path_matches_written_file(self, tmp_path: Path) -> None: + """merged_data_path() returns the same path as the file written by write_merged_data_model().""" + returned = DataMerger.write_merged_data_model({"key": "value"}, tmp_path) + assert DataMerger.merged_data_path(tmp_path) == returned def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: """Data written to YAML can be read back with the same structure.""" - from nac_yaml import yaml - original = {"host": "router1", "vlan": 100, "tags": ["a", "b"]} - DataMerger.write_merged_data_model(original, tmp_path) - output_path = tmp_path / DEFAULT_MERGED_DATA_FILENAME + output_path = DataMerger.write_merged_data_model(original, tmp_path) reloaded = yaml.load_yaml_files([output_path]) assert reloaded["host"] == "router1" assert reloaded["vlan"] == 100 @@ -109,13 +111,10 @@ def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: def test_roundtrip_preserves_merged_fixture_content(self, tmp_path: Path) -> None: """Full merge result can be written and reloaded without data loss.""" - from nac_yaml import yaml - merged = DataMerger.merge_data_files( [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] ) - DataMerger.write_merged_data_model(merged, tmp_path) - output_path = tmp_path / DEFAULT_MERGED_DATA_FILENAME + output_path = DataMerger.write_merged_data_model(merged, tmp_path) reloaded = yaml.load_yaml_files([output_path]) assert reloaded["root"]["attr1"] == "value1" assert reloaded["root"]["attr2"] == "value2" From b5984a88595ec95afbc0dec28f7fcd5b8ec93fed Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 08:52:31 +0100 Subject: [PATCH 06/28] refactor: remove exit_with_cleanup closure from main() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit typer.Exit is caught by Click internally, which calls sys.exit() — this triggers atexit handlers reliably on every exit path. The exit_with_cleanup closure (which called cleanup_now() before raising typer.Exit) was therefore redundant with the atexit handler already registered by CleanupManager. Replace all exit_with_cleanup() call sites with plain raise typer.Exit(), remove the closure, and drop the now-unused NoReturn import. --- nac_test/cli/main.py | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 84d50dad..e57b6aa4 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -6,7 +6,7 @@ import logging from datetime import datetime from pathlib import Path -from typing import Annotated, NoReturn +from typing import Annotated import typer from rich.console import Console @@ -393,10 +393,6 @@ def main( duration = (datetime.now() - start_time).total_seconds() typer.echo(f"✅ Data model merging completed ({format_duration(duration)})") - def exit_with_cleanup(code: int) -> NoReturn: - cleanup_manager.cleanup_now() - raise typer.Exit(code) - # CombinedOrchestrator - handles both dev and production modes (uses pre-created merged data) orchestrator = CombinedOrchestrator( data_paths=data, @@ -432,13 +428,10 @@ def exit_with_cleanup(code: int) -> NoReturn: fg=typer.colors.YELLOW, ) ) - exit_with_cleanup(EXIT_INTERRUPTED) + raise typer.Exit(EXIT_INTERRUPTED) from None except Exception as e: typer.echo(f"Error during execution: {e}") - if DEBUG_MODE: - cleanup_manager.cleanup_now() - raise typer.Exit(EXIT_ERROR) from e - exit_with_cleanup(EXIT_ERROR) + raise typer.Exit(EXIT_ERROR) from e # Display total runtime before exit runtime_end = datetime.now() @@ -453,9 +446,9 @@ def exit_with_cleanup(code: int) -> NoReturn: typer.echo(f"\n❌ Template rendering failed: {reason}", err=True) else: typer.echo("\n❌ Template rendering failed", err=True) - exit_with_cleanup(stats.exit_code) + raise typer.Exit(stats.exit_code) typer.echo("\n✅ Templates rendered successfully (render-only mode)") - exit_with_cleanup(0) + raise typer.Exit(0) if stats.pre_flight_failure is not None: pf = stats.pre_flight_failure @@ -463,22 +456,22 @@ def exit_with_cleanup(code: int) -> NoReturn: f"\n❌ Pre-flight failure ({pf.failure_type.display_name})", err=True, ) - exit_with_cleanup(stats.exit_code) + raise typer.Exit(stats.exit_code) if stats.has_errors: error_list = "; ".join(stats.errors) typer.echo(f"\n❌ Execution errors: {error_list}", err=True) - exit_with_cleanup(stats.exit_code) + raise typer.Exit(stats.exit_code) if stats.has_failures: typer.echo( f"\n❌ Tests failed: {stats.failed} out of {stats.total} tests", err=True ) - exit_with_cleanup(stats.exit_code) + raise typer.Exit(stats.exit_code) if stats.is_empty: typer.echo("\n⚠️ No tests were executed", err=True) - exit_with_cleanup(stats.exit_code) + raise typer.Exit(stats.exit_code) typer.echo(f"\n✅ All tests passed: {stats.passed} out of {stats.total} tests") - exit_with_cleanup(0) + raise typer.Exit(0) From 211b88d9de2177a79b368d775043d3acf10166db Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 09:13:41 +0100 Subject: [PATCH 07/28] feat: preserve debug-relevant files when NAC_TEST_DEBUG is set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CleanupManager.register() gains a skip_if_debug parameter. Files registered with skip_if_debug=True are deleted on normal exit but retained when NAC_TEST_DEBUG is set, letting developers inspect intermediate artifacts without any extra flags or code changes. The merged data model YAML is the first consumer: it now registers with skip_if_debug=True so it survives for inspection in debug runs. The same mechanism is available for job scripts, testbed YAMLs, and any other artifacts tracked in PR #697. Internally, _files changes from set[Path] to dict[Path, bool], keeping the flag co-located with the path and avoiding two collections to sync. Remove the exit_with_cleanup() closure from main() — typer.Exit is caught by Click which calls sys.exit(), reliably triggering atexit handlers on every exit path. The closure and all cleanup_now() pre-exit calls were redundant with the already-registered atexit handler. --- nac_test/cli/main.py | 2 +- nac_test/utils/cleanup.py | 32 +++++++-- tests/unit/utils/test_cleanup.py | 109 +++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 7 deletions(-) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index e57b6aa4..11354bd7 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -388,7 +388,7 @@ def main( # Register merged data file for cleanup on exit/signal (SIGTERM/SIGINT) cleanup_manager = get_cleanup_manager() - cleanup_manager.register(merged_data_path) + cleanup_manager.register(merged_data_path, skip_if_debug=True) duration = (datetime.now() - start_time).total_seconds() typer.echo(f"✅ Data model merging completed ({format_duration(duration)})") diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index 69ff9112..e9141d2e 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -14,6 +14,7 @@ from types import FrameType from typing import Any +from nac_test.core.constants import DEBUG_MODE from nac_test.pyats_core.discovery.test_type_resolver import VALID_TEST_TYPES logger = logging.getLogger(__name__) @@ -34,6 +35,9 @@ class CleanupManager: cleanup_manager.register(Path("/tmp/sensitive_file.yaml")) # File will be automatically removed on exit + # Skip deletion when NAC_TEST_DEBUG is set (useful for debugging): + cleanup_manager.register(Path("/tmp/temp_job.py"), skip_if_debug=True) + Thread Safety: All operations are thread-safe via internal locking. """ @@ -55,7 +59,7 @@ def __init__(self) -> None: if self._initialized: return - self._files: set[Path] = set() + self._files: dict[Path, bool] = {} # path → skip_if_debug self._lock = threading.Lock() self._original_sigterm: Any = None self._original_sigint: Any = None @@ -105,17 +109,22 @@ def _signal_handler(self, signum: int, frame: FrameType | None) -> None: # For SIGTERM, re-send the signal signal.raise_signal(signum) - def register(self, path: Path) -> None: + def register(self, path: Path, skip_if_debug: bool = False) -> None: """Register a file path for cleanup on exit. Args: path: Path to file that should be removed on exit. Directories are not supported (use shutil.rmtree directly). + skip_if_debug: If True, the file will not be deleted when + NAC_TEST_DEBUG is set — useful for intermediate files + (job scripts, testbed YAMLs, merged data) that aid debugging. """ with self._lock: resolved = path.resolve() - self._files.add(resolved) - logger.debug(f"Registered for cleanup: {resolved}") + self._files[resolved] = skip_if_debug + logger.debug( + f"Registered for cleanup: {resolved} (skip_if_debug={skip_if_debug})" + ) def unregister(self, path: Path) -> None: """Unregister a file path from cleanup. @@ -125,7 +134,7 @@ def unregister(self, path: Path) -> None: """ with self._lock: resolved = path.resolve() - self._files.discard(resolved) + self._files.pop(resolved, None) logger.debug(f"Unregistered from cleanup: {resolved}") def _cleanup(self) -> None: @@ -135,7 +144,11 @@ def _cleanup(self) -> None: return self._cleanup_done = True - for path in self._files: + debug_kept: list[Path] = [] + for path, skip_if_debug in self._files.items(): + if skip_if_debug and DEBUG_MODE: + debug_kept.append(path) + continue try: if path.exists(): path.unlink() @@ -143,6 +156,13 @@ def _cleanup(self) -> None: except Exception as e: logger.warning(f"Failed to clean up {path}: {e}") + if debug_kept: + logger.info( + "Keeping %d file(s) for debugging (NAC_TEST_DEBUG is set): %s", + len(debug_kept), + ", ".join(str(p) for p in debug_kept), + ) + self._files.clear() def cleanup_now(self) -> None: diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index c77bafea..3f239459 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -60,6 +60,28 @@ def test_register_adds_path( assert test_file.resolve() in fresh_cleanup_manager._files + def test_register_default_flag_is_false( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """register() without skip_if_debug stores False (always delete).""" + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + + assert fresh_cleanup_manager._files[test_file.resolve()] is False + + def test_register_skip_if_debug_stores_true( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """register(skip_if_debug=True) stores True for the path.""" + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file, skip_if_debug=True) + + assert fresh_cleanup_manager._files[test_file.resolve()] is True + def test_register_multiple_files( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: @@ -73,6 +95,19 @@ def test_register_multiple_files( assert len(fresh_cleanup_manager._files) == 2 + def test_unregister_prevents_deletion( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """A file removed from the registry is not deleted on cleanup.""" + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + fresh_cleanup_manager.unregister(test_file) + fresh_cleanup_manager.cleanup_now() + + assert test_file.exists() + def test_unregister_removes_path( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: @@ -84,6 +119,18 @@ def test_unregister_removes_path( assert test_file.resolve() not in fresh_cleanup_manager._files + def test_unregister_removes_skip_if_debug_path( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """unregister() removes paths registered with skip_if_debug=True.""" + test_file = tmp_path / "test.txt" + test_file.touch() + + fresh_cleanup_manager.register(test_file, skip_if_debug=True) + fresh_cleanup_manager.unregister(test_file) + + assert test_file.resolve() not in fresh_cleanup_manager._files + def test_unregister_nonexistent_path_is_safe( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: @@ -151,6 +198,68 @@ def test_cleanup_continues_after_single_file_error( assert fresh_cleanup_manager._cleanup_done +class TestCleanupManagerSkipIfDebug: + """Tests for skip_if_debug behaviour during cleanup.""" + + @pytest.mark.parametrize( + ("debug_mode", "expected_exists"), + [ + (False, False), # debug off → file is deleted + (True, True), # debug on → file is kept + ], + ids=["debug_off_deletes", "debug_on_keeps"], + ) + def test_skip_if_debug_respects_debug_mode( + self, + fresh_cleanup_manager: CleanupManager, + tmp_path: Path, + debug_mode: bool, + expected_exists: bool, + ) -> None: + """Files registered with skip_if_debug=True are kept iff NAC_TEST_DEBUG is set.""" + test_file = tmp_path / "job.py" + test_file.touch() + + fresh_cleanup_manager.register(test_file, skip_if_debug=True) + + with patch("nac_test.utils.cleanup.DEBUG_MODE", debug_mode): + fresh_cleanup_manager.cleanup_now() + + assert test_file.exists() is expected_exists + + def test_normal_files_always_deleted_regardless_of_debug( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """Files registered without skip_if_debug are always deleted, even in debug mode.""" + test_file = tmp_path / "sensitive.yaml" + test_file.touch() + + fresh_cleanup_manager.register(test_file) + + with patch("nac_test.utils.cleanup.DEBUG_MODE", True): + fresh_cleanup_manager.cleanup_now() + + assert not test_file.exists() + + def test_mixed_registration_in_debug_mode( + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + ) -> None: + """In debug mode, sensitive files are deleted but debug-skipped files are kept.""" + sensitive = tmp_path / "merged_data.yaml" + debug_file = tmp_path / "job.py" + sensitive.touch() + debug_file.touch() + + fresh_cleanup_manager.register(sensitive) + fresh_cleanup_manager.register(debug_file, skip_if_debug=True) + + with patch("nac_test.utils.cleanup.DEBUG_MODE", True): + fresh_cleanup_manager.cleanup_now() + + assert not sensitive.exists() + assert debug_file.exists() + + class TestCleanupManagerThreadSafety: """Tests for thread-safe behavior.""" From 64bba1e3ddc1f3c60ea31833450661035de97997 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 09:18:53 +0100 Subject: [PATCH 08/28] test: assert merged data YAML is absent after a successful run --- tests/e2e/test_e2e_scenarios.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/e2e/test_e2e_scenarios.py b/tests/e2e/test_e2e_scenarios.py index 75e61099..f4d88b09 100644 --- a/tests/e2e/test_e2e_scenarios.py +++ b/tests/e2e/test_e2e_scenarios.py @@ -29,6 +29,7 @@ HTML_REPORTS_DIRNAME, IS_WINDOWS, LOG_HTML, + MERGED_DATA_FILENAME, OUTPUT_XML, PRE_FLIGHT_FAILURE_FILENAME, PYATS_RESULTS_DIRNAME, @@ -166,7 +167,6 @@ def test_output_root_contains_only_expected_entries( expected_files = { COMBINED_SUMMARY_FILENAME, - "merged_data_model_test_variables.yaml", } if results.has_robot_results or results.has_pyats_results: expected_files.add(XUNIT_XML) @@ -182,6 +182,18 @@ def test_output_root_contains_only_expected_entries( f"Expected only: {sorted(allowed)}" ) + def test_merged_data_file_removed_after_run(self, results: E2EResults) -> None: + """Merged data model YAML must not persist after a successful run. + + The file contains potentially sensitive variable data and is registered + with CleanupManager for deletion on exit. Its absence confirms cleanup ran. + """ + merged_data_file = results.output_dir / MERGED_DATA_FILENAME + assert not merged_data_file.exists(), ( + f"{MERGED_DATA_FILENAME} was not cleaned up after run — " + "it may contain sensitive data and should be deleted on exit" + ) + # ------------------------------------------------------------------------- # Robot Framework Output Tests # ------------------------------------------------------------------------- From ead4ae11bceac35e798714f8e5e1135280553b03 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 09:25:51 +0100 Subject: [PATCH 09/28] restore comments in main --- nac_test/cli/main.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 11354bd7..06eb54d6 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -422,15 +422,20 @@ def main( try: stats = orchestrator.run_tests() except KeyboardInterrupt: + # Handle Ctrl+C interruption gracefully typer.echo( typer.style( "\n⚠️ Test execution was interrupted by user (Ctrl+C)", fg=typer.colors.YELLOW, ) ) + # Exit with code 253 following Robot Framework convention raise typer.Exit(EXIT_INTERRUPTED) from None except Exception as e: + # Infrastructure errors (template rendering, controller detection, etc.) typer.echo(f"Error during execution: {e}") + # Pretty exception display is controlled by pretty_exceptions_enable=DEBUG_MODE + # on the Typer app — no need for a separate DEBUG_MODE branch here. raise typer.Exit(EXIT_ERROR) from e # Display total runtime before exit From a79a3b2cc3bd4df91be4bec1cf96ff6f51b2087d Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 09:39:58 +0100 Subject: [PATCH 10/28] refactor: place merged_data before optional params in orchestrator signatures --- nac_test/cli/main.py | 4 ++-- nac_test/combined_orchestrator.py | 12 ++++++------ nac_test/robot/orchestrator.py | 10 +++++----- tests/unit/robot/test_orchestrator.py | 8 ++++---- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 06eb54d6..7fc77caa 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -397,8 +397,9 @@ def main( orchestrator = CombinedOrchestrator( data_paths=data, templates_dir=templates, - custom_testbed_path=testbed, output_dir=output, + merged_data=merged_data, + custom_testbed_path=testbed, filters_path=filters, tests_path=tests, include_tags=include, @@ -413,7 +414,6 @@ def main( dev_pyats_only=pyats, dev_robot_only=robot, verbose=verbose, - merged_data=merged_data, ) # Track total runtime for benchmarking diff --git a/nac_test/combined_orchestrator.py b/nac_test/combined_orchestrator.py index cfdfb74f..d7cdf339 100644 --- a/nac_test/combined_orchestrator.py +++ b/nac_test/combined_orchestrator.py @@ -75,6 +75,7 @@ def __init__( data_paths: list[Path], templates_dir: Path, output_dir: Path, + merged_data: dict[str, Any] | None = None, filters_path: Path | None = None, tests_path: Path | None = None, include_tags: list[str] | None = None, @@ -90,7 +91,6 @@ def __init__( processes: int | None = None, extra_args: ValidatedRobotArgs | None = None, verbose: bool = False, - merged_data: dict[str, Any] | None = None, ): """Initialize the combined orchestrator. @@ -98,6 +98,7 @@ def __init__( data_paths: List of paths to data model YAML files templates_dir: Directory containing test templates and PyATS test files output_dir: Base directory for test output + merged_data: Already-loaded merged data model dict (avoids re-reading from disk) filters_path: Path to Jinja filters (Robot only) tests_path: Path to Jinja tests (Robot only) include_tags: Tags to include (Robot only) @@ -113,11 +114,13 @@ def __init__( dev_pyats_only: Development mode - run only PyATS tests (skip Robot) dev_robot_only: Development mode - run only Robot Framework tests (skip PyATS) verbose: Enable verbose mode - keeps archive files, enables verbose output - merged_data: Already-loaded merged data model dict (avoids re-reading from disk) """ self.data_paths = data_paths self.templates_dir = Path(templates_dir) self.output_dir = Path(output_dir) + self.merged_data: dict[str, Any] = ( + merged_data if merged_data is not None else {} + ) # Robot-specific parameters self.filters_path = filters_path @@ -139,9 +142,6 @@ def __init__( self.dev_pyats_only = dev_pyats_only self.dev_robot_only = dev_robot_only self.verbose = verbose - self.merged_data: dict[str, Any] = ( - merged_data if merged_data is not None else {} - ) # Controller type — detected lazily in run_tests() when PyATS tests are present self.controller_type: ControllerTypeKey | None = None @@ -237,6 +237,7 @@ def run_tests(self) -> CombinedResults: robot_orchestrator = RobotOrchestrator( templates_dir=self.templates_dir, output_dir=self.output_dir, + merged_data=self.merged_data, filters_path=self.filters_path, tests_path=self.tests_path, include_tags=self.include_tags, @@ -247,7 +248,6 @@ def run_tests(self) -> CombinedResults: extra_args=self.extra_args, loglevel=self.loglevel, verbose=self.verbose, - merged_data=self.merged_data, ) try: robot_results = robot_orchestrator.run_tests() diff --git a/nac_test/robot/orchestrator.py b/nac_test/robot/orchestrator.py index 1e951df0..ad168291 100644 --- a/nac_test/robot/orchestrator.py +++ b/nac_test/robot/orchestrator.py @@ -49,6 +49,7 @@ def __init__( self, templates_dir: Path, output_dir: Path, + merged_data: dict[str, Any] | None = None, filters_path: Path | None = None, tests_path: Path | None = None, include_tags: list[str] | None = None, @@ -59,13 +60,13 @@ def __init__( extra_args: ValidatedRobotArgs | None = None, loglevel: LogLevel = DEFAULT_LOGLEVEL, verbose: bool = False, - merged_data: dict[str, Any] | None = None, ): """Initialize the Robot Framework orchestrator. Args: templates_dir: Directory containing Robot template files output_dir: Base output directory (orchestrator uses its robot_results subdirectory) + merged_data: Already-loaded merged data model dict (avoids re-reading from disk) filters_path: Optional path to filter files tests_path: Optional path to test files include_tags: Optional list of tags to include @@ -76,11 +77,13 @@ def __init__( extra_args: Additional Robot Framework arguments to pass to pabot loglevel: Log level verbose: Enable verbose mode - enables verbose output for pabot - merged_data: Already-loaded merged data model dict (avoids re-reading from disk) """ self.templates_dir = Path(templates_dir) self.base_output_dir = Path(output_dir) self.output_dir = self.base_output_dir / ROBOT_RESULTS_DIRNAME + self.merged_data: dict[str, Any] = ( + merged_data if merged_data is not None else {} + ) # Robot-specific parameters self.filters_path = filters_path @@ -93,9 +96,6 @@ def __init__( self.extra_args = extra_args self.loglevel = loglevel self.verbose = verbose - self.merged_data: dict[str, Any] = ( - merged_data if merged_data is not None else {} - ) # Determine if ordering file should be used for test-level parallelization if not DISABLE_TESTLEVELSPLIT: diff --git a/tests/unit/robot/test_orchestrator.py b/tests/unit/robot/test_orchestrator.py index 68ee1be0..35653305 100644 --- a/tests/unit/robot/test_orchestrator.py +++ b/tests/unit/robot/test_orchestrator.py @@ -55,8 +55,8 @@ def orchestrator(mock_templates_dir, temp_output_dir, merged_data) -> RobotOrche return RobotOrchestrator( templates_dir=mock_templates_dir, output_dir=temp_output_dir, - loglevel=DEFAULT_LOGLEVEL, merged_data=merged_data, + loglevel=DEFAULT_LOGLEVEL, ) @@ -85,6 +85,7 @@ def test_initialization_with_optional_params( orchestrator = RobotOrchestrator( templates_dir=mock_templates_dir, output_dir=temp_output_dir, + merged_data={"test": "data"}, include_tags=["smoke", "regression"], exclude_tags=["wip"], render_only=True, @@ -92,7 +93,6 @@ def test_initialization_with_optional_params( processes=4, extra_args=ValidatedRobotArgs(args=["--exitonfailure"], robot_opts={}), loglevel=LogLevel.DEBUG, - merged_data={"test": "data"}, ) assert orchestrator.include_tags == ["smoke", "regression"] @@ -340,9 +340,9 @@ def test_verbose_flag_passed_to_pabot( orchestrator = RobotOrchestrator( templates_dir=mock_templates_dir, output_dir=temp_output_dir, + merged_data={"test": "data"}, verbose=verbose, loglevel=loglevel, - merged_data={"test": "data"}, ) orchestrator.robot_writer.write = MagicMock() mock_pabot.return_value = 0 @@ -394,9 +394,9 @@ def test_include_exclude_tags_passed_to_pabot( orchestrator = RobotOrchestrator( templates_dir=mock_templates_dir, output_dir=temp_output_dir, + merged_data={"test": "data"}, include_tags=include_tags, exclude_tags=exclude_tags, - merged_data={"test": "data"}, ) orchestrator.robot_writer.write = MagicMock() mock_pabot.return_value = 0 From 60079051da3d70a72a623ebe0f5d51690a2297f4 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 10:13:42 +0100 Subject: [PATCH 11/28] fix: log full exception traceback to file while keeping terminal output clean (#573) Add logger.exception() before the typer.echo() in the top-level exception handler so the full stack trace (including chained causes) is captured in the log file. Terminal output remains a single clean line for end users. --- nac_test/cli/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 7fc77caa..014bf3b2 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -433,6 +433,7 @@ def main( raise typer.Exit(EXIT_INTERRUPTED) from None except Exception as e: # Infrastructure errors (template rendering, controller detection, etc.) + logger.exception("Unexpected error during execution") typer.echo(f"Error during execution: {e}") # Pretty exception display is controlled by pretty_exceptions_enable=DEBUG_MODE # on the Typer app — no need for a separate DEBUG_MODE branch here. From 403db83df9a4f9d06e78e4f2ca90d1d037d30cc2 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 10:24:55 +0100 Subject: [PATCH 12/28] refactor: resolve merged_data_path to absolute once at construction Call .absolute() when assigning self.merged_data_path in PyATSOrchestrator so all use sites get a consistent absolute path without redundant calls. Move the explanatory comment to the assignment and rephrase it to not imply a cwd change is always in effect. --- nac_test/pyats_core/orchestrator.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py index 76b42afb..13520042 100644 --- a/nac_test/pyats_core/orchestrator.py +++ b/nac_test/pyats_core/orchestrator.py @@ -97,7 +97,10 @@ def __init__( self.output_dir = ( self.base_output_dir / PYATS_RESULTS_DIRNAME ) # PyATS works in its own subdirectory - self.merged_data_path = DataMerger.merged_data_path(self.base_output_dir) + # Absolute path so child processes can locate it regardless of working directory. + self.merged_data_path = DataMerger.merged_data_path( + self.base_output_dir + ).absolute() self.minimal_reports = minimal_reports self.custom_testbed_path = custom_testbed_path self.dry_run = dry_run @@ -306,10 +309,8 @@ async def _execute_api_tests_standard(self, test_files: list[Path]) -> Path | No # We cannot pass Python objects across process boundaries # so we use env vars to communicate # configuration (like data file paths) from the orchestrator to the test subprocess. - # The merged data file is created by main.py at the base output level. - # Pass absolute path so the child process (with cwd set) can locate it. env["MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH"] = str( - self.merged_data_path.absolute() + self.merged_data_path ) # Set NAC_TEST_TYPE to differentiate API vs D2D test types for separate temp directories # This prevents race conditions where both test types write JSONL files to the same location From 7891146aa2921fb33897b9f2753dc05521145274 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 11:05:08 +0100 Subject: [PATCH 13/28] register DeviceExecutor temp files with CleanupManager The per-device job (.py) and testbed (.yaml) files created with NamedTemporaryFile(delete=False) were never deleted, accumulating in the OS temp directory across runs. Register them with CleanupManager so they are removed on normal exit, SIGTERM, and SIGINT. skip_if_debug=True is used consistently with merged_data_model_test_variables.yaml so that NAC_TEST_DEBUG retains all intermediate artifacts for debugging. Also removes the longstanding "UNCOMMENT ME" commented-out deletion block that this replaces. --- .../pyats_core/execution/device/device_executor.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/nac_test/pyats_core/execution/device/device_executor.py b/nac_test/pyats_core/execution/device/device_executor.py index 065a2995..c2c30825 100644 --- a/nac_test/pyats_core/execution/device/device_executor.py +++ b/nac_test/pyats_core/execution/device/device_executor.py @@ -14,6 +14,7 @@ from nac_test.pyats_core.constants import ENV_TEST_DIR from nac_test.pyats_core.execution.job_generator import JobGenerator from nac_test.pyats_core.execution.subprocess_runner import SubprocessRunner +from nac_test.utils.cleanup import get_cleanup_manager from .testbed_generator import TestbedGenerator @@ -100,6 +101,11 @@ async def run_device_job_with_semaphore( testbed_file.write(testbed_content) testbed_file_path = Path(testbed_file.name) + # Register temp files for deletion on exit (kept when NAC_TEST_DEBUG is set). + cleanup = get_cleanup_manager() + cleanup.register(job_file_path, skip_if_debug=True) + cleanup.register(testbed_file_path, skip_if_debug=True) + # Set up environment for this device # Always start with a copy of os.environ to preserve PATH and other variables env = os.environ.copy() @@ -150,13 +156,6 @@ async def run_device_job_with_semaphore( if test_name in self.test_status: self.test_status[test_name]["status"] = status - # Clean up temporary files -- UNCOMMENT ME - # try: - # job_file_path.unlink() - # testbed_file_path.unlink() - # except Exception: - # pass - if archive_path: logger.info( f"Completed tests for device {hostname}: {archive_path}" From ef0e3989e2a28f9bbf229e73f41ad729a9e80836 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 11:07:03 +0100 Subject: [PATCH 14/28] extract _setup_run_tests_mocks helper in test_orchestrator The mock-wiring and artifact-creation block (pabot return value, RobotReportGenerator stub, robot_results/ stub files) was copy-pasted verbatim across test_verbose_flag_passed_to_pabot and test_include_exclude_tags_passed_to_pabot. Extract it into a _setup_run_tests_mocks static helper so future tests can reuse it without duplicating the setup. --- tests/unit/robot/test_orchestrator.py | 57 +++++++++++++++------------ 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/tests/unit/robot/test_orchestrator.py b/tests/unit/robot/test_orchestrator.py index 35653305..a4872ef3 100644 --- a/tests/unit/robot/test_orchestrator.py +++ b/tests/unit/robot/test_orchestrator.py @@ -63,6 +63,33 @@ def orchestrator(mock_templates_dir, temp_output_dir, merged_data) -> RobotOrche class TestRobotOrchestrator: """Test suite for RobotOrchestrator.""" + @staticmethod + def _setup_run_tests_mocks( + mock_generator: MagicMock, + mock_pabot: MagicMock, + orchestrator: RobotOrchestrator, + temp_output_dir: Path, + ) -> None: + """Wire up the standard mocks and artifact files needed to exercise run_tests(). + + Creates the robot_results/ directory with stub artifact files so that + _create_backward_compat_links() and the report generator don't fail. + """ + orchestrator.robot_writer.write = MagicMock() + mock_pabot.return_value = 0 + + mock_generator_instance = MagicMock() + mock_generator_instance.generate_summary_report.return_value = ( + None, + TestResults(), + ) + mock_generator.return_value = mock_generator_instance + + robot_results_dir = temp_output_dir / ROBOT_RESULTS_DIRNAME + robot_results_dir.mkdir(exist_ok=True) + for filename in [LOG_HTML, OUTPUT_XML, REPORT_HTML, XUNIT_XML]: + (robot_results_dir / filename).write_text(f"Mock {filename}") + def test_initialization( self, orchestrator, temp_output_dir, mock_templates_dir ) -> None: @@ -344,20 +371,9 @@ def test_verbose_flag_passed_to_pabot( verbose=verbose, loglevel=loglevel, ) - orchestrator.robot_writer.write = MagicMock() - mock_pabot.return_value = 0 - - mock_generator_instance = MagicMock() - mock_generator_instance.generate_summary_report.return_value = ( - None, - TestResults(), + self._setup_run_tests_mocks( + mock_generator, mock_pabot, orchestrator, temp_output_dir ) - mock_generator.return_value = mock_generator_instance - - robot_results_dir = temp_output_dir / ROBOT_RESULTS_DIRNAME - robot_results_dir.mkdir() - for filename in [LOG_HTML, OUTPUT_XML, REPORT_HTML, XUNIT_XML]: - (robot_results_dir / filename).write_text(f"Mock {filename}") orchestrator.run_tests() @@ -398,20 +414,9 @@ def test_include_exclude_tags_passed_to_pabot( include_tags=include_tags, exclude_tags=exclude_tags, ) - orchestrator.robot_writer.write = MagicMock() - mock_pabot.return_value = 0 - - mock_generator_instance = MagicMock() - mock_generator_instance.generate_summary_report.return_value = ( - None, - TestResults(), + self._setup_run_tests_mocks( + mock_generator, mock_pabot, orchestrator, temp_output_dir ) - mock_generator.return_value = mock_generator_instance - - robot_results_dir = temp_output_dir / ROBOT_RESULTS_DIRNAME - robot_results_dir.mkdir() - for filename in [LOG_HTML, OUTPUT_XML, REPORT_HTML, XUNIT_XML]: - (robot_results_dir / filename).write_text(f"Mock {filename}") orchestrator.run_tests() From 2d3e99350694a8b61954b9ef71ae148cf8778c83 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 11:09:11 +0100 Subject: [PATCH 15/28] tests: remove duplicate test_merged_data_path_matches_written_file test_returns_path_to_written_file already asserts the same equality (with operands swapped) and additionally checks that the file exists, making the duplicate redundant. --- tests/unit/test_data_merger.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index 3cbd8d6e..722b7254 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -95,11 +95,6 @@ def test_writes_no_extra_files(self, tmp_path: Path) -> None: DataMerger.write_merged_data_model({"key": "value"}, tmp_path) assert len(list(tmp_path.iterdir())) == 1 - def test_merged_data_path_matches_written_file(self, tmp_path: Path) -> None: - """merged_data_path() returns the same path as the file written by write_merged_data_model().""" - returned = DataMerger.write_merged_data_model({"key": "value"}, tmp_path) - assert DataMerger.merged_data_path(tmp_path) == returned - def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: """Data written to YAML can be read back with the same structure.""" original = {"host": "router1", "vlan": 100, "tags": ["a", "b"]} From 2b644c53b67ae0cabcb85dbcc8a3352d56e8d1eb Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 11:41:22 +0100 Subject: [PATCH 16/28] tests: add integration test for CleanupManager atexit via typer.Exit(0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parametrized subprocess test covering the production exit path with skip_if_debug in both debug and non-debug modes — the atexit handler can't be exercised from in-process unit tests. --- tests/integration/test_cleanup_atexit.py | 69 ++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 tests/integration/test_cleanup_atexit.py diff --git a/tests/integration/test_cleanup_atexit.py b/tests/integration/test_cleanup_atexit.py new file mode 100644 index 00000000..2ba56f17 --- /dev/null +++ b/tests/integration/test_cleanup_atexit.py @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2025 Daniel Schmidt + +"""Integration tests for CleanupManager atexit behaviour. + +Spawns isolated subprocesses so that atexit fires in a real interpreter +exit, not a mock or in-process call. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +pytestmark = [ + pytest.mark.integration, + pytest.mark.windows, +] + +_SCRIPT = """ +import os +import typer +from pathlib import Path +from nac_test.utils.cleanup import get_cleanup_manager + +app = typer.Typer() + +@app.command() +def main() -> None: + get_cleanup_manager().register(Path({sentinel!r}), skip_if_debug=True) + raise typer.Exit(0) + +app() +""" + + +@pytest.mark.parametrize( + ("debug_env", "expect_exists"), + [ + (None, False), # NAC_TEST_DEBUG unset → file is deleted + ("true", True), # NAC_TEST_DEBUG=true → file is kept + ], +) +def test_cleanup_atexit_via_typer_exit( + tmp_path: Path, debug_env: str | None, expect_exists: bool +) -> None: + """CleanupManager deletes (or retains) registered files on typer.Exit(0). + + Exercises the exact production exit path and the skip_if_debug flag. + """ + sentinel = tmp_path / "job.py" + sentinel.write_text("# temp job") + + env = None + if debug_env is not None: + env = os.environ.copy() + env["NAC_TEST_DEBUG"] = debug_env + + result = subprocess.run( + [sys.executable, "-c", _SCRIPT.format(sentinel=str(sentinel))], + capture_output=True, + text=True, + env=env, + ) + + assert result.returncode == 0, result.stderr + assert sentinel.exists() is expect_exists From dd5c8ff0ce19f1d07c0aac85311f081fa1747cc7 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 11:48:00 +0100 Subject: [PATCH 17/28] refactor: rename skip_if_debug to keep_if_debug in CleanupManager The old name required parsing "skip [deletion] if debug" to understand the intent. keep_if_debug=True reads directly as what it does. --- nac_test/cli/main.py | 2 +- .../execution/device/device_executor.py | 4 +-- nac_test/utils/cleanup.py | 16 ++++++------ tests/integration/test_cleanup_atexit.py | 4 +-- tests/unit/utils/test_cleanup.py | 26 +++++++++---------- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 014bf3b2..1a17fcce 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -388,7 +388,7 @@ def main( # Register merged data file for cleanup on exit/signal (SIGTERM/SIGINT) cleanup_manager = get_cleanup_manager() - cleanup_manager.register(merged_data_path, skip_if_debug=True) + cleanup_manager.register(merged_data_path, keep_if_debug=True) duration = (datetime.now() - start_time).total_seconds() typer.echo(f"✅ Data model merging completed ({format_duration(duration)})") diff --git a/nac_test/pyats_core/execution/device/device_executor.py b/nac_test/pyats_core/execution/device/device_executor.py index c2c30825..d4d5433b 100644 --- a/nac_test/pyats_core/execution/device/device_executor.py +++ b/nac_test/pyats_core/execution/device/device_executor.py @@ -103,8 +103,8 @@ async def run_device_job_with_semaphore( # Register temp files for deletion on exit (kept when NAC_TEST_DEBUG is set). cleanup = get_cleanup_manager() - cleanup.register(job_file_path, skip_if_debug=True) - cleanup.register(testbed_file_path, skip_if_debug=True) + cleanup.register(job_file_path, keep_if_debug=True) + cleanup.register(testbed_file_path, keep_if_debug=True) # Set up environment for this device # Always start with a copy of os.environ to preserve PATH and other variables diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index e9141d2e..42dd5c71 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -36,7 +36,7 @@ class CleanupManager: # File will be automatically removed on exit # Skip deletion when NAC_TEST_DEBUG is set (useful for debugging): - cleanup_manager.register(Path("/tmp/temp_job.py"), skip_if_debug=True) + cleanup_manager.register(Path("/tmp/temp_job.py"), keep_if_debug=True) Thread Safety: All operations are thread-safe via internal locking. @@ -59,7 +59,7 @@ def __init__(self) -> None: if self._initialized: return - self._files: dict[Path, bool] = {} # path → skip_if_debug + self._files: dict[Path, bool] = {} # path → keep_if_debug self._lock = threading.Lock() self._original_sigterm: Any = None self._original_sigint: Any = None @@ -109,21 +109,21 @@ def _signal_handler(self, signum: int, frame: FrameType | None) -> None: # For SIGTERM, re-send the signal signal.raise_signal(signum) - def register(self, path: Path, skip_if_debug: bool = False) -> None: + def register(self, path: Path, keep_if_debug: bool = False) -> None: """Register a file path for cleanup on exit. Args: path: Path to file that should be removed on exit. Directories are not supported (use shutil.rmtree directly). - skip_if_debug: If True, the file will not be deleted when + keep_if_debug: If True, the file will not be deleted when NAC_TEST_DEBUG is set — useful for intermediate files (job scripts, testbed YAMLs, merged data) that aid debugging. """ with self._lock: resolved = path.resolve() - self._files[resolved] = skip_if_debug + self._files[resolved] = keep_if_debug logger.debug( - f"Registered for cleanup: {resolved} (skip_if_debug={skip_if_debug})" + f"Registered for cleanup: {resolved} (keep_if_debug={keep_if_debug})" ) def unregister(self, path: Path) -> None: @@ -145,8 +145,8 @@ def _cleanup(self) -> None: self._cleanup_done = True debug_kept: list[Path] = [] - for path, skip_if_debug in self._files.items(): - if skip_if_debug and DEBUG_MODE: + for path, keep_if_debug in self._files.items(): + if keep_if_debug and DEBUG_MODE: debug_kept.append(path) continue try: diff --git a/tests/integration/test_cleanup_atexit.py b/tests/integration/test_cleanup_atexit.py index 2ba56f17..3f6d4962 100644 --- a/tests/integration/test_cleanup_atexit.py +++ b/tests/integration/test_cleanup_atexit.py @@ -29,7 +29,7 @@ @app.command() def main() -> None: - get_cleanup_manager().register(Path({sentinel!r}), skip_if_debug=True) + get_cleanup_manager().register(Path("{sentinel}"), keep_if_debug=True) raise typer.Exit(0) app() @@ -48,7 +48,7 @@ def test_cleanup_atexit_via_typer_exit( ) -> None: """CleanupManager deletes (or retains) registered files on typer.Exit(0). - Exercises the exact production exit path and the skip_if_debug flag. + Exercises the exact production exit path and the keep_if_debug flag. """ sentinel = tmp_path / "job.py" sentinel.write_text("# temp job") diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index 3f239459..54ded4e5 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -63,7 +63,7 @@ def test_register_adds_path( def test_register_default_flag_is_false( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: - """register() without skip_if_debug stores False (always delete).""" + """register() without keep_if_debug stores False (always delete).""" test_file = tmp_path / "test.txt" test_file.touch() @@ -71,14 +71,14 @@ def test_register_default_flag_is_false( assert fresh_cleanup_manager._files[test_file.resolve()] is False - def test_register_skip_if_debug_stores_true( + def test_register_keep_if_debug_stores_true( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: - """register(skip_if_debug=True) stores True for the path.""" + """register(keep_if_debug=True) stores True for the path.""" test_file = tmp_path / "test.txt" test_file.touch() - fresh_cleanup_manager.register(test_file, skip_if_debug=True) + fresh_cleanup_manager.register(test_file, keep_if_debug=True) assert fresh_cleanup_manager._files[test_file.resolve()] is True @@ -119,14 +119,14 @@ def test_unregister_removes_path( assert test_file.resolve() not in fresh_cleanup_manager._files - def test_unregister_removes_skip_if_debug_path( + def test_unregister_removes_keep_if_debug_path( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: - """unregister() removes paths registered with skip_if_debug=True.""" + """unregister() removes paths registered with keep_if_debug=True.""" test_file = tmp_path / "test.txt" test_file.touch() - fresh_cleanup_manager.register(test_file, skip_if_debug=True) + fresh_cleanup_manager.register(test_file, keep_if_debug=True) fresh_cleanup_manager.unregister(test_file) assert test_file.resolve() not in fresh_cleanup_manager._files @@ -199,7 +199,7 @@ def test_cleanup_continues_after_single_file_error( class TestCleanupManagerSkipIfDebug: - """Tests for skip_if_debug behaviour during cleanup.""" + """Tests for keep_if_debug behaviour during cleanup.""" @pytest.mark.parametrize( ("debug_mode", "expected_exists"), @@ -209,18 +209,18 @@ class TestCleanupManagerSkipIfDebug: ], ids=["debug_off_deletes", "debug_on_keeps"], ) - def test_skip_if_debug_respects_debug_mode( + def test_keep_if_debug_respects_debug_mode( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path, debug_mode: bool, expected_exists: bool, ) -> None: - """Files registered with skip_if_debug=True are kept iff NAC_TEST_DEBUG is set.""" + """Files registered with keep_if_debug=True are kept iff NAC_TEST_DEBUG is set.""" test_file = tmp_path / "job.py" test_file.touch() - fresh_cleanup_manager.register(test_file, skip_if_debug=True) + fresh_cleanup_manager.register(test_file, keep_if_debug=True) with patch("nac_test.utils.cleanup.DEBUG_MODE", debug_mode): fresh_cleanup_manager.cleanup_now() @@ -230,7 +230,7 @@ def test_skip_if_debug_respects_debug_mode( def test_normal_files_always_deleted_regardless_of_debug( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: - """Files registered without skip_if_debug are always deleted, even in debug mode.""" + """Files registered without keep_if_debug are always deleted, even in debug mode.""" test_file = tmp_path / "sensitive.yaml" test_file.touch() @@ -251,7 +251,7 @@ def test_mixed_registration_in_debug_mode( debug_file.touch() fresh_cleanup_manager.register(sensitive) - fresh_cleanup_manager.register(debug_file, skip_if_debug=True) + fresh_cleanup_manager.register(debug_file, keep_if_debug=True) with patch("nac_test.utils.cleanup.DEBUG_MODE", True): fresh_cleanup_manager.cleanup_now() From 6a521313b3f3a2993382252f2831d8c56be61051 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 13:14:40 +0100 Subject: [PATCH 18/28] tests: add subprocess-level signal coverage for CleanupManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fill the contract gap where unit tests call _signal_handler() directly, bypassing the _install_signal_handlers() registration path. - Add test_cleanup_on_signal parametrized over SIGTERM and SIGINT: spawns a subprocess, waits for a "ready" handshake, delivers the signal, and asserts the sentinel file was deleted - Add timeout=30 to the existing subprocess.run() call - Rename _SCRIPT → _ATEXIT_SCRIPT, add _SIGNAL_SCRIPT at module level - Move @pytest.mark.windows to the atexit test only; signal test runs Linux-only by CI convention --- tests/integration/test_cleanup_atexit.py | 81 +++++++++++++++++++++--- 1 file changed, 73 insertions(+), 8 deletions(-) diff --git a/tests/integration/test_cleanup_atexit.py b/tests/integration/test_cleanup_atexit.py index 3f6d4962..35c28b78 100644 --- a/tests/integration/test_cleanup_atexit.py +++ b/tests/integration/test_cleanup_atexit.py @@ -1,25 +1,24 @@ # SPDX-License-Identifier: MPL-2.0 # Copyright (c) 2025 Daniel Schmidt -"""Integration tests for CleanupManager atexit behaviour. +"""Integration tests for CleanupManager atexit and signal behaviour. Spawns isolated subprocesses so that atexit fires in a real interpreter -exit, not a mock or in-process call. +exit, not a mock or in-process call, and so that signals are actually +delivered to the process rather than simulated via direct method calls. """ import os +import signal import subprocess import sys from pathlib import Path import pytest -pytestmark = [ - pytest.mark.integration, - pytest.mark.windows, -] +pytestmark = pytest.mark.integration -_SCRIPT = """ +_ATEXIT_SCRIPT = """ import os import typer from pathlib import Path @@ -35,7 +34,22 @@ def main() -> None: app() """ +_SIGNAL_SCRIPT = """ +import sys +import time +from pathlib import Path +from nac_test.utils.cleanup import get_cleanup_manager +get_cleanup_manager().register(Path("{sentinel}")) + +sys.stdout.write("ready\\n") +sys.stdout.flush() + +time.sleep(30) +""" + + +@pytest.mark.windows @pytest.mark.parametrize( ("debug_env", "expect_exists"), [ @@ -59,11 +73,62 @@ def test_cleanup_atexit_via_typer_exit( env["NAC_TEST_DEBUG"] = debug_env result = subprocess.run( - [sys.executable, "-c", _SCRIPT.format(sentinel=str(sentinel))], + [sys.executable, "-c", _ATEXIT_SCRIPT.format(sentinel=str(sentinel))], capture_output=True, text=True, env=env, + timeout=30, ) assert result.returncode == 0, result.stderr assert sentinel.exists() is expect_exists + + +@pytest.mark.parametrize( + "signum", + [signal.SIGTERM, signal.SIGINT], + ids=["SIGTERM", "SIGINT"], +) +def test_cleanup_on_signal(tmp_path: Path, signum: signal.Signals) -> None: + """CleanupManager deletes registered files when the process receives SIGTERM or SIGINT. + + Delivers an actual signal to a child process (not a direct _signal_handler() + call) so that the signal registration path in _install_signal_handlers() is + exercised end-to-end. SIGTERM and SIGINT have distinct re-raise paths: + SIGTERM re-sends the signal via signal.raise_signal(); SIGINT raises + KeyboardInterrupt. Both must still delete registered files before exiting. + """ + sentinel = tmp_path / "sensitive.yaml" + sentinel.write_text("secret: value") + + proc = subprocess.Popen( + [sys.executable, "-c", _SIGNAL_SCRIPT.format(sentinel=str(sentinel))], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + # Wait until the child signals it is ready (handlers installed, file registered). + try: + ready_line = proc.stdout.readline() # type: ignore[union-attr] + assert ready_line.strip() == "ready", f"Unexpected output: {ready_line!r}" + except Exception as e: + proc.kill() + proc.wait() + raise AssertionError(f"Child process failed to signal readiness: {e}") from e + + proc.send_signal(signum) + + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired as e: + proc.kill() + proc.wait() + raise AssertionError( + f"Child process did not exit within 10 s after {signum.name}" + ) from e + + assert not sentinel.exists(), ( + f"{sentinel.name} was not deleted after {signum.name} — " + "CleanupManager signal handler may not be registered correctly" + ) From 9cdf355e10b495e6322491a5416abece8cbd78c2 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 13:30:10 +0100 Subject: [PATCH 19/28] remote trivial changes from new test_robot_writer.py --- tests/unit/robot/test_robot_writer.py | 49 --------------------------- 1 file changed, 49 deletions(-) diff --git a/tests/unit/robot/test_robot_writer.py b/tests/unit/robot/test_robot_writer.py index 45e7ae78..528071df 100644 --- a/tests/unit/robot/test_robot_writer.py +++ b/tests/unit/robot/test_robot_writer.py @@ -6,7 +6,6 @@ """Unit tests for RobotWriter. Covers: -- Constructor: merged_data dict is stored directly (no file I/O, no conversion) - render_template: uses self.data as template context by default - render_template: custom_data overrides self.data when provided - render_template: output file and parent directories are created @@ -49,54 +48,6 @@ def writer() -> RobotWriter: ) -# --------------------------------------------------------------------------- -# Constructor tests -# --------------------------------------------------------------------------- - - -class TestRobotWriterInit: - """Tests for RobotWriter.__init__().""" - - def test_stores_merged_data_directly(self) -> None: - """Constructor stores the dict by reference — no copy or type conversion.""" - data = {"host": "sw1", "count": 5} - w = RobotWriter(merged_data=data, filters_path=None, tests_path=None) - assert w.data is data - - def test_accepts_empty_dict(self) -> None: - """Constructor accepts an empty dict without raising.""" - w = RobotWriter(merged_data={}, filters_path=None, tests_path=None) - assert w.data == {} - - def test_tags_default_to_empty_lists(self) -> None: - """include_tags and exclude_tags default to empty lists when omitted.""" - w = RobotWriter(merged_data={}, filters_path=None, tests_path=None) - assert w.include_tags == [] - assert w.exclude_tags == [] - - @pytest.mark.parametrize( - ("include_tags", "exclude_tags"), - [ - (["smoke"], []), - ([], ["slow"]), - (["smoke"], ["slow"]), - (["smoke", "regression"], ["slow", "wip"]), - ], - ids=["include_only", "exclude_only", "both", "multiple_each"], - ) - def test_tags_stored_correctly(self, include_tags, exclude_tags) -> None: - """Explicit include/exclude tags are stored without modification.""" - w = RobotWriter( - merged_data={}, - filters_path=None, - tests_path=None, - include_tags=include_tags, - exclude_tags=exclude_tags, - ) - assert w.include_tags == include_tags - assert w.exclude_tags == exclude_tags - - # --------------------------------------------------------------------------- # render_template tests # --------------------------------------------------------------------------- From e931ada80fe39c097d3a4973b69c4ae6d98f0a86 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 13:30:59 +0100 Subject: [PATCH 20/28] fix: guard bad directive syntax, fix stale docstring, use LOG_HTML constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - robot_writer.py: raise ValueError with file/directive context when iterate_list_chunked chunk_size cannot be parsed — propagates through the existing except Exception handler in combined_orchestrator, aborting execution cleanly instead of silently continuing with a bad directive - robot/orchestrator.py: remove stale step "3. Creates merged data model file" from run_tests() docstring; renumber remaining steps 4-6 → 3-5 - combined_orchestrator.py: replace bare "log.html" with LOG_HTML constant --- nac_test/combined_orchestrator.py | 2 +- nac_test/robot/orchestrator.py | 7 +++---- nac_test/robot/robot_writer.py | 8 +++++++- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/nac_test/combined_orchestrator.py b/nac_test/combined_orchestrator.py index d7cdf339..80aaf581 100644 --- a/nac_test/combined_orchestrator.py +++ b/nac_test/combined_orchestrator.py @@ -406,7 +406,7 @@ def _print_execution_summary( if combined_dashboard.exists(): typer.echo(f"Dashboard: {combined_dashboard.resolve()}") if results.robot is not None and not results.robot.is_empty: - robot_log = self.output_dir / ROBOT_RESULTS_DIRNAME / "log.html" + robot_log = self.output_dir / ROBOT_RESULTS_DIRNAME / LOG_HTML if robot_log.exists(): typer.echo(f"Robot: {robot_log.resolve()}") if results.api is not None and not results.api.is_empty: diff --git a/nac_test/robot/orchestrator.py b/nac_test/robot/orchestrator.py index ad168291..b16ec1fc 100644 --- a/nac_test/robot/orchestrator.py +++ b/nac_test/robot/orchestrator.py @@ -118,10 +118,9 @@ def run_tests(self) -> TestResults: This method: 1. Creates the Robot Framework working directory under robot_results/ 2. Renders Robot test templates using RobotWriter - 3. Creates merged data model file in the Robot working directory - 4. Executes tests using pabot (unless render_only mode) - 5. Creates backward compatibility symlinks - 6. Extracts and returns test statistics + 3. Executes tests using pabot (unless render_only mode) + 4. Creates backward compatibility symlinks + 5. Extracts and returns test statistics Follows the same pattern as PyATSOrchestrator.run_tests(). diff --git a/nac_test/robot/robot_writer.py b/nac_test/robot/robot_writer.py index 45c4c72d..c6ae4ae1 100644 --- a/nac_test/robot/robot_writer.py +++ b/nac_test/robot/robot_writer.py @@ -355,7 +355,13 @@ def write( if params[1] == "iterate_list_chunked": # Handle chunked iteration object_path = params[5] - chunk_size = int(params[6].rstrip("#}")) + try: + chunk_size = int(params[6].rstrip("#}")) + except (ValueError, IndexError) as e: + raise ValueError( + f"Invalid chunk_size in iterate_list_chunked directive " + f"in {Path(dir, filename)}: {match.group()!r}" + ) from e for item in elem: attr_value = item.get(attr) if attr_value is None: From 740fe99fd6a51bf2f9bbbedda6a073758d31bb98 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 13:40:09 +0100 Subject: [PATCH 21/28] add PRD, adjust comment --- dev-docs/PRD_AND_ARCHITECTURE.md | 51 ++++++++++++++++++++++++++++---- nac_test/robot/orchestrator.py | 4 +-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/dev-docs/PRD_AND_ARCHITECTURE.md b/dev-docs/PRD_AND_ARCHITECTURE.md index f0b9a6f1..6ac890aa 100644 --- a/dev-docs/PRD_AND_ARCHITECTURE.md +++ b/dev-docs/PRD_AND_ARCHITECTURE.md @@ -6058,16 +6058,57 @@ class SystemResourceCalculator: ### Cleanup Utilities (`utils/cleanup.py`) -Resource cleanup on exit: +#### CleanupManager + +`CleanupManager` is a process-wide singleton that ensures registered files +are deleted when the process exits, regardless of how it exits. + +**Triggers covered:** +- Normal exit via `atexit` (including `typer.Exit`) +- `SIGTERM` (e.g. `docker stop`, `kill`) — Unix only +- `SIGINT` (Ctrl+C / `KeyboardInterrupt`) — Unix only + +`SIGKILL` cannot be intercepted; files may remain if the process is killed +with `-9`. + +**Key methods:** ```python -def cleanup_temp_files(patterns: List[str]) -> None: - """Remove temporary files matching patterns.""" +cleanup = get_cleanup_manager() # always returns the singleton -def cleanup_output_directory(output_dir: Path) -> None: - """Clean up output directory before run.""" +cleanup.register(path) # delete path on exit +cleanup.register(path, keep_if_debug=True) # skip deletion when NAC_TEST_DEBUG=true +cleanup.unregister(path) # cancel a previously registered path +cleanup.cleanup_now() # trigger cleanup immediately (idempotent) ``` +**`keep_if_debug` flag** + +Files registered with `keep_if_debug=True` are retained when +`NAC_TEST_DEBUG=true` is set. Use this for intermediate files that are +useful for debugging (job scripts, per-device testbed YAMLs, merged data +model). Sensitive files (e.g. files containing credentials) should always +be registered without this flag so they are unconditionally removed. + +**Thread safety:** all operations are protected by an internal lock. + +**Signal handler behaviour:** on SIGTERM the original handler is restored +and the signal is re-raised via `signal.raise_signal()` so upstream +process managers (Docker, systemd) see the expected exit status. On SIGINT +the original handler is restored and `KeyboardInterrupt` is raised so the +normal Python exception handling chain proceeds. + +#### PyATS runtime cleanup helpers + +Three standalone functions handle CI/CD-specific directory hygiene (not +related to `CleanupManager`): + +| Function | Purpose | +|---|---| +| `cleanup_pyats_runtime(workspace_path)` | Removes the `.pyats/` runtime directory before a run to prevent disk exhaustion | +| `cleanup_old_test_outputs(output_dir, days)` | Removes `.jsonl` result files older than `days` days | +| `cleanup_stale_test_artifacts(output_dir)` | Removes `api/`, `d2d/`, and `default/` subdirectories containing stale JSONL files from interrupted runs | + ### Environment Utilities (`utils/environment.py`) Environment variable handling: diff --git a/nac_test/robot/orchestrator.py b/nac_test/robot/orchestrator.py index b16ec1fc..47fb1365 100644 --- a/nac_test/robot/orchestrator.py +++ b/nac_test/robot/orchestrator.py @@ -136,13 +136,13 @@ def run_tests(self) -> TestResults: logger.info(f"Robot working directory: {self.output_dir}") logger.info(f"Templates directory: {self.templates_dir}") - # Phase 1: Template rendering (delegate to existing RobotWriter) + # Phase 2: Template rendering (delegate to existing RobotWriter) typer.echo("📝 Rendering Robot Framework templates...") self.robot_writer.write( self.templates_dir, self.output_dir, ordering_file=self.ordering_file ) - # Phase 2: Test execution (unless render-only mode) + # Phase 3: Test execution (unless render-only mode) if not self.render_only: typer.echo("🤖 Executing Robot Framework tests...\n\n") default_robot_loglevel = ( From a9bfb7f0ec18ea75e973d0ea72e516b8fcc0a569 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 13:52:32 +0100 Subject: [PATCH 22/28] minor adjustments in cleanup.py --- nac_test/utils/cleanup.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index 42dd5c71..cd81a688 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -7,14 +7,13 @@ import logging import shutil import signal -import sys import threading import time from pathlib import Path from types import FrameType from typing import Any -from nac_test.core.constants import DEBUG_MODE +from nac_test.core.constants import DEBUG_MODE, IS_WINDOWS from nac_test.pyats_core.discovery.test_type_resolver import VALID_TEST_TYPES logger = logging.getLogger(__name__) @@ -69,7 +68,7 @@ def __init__(self) -> None: atexit.register(self._cleanup) # Install signal handlers (Unix only - Windows doesn't support SIGTERM properly) - if sys.platform != "win32": + if not IS_WINDOWS: self._install_signal_handlers() self._initialized = True @@ -120,6 +119,8 @@ def register(self, path: Path, keep_if_debug: bool = False) -> None: (job scripts, testbed YAMLs, merged data) that aid debugging. """ with self._lock: + # resolve() canonicalises symlinks (e.g. macOS /tmp → /private/tmp) + # so registration and cleanup always refer to the same inode. resolved = path.resolve() self._files[resolved] = keep_if_debug logger.debug( From 03c7b9490f250bb1b2ddfd0db5145df7137e42a3 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 16:40:59 +0100 Subject: [PATCH 23/28] tests: restore integration rendering tests and fix fixture placement --- .../fixtures/data_merge/defaults.yaml | 0 .../fixtures/data_merge/file1.yaml | 0 .../fixtures/data_merge/file2.yaml | 0 .../fixtures/data_merge/result.yaml | 0 tests/integration/test_cli_rendering.py | 37 ++++++++++ tests/unit/test_data_merger.py | 71 +------------------ 6 files changed, 38 insertions(+), 70 deletions(-) rename tests/{unit => integration}/fixtures/data_merge/defaults.yaml (100%) rename tests/{unit => integration}/fixtures/data_merge/file1.yaml (100%) rename tests/{unit => integration}/fixtures/data_merge/file2.yaml (100%) rename tests/{unit => integration}/fixtures/data_merge/result.yaml (100%) diff --git a/tests/unit/fixtures/data_merge/defaults.yaml b/tests/integration/fixtures/data_merge/defaults.yaml similarity index 100% rename from tests/unit/fixtures/data_merge/defaults.yaml rename to tests/integration/fixtures/data_merge/defaults.yaml diff --git a/tests/unit/fixtures/data_merge/file1.yaml b/tests/integration/fixtures/data_merge/file1.yaml similarity index 100% rename from tests/unit/fixtures/data_merge/file1.yaml rename to tests/integration/fixtures/data_merge/file1.yaml diff --git a/tests/unit/fixtures/data_merge/file2.yaml b/tests/integration/fixtures/data_merge/file2.yaml similarity index 100% rename from tests/unit/fixtures/data_merge/file2.yaml rename to tests/integration/fixtures/data_merge/file2.yaml diff --git a/tests/unit/fixtures/data_merge/result.yaml b/tests/integration/fixtures/data_merge/result.yaml similarity index 100% rename from tests/unit/fixtures/data_merge/result.yaml rename to tests/integration/fixtures/data_merge/result.yaml diff --git a/tests/integration/test_cli_rendering.py b/tests/integration/test_cli_rendering.py index c0761115..56f5cfa9 100644 --- a/tests/integration/test_cli_rendering.py +++ b/tests/integration/test_cli_rendering.py @@ -8,6 +8,7 @@ chunked rendering, and merged data model output. """ +import filecmp from pathlib import Path import pytest @@ -262,6 +263,42 @@ def test_chunked_list_rendering_produces_expected_content(tmp_path: Path) -> Non ) +def test_merged_data_model_creates_default_filename(tmp_path: Path) -> None: + """Test that the merged data model is written with the expected filename and content.""" + runner = CliRunner() + templates_path = "tests/integration/fixtures/templates/" + expected_filename = "merged_data_model_test_variables.yaml" + output_model_path = tmp_path / expected_filename + data_dir = Path("tests/integration/fixtures/data_merge") + expected_model_path = data_dir / "result.yaml" + + result = runner.invoke( + nac_test.cli.main.app, + [ + "-d", + str(data_dir / "file1.yaml"), + "-d", + str(data_dir / "file2.yaml"), + "-t", + templates_path, + "-o", + str(tmp_path), + "--render-only", + ], + ) + assert result.exit_code == 0, ( + f"Merged data model creation should succeed, got exit code " + f"{result.exit_code}: {result.output}" + ) + assert output_model_path.exists(), ( + f"Merged data model file should exist at {output_model_path}" + ) + assert filecmp.cmp(output_model_path, expected_model_path, shallow=False), ( + f"Merged data model content should match expected content from " + f"{expected_model_path}" + ) + + def test_render_only_without_controller_credentials(tmp_path: Path) -> None: """Render-only mode works without controller environment variables. All other tests in this module implicitly also test this, but this diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index 722b7254..5d2d32ed 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -4,7 +4,7 @@ """Unit tests for DataMerger. Covers: -- merge_data_files: content correctness, empty input, single file +- merge_data_files: empty input edge case - write_merged_data_model: output filename, YAML roundtrip """ @@ -14,67 +14,10 @@ from nac_test.data_merger import DataMerger -FIXTURES_DIR = Path("tests/unit/fixtures/data_merge") - class TestMergeDataFiles: """Tests for DataMerger.merge_data_files().""" - def test_merge_produces_correct_scalar_values(self) -> None: - """Scalar attributes from both files are present in the merged result.""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - assert result["root"]["attr1"] == "value1" - assert result["root"]["attr2"] == "value2" - - def test_merge_concatenates_primitive_lists(self) -> None: - """Primitive lists are concatenated (not de-duplicated) across files.""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - assert result["root"]["primitive_list"] == ["item1", "item1", "item1"] - - def test_merge_merges_dict_lists_by_name_key(self) -> None: - """Dict-list entries with the same name key are merged (not duplicated).""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - dict_list = result["root"]["dict_list"] - assert len(dict_list) == 1 - assert dict_list[0]["name"] == "abc" - assert dict_list[0]["extra"] == "def" - - def test_merge_combines_extra_fields_across_files(self) -> None: - """Extra fields from both files are merged into the same named entry.""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - entry = result["root"]["dict_list_extra"][0] - assert entry["name"] == "abc" - assert entry["extra1"] == "def" - assert entry["extra2"] == "ghi" - - def test_merge_preserves_nested_defaults(self) -> None: - """Nested scalar values present only in the first file are preserved.""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - assert result["defaults"]["apic"]["version"] == 6.0 - - def test_merge_returns_dict(self) -> None: - """Return type is always a dict.""" - result = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - assert isinstance(result, dict) - - def test_merge_single_file_returns_its_content(self) -> None: - """A single-file list returns the content of that file unchanged.""" - result = DataMerger.merge_data_files([FIXTURES_DIR / "file1.yaml"]) - assert result["root"]["attr1"] == "value1" - assert "attr2" not in result.get("root", {}) - def test_merge_empty_list_returns_empty_dict(self) -> None: """An empty path list returns an empty dict rather than raising.""" result = DataMerger.merge_data_files([]) @@ -103,15 +46,3 @@ def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: assert reloaded["host"] == "router1" assert reloaded["vlan"] == 100 assert list(reloaded["tags"]) == ["a", "b"] - - def test_roundtrip_preserves_merged_fixture_content(self, tmp_path: Path) -> None: - """Full merge result can be written and reloaded without data loss.""" - merged = DataMerger.merge_data_files( - [FIXTURES_DIR / "file1.yaml", FIXTURES_DIR / "file2.yaml"] - ) - output_path = DataMerger.write_merged_data_model(merged, tmp_path) - reloaded = yaml.load_yaml_files([output_path]) - assert reloaded["root"]["attr1"] == "value1" - assert reloaded["root"]["attr2"] == "value2" - assert reloaded["root"]["primitive_list"] == ["item1", "item1", "item1"] - assert reloaded["root"]["dict_list"][0]["extra"] == "def" From c0a8442f61497f9b2ba8c36862f32c97a53ff180 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 17:14:28 +0100 Subject: [PATCH 24/28] refactor: replace _cleanup/cleanup_now with single public run_cleanup() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the private _cleanup() method and the cleanup_now() wrapper — the split served no purpose since both had identical behaviour and there was no reason to bypass the public API in atexit.register() or _signal_handler(). Inline the implementation directly into run_cleanup(), which is now the single method registered with atexit, called from the signal handler, and available for manual invocation. --- nac_test/utils/cleanup.py | 22 ++++++++++------------ tests/unit/utils/test_cleanup.py | 31 +++++++++++-------------------- 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index cd81a688..18de67ce 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -65,7 +65,7 @@ def __init__(self) -> None: self._cleanup_done = False # Register atexit handler - atexit.register(self._cleanup) + atexit.register(self.run_cleanup) # Install signal handlers (Unix only - Windows doesn't support SIGTERM properly) if not IS_WINDOWS: @@ -90,7 +90,7 @@ def _signal_handler(self, signum: int, frame: FrameType | None) -> None: logger.debug(f"Received {sig_name}, performing cleanup") # Perform cleanup - self._cleanup() + self.run_cleanup() # Re-raise the signal with original handler or default behavior if signum == signal.SIGTERM: @@ -138,8 +138,14 @@ def unregister(self, path: Path) -> None: self._files.pop(resolved, None) logger.debug(f"Unregistered from cleanup: {resolved}") - def _cleanup(self) -> None: - """Remove all registered files. Safe to call multiple times.""" + def run_cleanup(self) -> None: + """Delete all registered files. Safe to call multiple times. + + Called automatically on normal exit (atexit), SIGTERM, and SIGINT. + Can also be called manually before an explicit exit. + After the first call, registered files are cleared and subsequent + calls are no-ops. + """ with self._lock: if self._cleanup_done: return @@ -166,14 +172,6 @@ def _cleanup(self) -> None: self._files.clear() - def cleanup_now(self) -> None: - """Manually trigger cleanup (e.g., before explicit exit). - - After calling this, registered files are cleared and won't be - cleaned up again on exit. - """ - self._cleanup() - def get_cleanup_manager() -> CleanupManager: """Get the singleton CleanupManager instance. diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index 54ded4e5..ddd46199 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -50,25 +50,16 @@ def test_get_cleanup_manager_returns_singleton( class TestCleanupManagerRegistration: """Tests for file registration and unregistration.""" - def test_register_adds_path( + def test_register_adds_path_with_default_flag_false( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: + """register() without keep_if_debug adds the path and stores False (always delete).""" test_file = tmp_path / "test.txt" test_file.touch() fresh_cleanup_manager.register(test_file) assert test_file.resolve() in fresh_cleanup_manager._files - - def test_register_default_flag_is_false( - self, fresh_cleanup_manager: CleanupManager, tmp_path: Path - ) -> None: - """register() without keep_if_debug stores False (always delete).""" - test_file = tmp_path / "test.txt" - test_file.touch() - - fresh_cleanup_manager.register(test_file) - assert fresh_cleanup_manager._files[test_file.resolve()] is False def test_register_keep_if_debug_stores_true( @@ -104,7 +95,7 @@ def test_unregister_prevents_deletion( fresh_cleanup_manager.register(test_file) fresh_cleanup_manager.unregister(test_file) - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() assert test_file.exists() @@ -148,7 +139,7 @@ def test_cleanup_removes_registered_files( test_file.write_text("secret: password123") fresh_cleanup_manager.register(test_file) - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() assert not test_file.exists() @@ -159,8 +150,8 @@ def test_cleanup_is_idempotent( test_file.touch() fresh_cleanup_manager.register(test_file) - fresh_cleanup_manager.cleanup_now() - fresh_cleanup_manager.cleanup_now() # Second call should be safe + fresh_cleanup_manager.run_cleanup() + fresh_cleanup_manager.run_cleanup() # Second call should be safe assert fresh_cleanup_manager._cleanup_done @@ -173,7 +164,7 @@ def test_cleanup_handles_already_deleted_file( fresh_cleanup_manager.register(test_file) test_file.unlink() # Delete before cleanup - fresh_cleanup_manager.cleanup_now() # Should not raise + fresh_cleanup_manager.run_cleanup() # Should not raise def test_cleanup_continues_after_single_file_error( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path @@ -190,7 +181,7 @@ def test_cleanup_continues_after_single_file_error( file1.unlink() file1.mkdir() - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() # file2 should still be cleaned up despite file1 error assert not file2.exists() @@ -223,7 +214,7 @@ def test_keep_if_debug_respects_debug_mode( fresh_cleanup_manager.register(test_file, keep_if_debug=True) with patch("nac_test.utils.cleanup.DEBUG_MODE", debug_mode): - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() assert test_file.exists() is expected_exists @@ -237,7 +228,7 @@ def test_normal_files_always_deleted_regardless_of_debug( fresh_cleanup_manager.register(test_file) with patch("nac_test.utils.cleanup.DEBUG_MODE", True): - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() assert not test_file.exists() @@ -254,7 +245,7 @@ def test_mixed_registration_in_debug_mode( fresh_cleanup_manager.register(debug_file, keep_if_debug=True) with patch("nac_test.utils.cleanup.DEBUG_MODE", True): - fresh_cleanup_manager.cleanup_now() + fresh_cleanup_manager.run_cleanup() assert not sensitive.exists() assert debug_file.exists() From 7156e722e80e9250c8ff8baf5da2b7b73ce59832 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 17:15:05 +0100 Subject: [PATCH 25/28] reference MERGED_DATA_FILENAME in tests --- tests/integration/test_cli_rendering.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_cli_rendering.py b/tests/integration/test_cli_rendering.py index 56f5cfa9..4db036a3 100644 --- a/tests/integration/test_cli_rendering.py +++ b/tests/integration/test_cli_rendering.py @@ -16,7 +16,11 @@ from typer.testing import CliRunner import nac_test.cli.main -from nac_test.core.constants import EXIT_ERROR, ROBOT_RESULTS_DIRNAME +from nac_test.core.constants import ( + EXIT_ERROR, + MERGED_DATA_FILENAME, + ROBOT_RESULTS_DIRNAME, +) pytestmark = [pytest.mark.integration, pytest.mark.windows] @@ -267,8 +271,7 @@ def test_merged_data_model_creates_default_filename(tmp_path: Path) -> None: """Test that the merged data model is written with the expected filename and content.""" runner = CliRunner() templates_path = "tests/integration/fixtures/templates/" - expected_filename = "merged_data_model_test_variables.yaml" - output_model_path = tmp_path / expected_filename + output_model_path = tmp_path / MERGED_DATA_FILENAME data_dir = Path("tests/integration/fixtures/data_merge") expected_model_path = data_dir / "result.yaml" From 2c19d627d5138beab601a1b1850047177c6b85e1 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 17:26:19 +0100 Subject: [PATCH 26/28] add comment on fork safety --- nac_test/utils/cleanup.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/nac_test/utils/cleanup.py b/nac_test/utils/cleanup.py index 18de67ce..eafb3d02 100644 --- a/nac_test/utils/cleanup.py +++ b/nac_test/utils/cleanup.py @@ -39,6 +39,12 @@ class CleanupManager: Thread Safety: All operations are thread-safe via internal locking. + + Fork Safety: + Not safe to use in forked child processes. If a process is forked + while the singleton is initialised, the child inherits the lock in + an undefined state. Subprocesses spawned via subprocess.Popen are + unaffected (they get a fresh interpreter). """ _instance: "CleanupManager | None" = None From 56a3762f11903070e8813a8276c3f584238f4e39 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 17:27:26 +0100 Subject: [PATCH 27/28] tests: improve CleanupManager unit test quality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Parametrize test_unregister_removes_path over keep_if_debug values - Remove test_register_multiple_files (only tested dict insertion count) - Strengthen test_cleanup_is_idempotent — assert file deleted on first call - Replace test_concurrent_registration with test_concurrent_register_and_cleanup: races register() against run_cleanup() to verify the lock prevents silent data loss, rather than just testing concurrent dict insertions --- tests/unit/utils/test_cleanup.py | 82 +++++++++++++++++++------------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index ddd46199..c5700ee7 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -73,19 +73,6 @@ def test_register_keep_if_debug_stores_true( assert fresh_cleanup_manager._files[test_file.resolve()] is True - def test_register_multiple_files( - self, fresh_cleanup_manager: CleanupManager, tmp_path: Path - ) -> None: - file1 = tmp_path / "file1.txt" - file2 = tmp_path / "file2.txt" - file1.touch() - file2.touch() - - fresh_cleanup_manager.register(file1) - fresh_cleanup_manager.register(file2) - - assert len(fresh_cleanup_manager._files) == 2 - def test_unregister_prevents_deletion( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: @@ -99,25 +86,15 @@ def test_unregister_prevents_deletion( assert test_file.exists() + @pytest.mark.parametrize("keep_if_debug", [False, True]) def test_unregister_removes_path( - self, fresh_cleanup_manager: CleanupManager, tmp_path: Path - ) -> None: - test_file = tmp_path / "test.txt" - test_file.touch() - - fresh_cleanup_manager.register(test_file) - fresh_cleanup_manager.unregister(test_file) - - assert test_file.resolve() not in fresh_cleanup_manager._files - - def test_unregister_removes_keep_if_debug_path( - self, fresh_cleanup_manager: CleanupManager, tmp_path: Path + self, fresh_cleanup_manager: CleanupManager, tmp_path: Path, keep_if_debug: bool ) -> None: - """unregister() removes paths registered with keep_if_debug=True.""" + """unregister() removes the path regardless of keep_if_debug flag.""" test_file = tmp_path / "test.txt" test_file.touch() - fresh_cleanup_manager.register(test_file, keep_if_debug=True) + fresh_cleanup_manager.register(test_file, keep_if_debug=keep_if_debug) fresh_cleanup_manager.unregister(test_file) assert test_file.resolve() not in fresh_cleanup_manager._files @@ -151,8 +128,11 @@ def test_cleanup_is_idempotent( fresh_cleanup_manager.register(test_file) fresh_cleanup_manager.run_cleanup() - fresh_cleanup_manager.run_cleanup() # Second call should be safe + assert not test_file.exists() # deleted on first call + assert fresh_cleanup_manager._cleanup_done + + fresh_cleanup_manager.run_cleanup() # second call must not raise assert fresh_cleanup_manager._cleanup_done def test_cleanup_handles_already_deleted_file( @@ -254,17 +234,32 @@ def test_mixed_registration_in_debug_mode( class TestCleanupManagerThreadSafety: """Tests for thread-safe behavior.""" - def test_concurrent_registration( + def test_concurrent_register_and_cleanup( self, fresh_cleanup_manager: CleanupManager, tmp_path: Path ) -> None: + """Concurrent register() and run_cleanup() calls must not lose files silently. + + The real risk is a file registered while cleanup is iterating _files, + causing either a RuntimeError (dict changed size) or a silently + skipped deletion. This test creates files in parallel with cleanup + and asserts that every file ends up either deleted (cleanup won the + race) or still tracked and then cleaned up explicitly — nothing is + silently lost. + """ num_threads = 10 files_per_thread = 10 + all_files: list[Path] = [] + errors: list[Exception] = [] def register_files(thread_id: int) -> None: - for i in range(files_per_thread): - f = tmp_path / f"thread{thread_id}_file{i}.txt" - f.touch() - fresh_cleanup_manager.register(f) + try: + for i in range(files_per_thread): + f = tmp_path / f"thread{thread_id}_file{i}.txt" + f.touch() + all_files.append(f) + fresh_cleanup_manager.register(f) + except Exception as e: + errors.append(e) threads = [ threading.Thread(target=register_files, args=(i,)) @@ -273,10 +268,29 @@ def register_files(thread_id: int) -> None: for t in threads: t.start() + + # Trigger cleanup mid-registration to exercise lock contention + fresh_cleanup_manager.run_cleanup() + for t in threads: t.join() - assert len(fresh_cleanup_manager._files) == num_threads * files_per_thread + assert not errors, f"Exceptions in registration threads: {errors}" + + # Any file not yet deleted must still be tracked; clean up remaining + for f in all_files: + if f.exists(): + assert f.resolve() in fresh_cleanup_manager._files, ( + f"{f.name} still exists but is no longer tracked — silently lost" + ) + + # Reset idempotency guard so we can clean up remaining files + fresh_cleanup_manager._cleanup_done = False + fresh_cleanup_manager.run_cleanup() + + assert all(not f.exists() for f in all_files), ( + "Some files were neither deleted by cleanup nor cleaned up on retry" + ) @pytest.mark.skipif(IS_WINDOWS, reason="Signal tests not supported on Windows") From 252fb78ec37f574100b71c5439269d7d2b8defff Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Sat, 28 Mar 2026 18:18:53 +0100 Subject: [PATCH 28/28] update PRD --- dev-docs/PRD_AND_ARCHITECTURE.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dev-docs/PRD_AND_ARCHITECTURE.md b/dev-docs/PRD_AND_ARCHITECTURE.md index 6ac890aa..bb1a0dd5 100644 --- a/dev-docs/PRD_AND_ARCHITECTURE.md +++ b/dev-docs/PRD_AND_ARCHITECTURE.md @@ -6079,7 +6079,7 @@ cleanup = get_cleanup_manager() # always returns the singleton cleanup.register(path) # delete path on exit cleanup.register(path, keep_if_debug=True) # skip deletion when NAC_TEST_DEBUG=true cleanup.unregister(path) # cancel a previously registered path -cleanup.cleanup_now() # trigger cleanup immediately (idempotent) +cleanup.run_cleanup() # trigger cleanup immediately (idempotent) ``` **`keep_if_debug` flag** @@ -6092,6 +6092,12 @@ be registered without this flag so they are unconditionally removed. **Thread safety:** all operations are protected by an internal lock. +**Fork safety:** the singleton must not be used in forked child processes. +If a process is forked while `CleanupManager` is initialised, the child +inherits the lock in an undefined state and cleanup behaviour is +unpredictable. Subprocesses spawned via `subprocess.Popen` are unaffected +— they start a fresh interpreter with no inherited singleton state. + **Signal handler behaviour:** on SIGTERM the original handler is restored and the signal is re-raised via `signal.raise_signal()` so upstream process managers (Docker, systemd) see the expected exit status. On SIGINT