From 8725f2fba507905a85196e41ede7a4059f0355f7 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Tue, 1 Sep 2026 09:03:06 +0200 Subject: [PATCH 1/7] perf!: replace merged data model YAML with JSON for faster loading The merged data model is now written and read as JSON instead of YAML. json.dump/json.load replaces ruamel's pure-Python YAML parser, which in one observation reduced per-test data model load time from ~1.9s to ~0.017s on a 2.3 MB file. MERGED_DATA_FILENAME constant value updated to .json; all consumers, tests, fixtures, and documentation updated accordingly. result.yaml fixture replaced with result.json. Closes #931 --- CHANGELOG.md | 6 +- README.md | 2 +- dev-docs/PRD_AND_ARCHITECTURE.md | 71 +++++++++---------- nac_test/core/constants.py | 2 +- nac_test/data_merger.py | 10 +-- nac_test/pyats_core/common/base_test.py | 3 +- .../fixtures/data_merge/result.json | 1 + .../fixtures/data_merge/result.yaml | 18 ----- tests/integration/test_cli_rendering.py | 2 +- tests/unit/test_data_merger.py | 9 +-- tests/utils/test_cleanup.py | 2 +- 11 files changed, 57 insertions(+), 69 deletions(-) create mode 100644 tests/integration/fixtures/data_merge/result.json delete mode 100644 tests/integration/fixtures/data_merge/result.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 302cca30..9e7e83df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Features -- robot rendering: added support for dicts as parent_key in `iterate_list_chunked` +- robot rendering: added support for dicts as parent_key in `iterate_list_chunked` - add support for SDWAN token authentication for pyATS test cases via SDWAN_USERNAME & SDWAN_API_TOKEN ## Bug Fixes @@ -13,6 +13,10 @@ - SSHTestBase.parse_output() is now async — test cases must use await self.parse_output(...) +## Breaking Changes + +- **Internal merged data model file format changed to JSON**: For performance reasons, the internal temporary file used to pass the merged data model to test subprocesses is now written as JSON (`merged_data_model_test_variables.json`, previously `.yaml`). This has no effect on your YAML data files or data model structure. The standard `self.data_model` API is unaffected. This is only breaking if your tests or scripts read the file directly via the `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` environment variable — in that case, switch from YAML parsing to `json.load()`. + # 2.0.0 ## Major Features diff --git a/README.md b/README.md index 95cd2f78..638ca73e 100644 --- a/README.md +++ b/README.md @@ -381,7 +381,7 @@ 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 as `merged_data_model_test_variables.yaml` +3. The merged result is written to the output directory as `merged_data_model_test_variables.json` and will be read by pyATS test cases 4. Both Robot and PyATS tests reference this merged data ### Accessing the Merged Data diff --git a/dev-docs/PRD_AND_ARCHITECTURE.md b/dev-docs/PRD_AND_ARCHITECTURE.md index cad3b960..786e3c46 100644 --- a/dev-docs/PRD_AND_ARCHITECTURE.md +++ b/dev-docs/PRD_AND_ARCHITECTURE.md @@ -472,7 +472,7 @@ sequenceDiagram DataMerger->>DataMerger: Load YAML files DataMerger->>DataMerger: Apply Jinja2 templating DataMerger->>DataMerger: Resolve environment variables - DataMerger-->>CombinedOrch: merged_data_model.yaml + DataMerger-->>CombinedOrch: merged_data_model.json Note over CombinedOrch: Phase 2: Test Discovery CombinedOrch->>PyATSOrch: run_tests() @@ -539,7 +539,7 @@ graph LR subgraph "Phase 1: Data Preparation" A1[Load Data Files] --> A2[Apply Jinja2] A2 --> A3[Merge YAML] - A3 --> A4[Write merged_data_model.yaml] + A3 --> A4[Write merged_data_model.json] end subgraph "Phase 2: Test Discovery" @@ -1104,7 +1104,7 @@ def main( minimal_reports: MinimalReports = False, verbosity: Verbosity = VerbosityLevel.WARNING, version: Version = False, # Handled by eager callback - merged_data_filename: MergedDataFilename = "merged_data_model_test_variables.yaml", + merged_data_filename: MergedDataFilename = "merged_data_model_test_variables.json", ) -> None: """A CLI tool to render and execute Robot Framework tests using Jinja templating.""" @@ -1565,7 +1565,6 @@ nac-test \ 1. **Fail Fast**: Invalid YAML detected before orchestrator initialization 2. **Single Source of Truth**: Both PyATS and Robot read identical merged data 3. **Timing Visibility**: User sees merge time separately from test execution time -4. **Debugging**: Merged data file available for inspection (`cat output/merged_data_model_test_variables.yaml`) **Alternative Rejected**: Merge data inside orchestrator - **Con**: Orchestrator initialization could fail due to data issues (less clear error) @@ -2149,7 +2148,7 @@ async def run_device_job_with_semaphore( "HOSTNAME": hostname, "DEVICE_INFO": json.dumps(device), # Serialized device dict "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH": str( - self.subprocess_runner.output_dir / "merged_data_model_test_variables.yaml" + self.subprocess_runner.output_dir / "merged_data_model_test_variables.json" ), "PYTHONPATH": get_pythonpath_for_tests(self.test_dir, [nac_test_dir]), }) @@ -2205,7 +2204,7 @@ archive_paths = await asyncio.gather(*tasks, return_exceptions=True) ┌─────────────────────────────────────────────────────────────────┐ │ CLI Entry (main.py) │ │ 1. Configure logging │ -│ 2. Merge data files → merged_data_model_test_variables.yaml │ +│ 2. Merge data files → merged_data_model_test_variables.json │ │ 3. Create CombinedOrchestrator │ └─────────────────────────┬───────────────────────────────────────┘ │ @@ -2462,7 +2461,7 @@ Set by PyATSOrchestrator and read by PyATS test subprocesses: | Variable | Purpose | Example Value | |----------|---------|---------------| -| `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` | Absolute path to merged data model | `/path/to/output/merged_data_model_test_variables.yaml` | +| `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` | Absolute path to merged data model | `/path/to/output/merged_data_model_test_variables.json` | | `NAC_TEST_TEST_DIR` | Absolute path to `test_dir` (templates root); used by the progress plugin and archive inspector to compute dot-notation test names relative to this directory | `/path/to/project/templates` | | `PYTHONPATH` | Python path for test discovery | `/path/to/nac-test:/path/to/templates` | | `PYATS_LOG_LEVEL` | PyATS logging level | `ERROR` | @@ -2579,7 +2578,7 @@ Rendering Robot templates... 📁 Results: /output/ 📊 Reports: /output/report.html -📄 Merged data model: /output/merged_data_model_test_variables.yaml +📄 Merged data model: /output/merged_data_model_test_variables.json ==================================================== Total runtime: 3 minutes 26.8 seconds @@ -3612,7 +3611,7 @@ nac_test/ {output_dir}/ ├── combined_summary.html # Root-level combined dashboard ✨ ├── xunit.xml # Merged xUnit XML (Robot + PyATS) ✨ -├── merged_data_model_test_variables.yaml # Merged data model (debugging) +├── merged_data_model_test_variables.json # Merged data model (debugging) ├── robot_results/ # Robot Framework results │ ├── # Rendered .robot files (from -t) │ ├── ordering.txt # Pabot test-level ordering (if applicable) @@ -4277,7 +4276,7 @@ graph TB subgraph "External Systems" APIC[Cisco APIC
REST API] - DataModel[Merged Data Model
YAML] + DataModel[Merged Data Model
JSON] end TenantTest --> APICTestBase @@ -6291,7 +6290,7 @@ project-root/ │ └── test_*.py │ └── output/ # Generated outputs (git-ignored) - ├── merged_data_model.yaml # Merged data file + ├── merged_data_model.json # Merged data file ├── pyats_results/ # Extracted archives │ ├── api/ # API test results │ │ ├── html_reports/ @@ -9417,7 +9416,7 @@ Environment variables solve this elegantly: | Variable Name | Set By | Used By | Purpose | Example Value | |--------------|--------|---------|---------|---------------| -| `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` | Orchestrator | All tests | Absolute path to merged YAML data model | `/path/to/output/merged_data_model_test_variables.yaml` | +| `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` | Orchestrator | All tests | Absolute path to merged JSON data model | `/path/to/output/merged_data_model_test_variables.json` | | `DEVICE_INFO` | Device Executor | D2D tests only | JSON-serialized device connection info | `{"hostname": "cedge-1", "host": "10.1.1.1", "username": "admin", "password": "secret"}` | | `HOSTNAME` | Device Executor | D2D tests only | Current device hostname (for convenience) | `cedge-1` | @@ -9698,7 +9697,7 @@ cat /proc//environ | tr '\0' '\n' # View env vars │ nac-test CLI / Orchestrator │ │ │ │ 1. Read user config (ACI_URL, ACI_USERNAME, ACI_PASSWORD) │ -│ 2. Create merged data model YAML file │ +│ 2. Create merged data model JSON file │ │ 3. Prepare environment: │ │ env = os.environ.copy() │ │ env["MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH"] = "/path/..." │ @@ -10929,7 +10928,7 @@ When nac-test executes, it creates this directory hierarchy: ``` {base_output_dir}/ # User-specified output directory (--output-dir) │ -├── merged_data_model_test_variables.yaml # Merged data model from all input YAMLs +├── merged_data_model_test_variables.json # Merged data model from all input YAMLs │ ├── html_report_data_temp/ # Temporary JSONL files written by tests │ ├── test_{test_id}_001.jsonl # One JSONL file per test execution @@ -11009,7 +11008,7 @@ env["MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH"] = str( ``` **What happens:** -- `main.py` merges all input YAML files into `merged_data_model_test_variables.yaml` +- `main.py` merges all input YAML files into `merged_data_model_test_variables.json` - File written to **base output directory** (not inside `pyats_results/`) - Absolute path passed to test subprocesses via environment variable - Tests read configuration from this central location @@ -11859,7 +11858,7 @@ env.update({ "HOSTNAME": hostname, # e.g., "apic1" "DEVICE_INFO": json.dumps(device), # Full device dict with credentials "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH": str( - self.subprocess_runner.output_dir / "merged_data_model_test_variables.yaml" + self.subprocess_runner.output_dir / "merged_data_model_test_variables.json" ), "PYTHONPATH": get_pythonpath_for_tests(self.test_dir, [nac_test_dir]), # Pass test_dir so the plugin subprocess can compute relative test names @@ -12234,7 +12233,7 @@ print("Job file written to /tmp/manual_job.py") EOF # Step 2: Set required environment variables -export MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH="$(pwd)/output/merged_data_model_test_variables.yaml" +export MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH="$(pwd)/output/merged_data_model_test_variables.json" export PYTHONPATH="$(pwd)/tests:$(pwd)" export PYATS_LOG_LEVEL="INFO" @@ -12273,7 +12272,7 @@ def main(runtime): EOF # Execute custom job -export MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH="$(pwd)/output/merged_data_model_test_variables.yaml" +export MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH="$(pwd)/output/merged_data_model_test_variables.json" export PYTHONPATH="$(pwd)/tests:$(pwd)" pyats run job /tmp/custom_job.py \ @@ -15903,7 +15902,7 @@ deduplicate_list_items() ↓ DataMerger.write_merged_data_model() ↓ -Write: merged_data_model_test_variables.yaml +Write: merged_data_model_test_variables.json ↓ Consumed by: - PyATS tests: Read via MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH env var @@ -15953,9 +15952,9 @@ class DataMerger: def write_merged_data_model( data: Dict[str, Any], output_directory: Path, - filename: str = "merged_data_model_test_variables.yaml", + filename: str = "merged_data_model_test_variables.json", ) -> None: - """Write merged data model to YAML file.""" + """Write merged data model to JSON file.""" full_output_path = output_directory / filename logger.info("Writing merged data model to %s", full_output_path) yaml.write_yaml_file(data, full_output_path) @@ -16416,7 +16415,7 @@ export PROD_USERNAME=admin nac-test run --data data/base.yaml --data data/prod.yaml ``` -**Resulting merged_data_model_test_variables.yaml (dev):** +**Resulting merged_data_model_test_variables.json (dev):** ```yaml --- @@ -16837,7 +16836,7 @@ devices: **Benefits:** 1. **Cross-process Communication**: Subprocess reads via environment variable path -2. **Debugging**: Inspect `merged_data_model_test_variables.yaml` to understand test data +2. **Debugging**: Inspect `merged_data_model_test_variables.json` to understand test data 3. **Single Source of Truth**: One file, one merge, consumed by all tests 4. **Reproducibility**: Archive merged data with test results for troubleshooting @@ -16845,7 +16844,7 @@ devices: ``` output/ -└── merged_data_model_test_variables.yaml ← Single source of truth +└── merged_data_model_test_variables.json ← Single source of truth ``` --- @@ -17374,7 +17373,7 @@ NAC_TEST_VERBOSE=1 nac-test run --data base.yaml %AETEST-INFO: +------------------------------------------------------------------------------+ %AETEST-INFO: | Starting section setup | %AETEST-INFO: +------------------------------------------------------------------------------+ -%SCRIPT-INFO: Loading merged data model from /path/to/merged_data_model_test_variables.yaml +%SCRIPT-INFO: Loading merged data model from /path/to/merged_data_model_test_variables.json %SCRIPT-INFO: Connecting to APIC at https://apic1.example.com %HTTPX-INFO: HTTP Request: GET https://apic1.example.com/api/class/fvTenant.json "HTTP/2 200 OK" -> Section setup passed @@ -19422,7 +19421,7 @@ nac-test run --data base.yaml --verbosity INFO ``` INFO - Loading yaml files from /path/to/base.yaml, /path/to/dev.yaml -INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.yaml +INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.json Discovered 45 PyATS test files Running with 10 parallel workers INFO - Executing 45 API tests using standard PyATS job execution @@ -19457,7 +19456,7 @@ DEBUG - Loaded dev.yaml with 50 lines DEBUG - Performing deep merge of 2 files DEBUG - Merged dict keys: ['apic', 'devices', 'tenants'] DEBUG - Data model conversion completed successfully -INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.yaml +INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.json DEBUG - Discovered test file: /path/to/tests/api/test_apic_tenants.py DEBUG - Discovered test file: /path/to/tests/api/test_apic_vrfs.py ... @@ -19493,7 +19492,7 @@ nac-test run --data base.yaml --verbosity INFO ``` INFO - Loading yaml files from /path/to/base.yaml -INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.yaml +INFO - Writing merged data model to /path/to/output/merged_data_model_test_variables.json Discovered 12 PyATS test files Running with 10 parallel workers INFO - Executing 12 D2D tests using device-centric execution with connection broker @@ -22782,7 +22781,7 @@ self.result_collector.add_result( **5.2: Data Model Access Contract** -Tests access merged YAML data via `self.data_model`: +Tests access merged JSON data via `self.data_model`: ```python @aetest.setup @@ -24732,7 +24731,7 @@ if self.dev_pyats_only: **Output Structure** (PyATS Only): ``` output_dir/ -├── merged_data_model_test_variables.yaml # SOT - always created +├── merged_data_model_test_variables.json # SOT - always created ├── pyats_results/ # PyATS-specific directory │ ├── api/ # API test archives │ │ └── api_tests_YYYYMMDD_HHMMSS_mmm.tar.gz @@ -24861,7 +24860,7 @@ if self.dev_robot_only: **Output Structure** (Robot Only): ``` output_dir/ -├── merged_data_model_test_variables.yaml # SOT - always created +├── merged_data_model_test_variables.json # SOT - always created ├── rendered/ # Rendered .robot files │ ├── test_suite_1.robot │ └── test_suite_2.robot @@ -25060,7 +25059,7 @@ def _print_execution_summary(self, has_pyats: bool, has_robot: bool) -> None: **Output Structure** (Combined): ``` output_dir/ -├── merged_data_model_test_variables.yaml # SOT - always created first +├── merged_data_model_test_variables.json # SOT - always created first │ ├── pyats_results/ # PyATS directory │ ├── api/ @@ -25216,7 +25215,7 @@ merge-data: - nac-test -d data/ -t templates/ -o output/ --render-only artifacts: paths: - - output/merged_data_model_test_variables.yaml + - output/merged_data_model_test_variables.json # Stage 2: PyATS tests in parallel test-api: @@ -25279,7 +25278,7 @@ $ nac-test -d data/ -t templates/ -o output/ 📁 Results: output/pyats_results/ 📊 Reports: output/pyats_results/html_reports/ -📄 Merged data model: output/merged_data_model_test_variables.yaml +📄 Merged data model: output/merged_data_model_test_variables.json ================================================== Total runtime: 5 minutes 32 seconds @@ -25309,7 +25308,7 @@ $ nac-test -d data/ -t templates/ -o output/ 📁 Results: output/ 📊 Reports: output/report.html -📄 Merged data model: output/merged_data_model_test_variables.yaml +📄 Merged data model: output/merged_data_model_test_variables.json ================================================== Total runtime: 12 minutes 8 seconds @@ -25463,7 +25462,7 @@ $ nac-test -d data/ -t templates/ -o output/ --pyats - vs merge twice (2-4 seconds + potential inconsistency) 3. **Debugging Simplicity**: One file to inspect for troubleshooting - - `output/merged_data_model_test_variables.yaml` shows exactly what tests see + - `output/merged_data_model_test_variables.json` shows exactly what tests see - No need to compare PyATS vs Robot merged data - Reduced cognitive load diff --git a/nac_test/core/constants.py b/nac_test/core/constants.py index 03ecca80..513e2577 100644 --- a/nac_test/core/constants.py +++ b/nac_test/core/constants.py @@ -86,7 +86,7 @@ REPORT_HTML: str = "report.html" XUNIT_XML: str = "xunit.xml" ORDERING_FILENAME: str = "ordering.txt" -MERGED_DATA_FILENAME: str = "merged_data_model_test_variables.yaml" +MERGED_DATA_FILENAME: str = "merged_data_model_test_variables.json" # Owner read/write only — prevents other users from reading sensitive merged data MERGED_DATA_FILE_MODE: int = 0o600 SUMMARY_SEPARATOR_WIDTH: int = 70 diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index e78fa557..9de46c6b 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -3,6 +3,7 @@ """Shared data merging utilities for both Robot and PyATS test execution.""" +import json import logging import os from pathlib import Path @@ -50,7 +51,7 @@ def merged_data_path(output_directory: Path) -> Path: output_directory: Base output directory passed to write_merged_data_model() Returns: - Full path to the merged data model YAML file + Full path to the merged data model JSON file """ return output_directory / MERGED_DATA_FILENAME @@ -59,21 +60,22 @@ def write_merged_data_model( data: dict[str, Any], output_directory: Path, ) -> Path: - """Write merged data model to YAML file. + """Write merged data model to JSON file. 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 + output_directory: Directory where the JSON file will be saved Returns: Path to the written file (use this instead of reconstructing the path) """ 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) + with open(full_output_path, "w", encoding="utf-8") as f: + json.dump(data, f) if not IS_WINDOWS: os.chmod(full_output_path, MERGED_DATA_FILE_MODE) return full_output_path diff --git a/nac_test/pyats_core/common/base_test.py b/nac_test/pyats_core/common/base_test.py index 5dde72e1..b6e3c029 100644 --- a/nac_test/pyats_core/common/base_test.py +++ b/nac_test/pyats_core/common/base_test.py @@ -51,7 +51,6 @@ from nac_test.pyats_core.reporting.types import ResultStatus from nac_test.utils import sanitize_hostname from nac_test.utils.formatting import format_file_timestamp_ms -from nac_test.utils.yaml import safe_load T = TypeVar("T") @@ -827,7 +826,7 @@ def load_data_model(self) -> dict[str, Any]: ) with open(data_file, encoding="utf-8") as f: - data = safe_load(f) + data = json.load(f) return data if isinstance(data, dict) else {} def get_default_value( diff --git a/tests/integration/fixtures/data_merge/result.json b/tests/integration/fixtures/data_merge/result.json new file mode 100644 index 00000000..69f097ca --- /dev/null +++ b/tests/integration/fixtures/data_merge/result.json @@ -0,0 +1 @@ +{"defaults": {"apic": {"version": 6.0}}, "root": {"attr1": "value1", "primitive_list": ["item1", "item1", "item1"], "dict_list": [{"name": "abc", "extra": "def"}], "dict_list_extra": [{"name": "abc", "extra1": "def", "extra2": "ghi"}], "attr2": "value2"}} \ No newline at end of file diff --git a/tests/integration/fixtures/data_merge/result.yaml b/tests/integration/fixtures/data_merge/result.yaml deleted file mode 100644 index 0a8c597e..00000000 --- a/tests/integration/fixtures/data_merge/result.yaml +++ /dev/null @@ -1,18 +0,0 @@ ---- -defaults: - apic: - version: 6.0 -root: - attr1: value1 - primitive_list: - - item1 - - item1 - - item1 - dict_list: - - name: abc - extra: def - dict_list_extra: - - name: abc - extra1: def - extra2: ghi - attr2: value2 diff --git a/tests/integration/test_cli_rendering.py b/tests/integration/test_cli_rendering.py index b203a65e..0e90dca0 100644 --- a/tests/integration/test_cli_rendering.py +++ b/tests/integration/test_cli_rendering.py @@ -314,7 +314,7 @@ def test_merged_data_model_creates_default_filename(tmp_path: Path) -> None: templates_path = "tests/integration/fixtures/templates/" output_model_path = tmp_path / MERGED_DATA_FILENAME data_dir = Path("tests/integration/fixtures/data_merge") - expected_model_path = data_dir / "result.yaml" + expected_model_path = data_dir / "result.json" result = runner.invoke( nac_test.cli.main.app, diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index f28d1d7f..648961c1 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -5,12 +5,12 @@ Covers: - merge_data_files: empty input edge case, ruamel type stripping contract -- write_merged_data_model: output filename, YAML roundtrip +- write_merged_data_model: output filename, JSON roundtrip """ +import json from pathlib import Path -from nac_yaml import yaml from ruamel.yaml import CommentedMap, CommentedSeq from nac_test.data_merger import DataMerger @@ -40,10 +40,11 @@ def test_writes_no_extra_files(self, tmp_path: Path) -> None: assert len(list(tmp_path.iterdir())) == 1 def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: - """Data written to YAML can be read back with the same structure.""" + """Data written to JSON can be read back with the same structure.""" original = {"host": "router1", "vlan": 100, "tags": ["a", "b"]} output_path = DataMerger.write_merged_data_model(original, tmp_path) - reloaded = yaml.load_yaml_files([output_path]) + with open(output_path, encoding="utf-8") as f: + reloaded = json.load(f) assert reloaded["host"] == "router1" assert reloaded["vlan"] == 100 assert list(reloaded["tags"]) == ["a", "b"] diff --git a/tests/utils/test_cleanup.py b/tests/utils/test_cleanup.py index 9c3b1e2f..40edd387 100644 --- a/tests/utils/test_cleanup.py +++ b/tests/utils/test_cleanup.py @@ -39,7 +39,7 @@ def test_removes_test_type_directories(self, tmp_path: Path) -> None: ("nac_test_job_d2d_20250224.zip", True), (PYATS_RESULTS_DIRNAME, False), ("robot_results", False), - ("merged_data_model.yaml", True), + ("merged_data_model.json", True), ], ) def test_preserves_expected_paths( From da927ba55da54052423ab67654e5a4bd69c4ba93 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 18:23:33 +0200 Subject: [PATCH 2/7] feat: add NAC_TEST_DUMP_YAML_DATA_MODEL to dump merged data model as YAML Optionally write the merged data model as a companion YAML file alongside the JSON, gated by the NAC_TEST_DUMP_YAML_DATA_MODEL env var, for post-run inspection/debugging. The YAML file is not registered for cleanup (persists after the run) and may contain sensitive values, so it is written with 0o600 permissions and a warning is logged advising manual removal. - add dump_yaml parameter to DataMerger.write_merged_data_model() - wire NAC_TEST_DUMP_YAML_DATA_MODEL constant through the CLI - unit test for JSON/YAML content parity - subprocess integration test verifying the YAML persists after exit - assert no YAML by default in existing render test - document env var in README and CHANGELOG --- CHANGELOG.md | 1 + README.md | 1 + nac_test/cli/main.py | 5 +- nac_test/core/constants.py | 3 + nac_test/data_merger.py | 27 ++++++- tests/integration/test_cli_rendering.py | 5 ++ .../integration/test_yaml_data_model_dump.py | 81 +++++++++++++++++++ tests/unit/test_data_merger.py | 36 ++++++++- 8 files changed, 155 insertions(+), 4 deletions(-) create mode 100644 tests/integration/test_yaml_data_model_dump.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e7e83df..c930df95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - robot rendering: added support for dicts as parent_key in `iterate_list_chunked` - add support for SDWAN token authentication for pyATS test cases via SDWAN_USERNAME & SDWAN_API_TOKEN +- add `NAC_TEST_DUMP_YAML_DATA_MODEL` environment variable to also write the merged data model as YAML (alongside the JSON file) for post-run inspection/debugging. The YAML file is not auto-cleaned up and may contain sensitive values (passwords, tokens, credentials), so review and remove it manually. ## Bug Fixes diff --git a/README.md b/README.md index 638ca73e..604095c6 100644 --- a/README.md +++ b/README.md @@ -818,6 +818,7 @@ In addition to CLI options, `nac-test` supports several environment variables fo | Variable | Default | Description | |----------|---------|-------------| | `NAC_TEST_VERBOSE` | unset | Enable verbose mode: verbose output and retain intermediate files (see `NAC_TEST_PYATS_KEEP_REPORT_DATA`) | +| `NAC_TEST_DUMP_YAML_DATA_MODEL` | unset | Write merged data model as YAML for debugging. File is not auto-cleaned up; you must remove it manually. WARNING: May contain sensitive values. | ## Troubleshooting diff --git a/nac_test/cli/main.py b/nac_test/cli/main.py index 539b4bfa..1f85a7ac 100644 --- a/nac_test/cli/main.py +++ b/nac_test/cli/main.py @@ -22,6 +22,7 @@ from nac_test.combined_orchestrator import CombinedOrchestrator from nac_test.core.constants import ( DEBUG_MODE, + DUMP_YAML_DATA_MODEL, EXIT_DATA_ERROR, EXIT_ERROR, EXIT_INTERRUPTED, @@ -390,7 +391,9 @@ def main( typer.echo("\n\n📄 Merging data model files...") merged_data = DataMerger.merge_data_files(data) - merged_data_path = DataMerger.write_merged_data_model(merged_data, output) + merged_data_path = DataMerger.write_merged_data_model( + merged_data, output, dump_yaml=DUMP_YAML_DATA_MODEL + ) # Register merged data file for cleanup — always delete, even in debug mode, # because it may contain credentials resolved from !env references. diff --git a/nac_test/core/constants.py b/nac_test/core/constants.py index 513e2577..cd9354c5 100644 --- a/nac_test/core/constants.py +++ b/nac_test/core/constants.py @@ -40,6 +40,9 @@ # Set NAC_TEST_DISABLE_TESTLEVELSPLIT=true to disable test-level parallelization DISABLE_TESTLEVELSPLIT: bool = get_bool_env("NAC_TEST_DISABLE_TESTLEVELSPLIT") +# Merged data model YAML dump for debugging +# Set NAC_TEST_DUMP_YAML_DATA_MODEL=true to write YAML alongside JSON +DUMP_YAML_DATA_MODEL: bool = get_bool_env("NAC_TEST_DUMP_YAML_DATA_MODEL") # Report timestamp format - single source of truth for all report generators REPORT_TIMESTAMP_FORMAT: str = "%Y-%m-%d %H:%M:%S" diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index 9de46c6b..4dc010a6 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -59,18 +59,25 @@ def merged_data_path(output_directory: Path) -> Path: def write_merged_data_model( data: dict[str, Any], output_directory: Path, + dump_yaml: bool = False, ) -> Path: - """Write merged data model to JSON file. + """Write merged data model to JSON file and optionally YAML. The output filename is always MERGED_DATA_FILENAME — the single fixed location used by all consumers (Robot, PyATS subprocesses, cleanup). + When dump_yaml is True, a companion YAML file is also written with the + same data. The YAML file is not auto-cleaned up and may contain sensitive + values. + Args: data: The merged data dictionary to write output_directory: Directory where the JSON file will be saved + dump_yaml: If True, also write a YAML file for debugging purposes. + Defaults to False. Returns: - Path to the written file (use this instead of reconstructing the path) + Path to the written JSON file (use this instead of reconstructing the path) """ full_output_path = DataMerger.merged_data_path(output_directory) logger.info("Writing merged data model to %s", full_output_path) @@ -78,4 +85,20 @@ def write_merged_data_model( json.dump(data, f) if not IS_WINDOWS: os.chmod(full_output_path, MERGED_DATA_FILE_MODE) + + # Optionally write YAML for debugging + if dump_yaml: + yaml_output_path = full_output_path.with_suffix(".yaml") + try: + yaml.write_yaml_file(data, yaml_output_path) + if not IS_WINDOWS: + os.chmod(yaml_output_path, MERGED_DATA_FILE_MODE) + logger.warning( + "Created %s for post-run debugging. It may contain sensitive values " + "(passwords, tokens, credentials). Please review and remove the file manually.", + yaml_output_path, + ) + except Exception as e: + logger.warning("Failed to write YAML data model: %s", e) + return full_output_path diff --git a/tests/integration/test_cli_rendering.py b/tests/integration/test_cli_rendering.py index 0e90dca0..2dc3162c 100644 --- a/tests/integration/test_cli_rendering.py +++ b/tests/integration/test_cli_rendering.py @@ -341,6 +341,11 @@ def test_merged_data_model_creates_default_filename(tmp_path: Path) -> None: f"Merged data model content should match expected content from " f"{expected_model_path}" ) + # By default (NAC_TEST_DUMP_YAML_DATA_MODEL unset) no YAML companion is written. + # The env-var-enabled case is covered in test_yaml_data_model_dump.py. + assert not output_model_path.with_suffix(".yaml").exists(), ( + "YAML data model should not be created unless NAC_TEST_DUMP_YAML_DATA_MODEL is set" + ) def test_render_only_without_controller_credentials(tmp_path: Path) -> None: diff --git a/tests/integration/test_yaml_data_model_dump.py b/tests/integration/test_yaml_data_model_dump.py new file mode 100644 index 00000000..c1eb8c2f --- /dev/null +++ b/tests/integration/test_yaml_data_model_dump.py @@ -0,0 +1,81 @@ +# SPDX-License-Identifier: MPL-2.0 +# Copyright (c) 2025 Daniel Schmidt + +"""Integration test for the optional YAML merged-data-model dump. + +When ``NAC_TEST_DUMP_YAML_DATA_MODEL`` is set, nac-test writes a companion +YAML file next to the JSON merged data model. Unlike the JSON file, the YAML +file is intentionally *not* registered with the CleanupManager, so it survives +after the run for post-run debugging. + +This test runs the real ``nac-test`` CLI in a subprocess. A subprocess is +required because ``DUMP_YAML_DATA_MODEL`` is evaluated from the environment at +import time — an in-process runner (e.g. CliRunner) cannot toggle it after the +module is already imported. The default (env var unset) case is covered by +``test_cli_rendering.py::test_merged_data_model_creates_default_filename``. +""" + +import os +import stat +import subprocess +from pathlib import Path + +import pytest + +from nac_test.core.constants import IS_WINDOWS, MERGED_DATA_FILENAME +from nac_test.utils.yaml import safe_load + +pytestmark = [pytest.mark.integration, pytest.mark.windows] + + +def test_yaml_dump_created_when_env_set(tmp_path: Path) -> None: + """With the env var set, the YAML dump is written and persists after exit. + + We assert on the YAML file (which is *not* cleanup-registered and therefore + survives) rather than the JSON file. Note that we do not assert here that the + JSON was removed: JSON cleanup-on-exit is already covered end-to-end by + ``tests/e2e/test_e2e_scenarios.py::test_merged_data_file_removed_after_run``. + """ + templates_path = "tests/integration/fixtures/templates/" + data_dir = Path("tests/integration/fixtures/data_merge") + yaml_output_path = (tmp_path / MERGED_DATA_FILENAME).with_suffix(".yaml") + + env = {**os.environ, "NAC_TEST_DUMP_YAML_DATA_MODEL": "true"} + result = subprocess.run( + [ + "nac-test", + "-d", + str(data_dir / "file1.yaml"), + "-d", + str(data_dir / "file2.yaml"), + "-t", + templates_path, + "-o", + str(tmp_path), + "--render-only", + ], + capture_output=True, + text=True, + encoding="utf-8", + env=env, + ) + assert result.returncode == 0, ( + f"nac-test should succeed, got exit code {result.returncode}\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + + assert yaml_output_path.exists(), ( + f"YAML dump should persist at {yaml_output_path} when " + "NAC_TEST_DUMP_YAML_DATA_MODEL=true (it is not cleanup-registered)" + ) + + # Sanity check: the YAML deserializes to the expected merged content. + yaml_data = safe_load(yaml_output_path.read_text(encoding="utf-8")) + assert isinstance(yaml_data, dict) and yaml_data, "YAML dump should be non-empty" + + # YAML dump must carry the same restrictive permissions as the JSON file. + if not IS_WINDOWS: + yaml_mode = stat.S_IMODE(yaml_output_path.stat().st_mode) + assert yaml_mode == 0o600, ( + f"YAML dump should have 0o600 permissions, got {oct(yaml_mode)}" + ) diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index 648961c1..3ef13e2d 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -5,7 +5,7 @@ Covers: - merge_data_files: empty input edge case, ruamel type stripping contract -- write_merged_data_model: output filename, JSON roundtrip +- write_merged_data_model: output filename, JSON roundtrip, YAML content parity """ import json @@ -14,6 +14,7 @@ from ruamel.yaml import CommentedMap, CommentedSeq from nac_test.data_merger import DataMerger +from nac_test.utils.yaml import safe_load class TestMergeDataFiles: @@ -49,6 +50,39 @@ def test_roundtrip_preserves_content(self, tmp_path: Path) -> None: assert reloaded["vlan"] == 100 assert list(reloaded["tags"]) == ["a", "b"] + def test_yaml_content_matches_json_when_dumped(self, tmp_path: Path) -> None: + """When dump_yaml=True, YAML content matches JSON content. + + This test verifies the content parity contract: the YAML and JSON files + contain identical data when deserialized. + """ + # Create test data + test_data = { + "host": "router1", + "vlan": 100, + "tags": ["a", "b"], + "nested": {"key": "value"}, + } + + # Write the merged data model with YAML enabled + DataMerger.write_merged_data_model(test_data, tmp_path, dump_yaml=True) + + # Verify both files exist + json_path = tmp_path / "merged_data_model_test_variables.json" + yaml_path = tmp_path / "merged_data_model_test_variables.yaml" + + assert json_path.exists(), "JSON file should be created" + assert yaml_path.exists(), "YAML file should be created when dump_yaml=True" + + # Load both and verify content matches + with open(json_path, encoding="utf-8") as f: + json_data = json.load(f) + yaml_data = safe_load(yaml_path.read_text(encoding="utf-8")) + + assert json_data == yaml_data, ( + f"YAML and JSON content should match.\nJSON: {json_data}\nYAML: {yaml_data}" + ) + def _assert_no_ruamel_types(value: object, path: str = "root") -> None: """Recursively assert no CommentedMap/CommentedSeq anywhere in the tree.""" From 0df19a2613b36aec633105370a304a9a9995d3d9 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 18:43:40 +0200 Subject: [PATCH 3/7] docs: drop stale "YAML" qualifier from merged data model docstrings The merged data model file has been JSON since 383c957 (perf!: replace merged data model YAML with JSON). Update two stale docstrings that still described it as YAML. --- nac_test/pyats_core/execution/device/device_executor.py | 2 +- tests/e2e/test_e2e_scenarios.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nac_test/pyats_core/execution/device/device_executor.py b/nac_test/pyats_core/execution/device/device_executor.py index f44971fd..c20ec717 100644 --- a/nac_test/pyats_core/execution/device/device_executor.py +++ b/nac_test/pyats_core/execution/device/device_executor.py @@ -41,7 +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 + merged_data_path: Path to the merged data model file custom_testbed_path: Optional path to custom PyATS testbed YAML """ self.job_generator = job_generator diff --git a/tests/e2e/test_e2e_scenarios.py b/tests/e2e/test_e2e_scenarios.py index 151a4536..15f1a07a 100644 --- a/tests/e2e/test_e2e_scenarios.py +++ b/tests/e2e/test_e2e_scenarios.py @@ -179,7 +179,7 @@ def test_output_root_contains_only_expected_entries( ) def test_merged_data_file_removed_after_run(self, results: E2EResults) -> None: - """Merged data model YAML must not persist after a successful run. + """Merged data model file 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. From 3c28fea760ee185ea1c80c23c76a1aff69dbce3f Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 18:44:12 +0200 Subject: [PATCH 4/7] test: create merged data model fixtures as JSON The merged data model file is JSON (parsed via json.load in base_test). Update remaining test fixtures that created or referenced it as YAML so they match the real format: - rename dummy .yaml files to .json - write JSON content instead of YAML --- .../test_controller_detection_integration.py | 4 ++-- .../common/test_base_test_controller_detection.py | 4 ++-- .../common/test_base_test_result_collector.py | 2 +- tests/unit/test_combined_orchestrator_controller.py | 12 ++++++------ tests/unit/utils/test_cleanup.py | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/integration/test_controller_detection_integration.py b/tests/integration/test_controller_detection_integration.py index ed8fc18b..0efafa9b 100644 --- a/tests/integration/test_controller_detection_integration.py +++ b/tests/integration/test_controller_detection_integration.py @@ -34,8 +34,8 @@ def test_end_to_end_controller_detection( output_dir.mkdir() # Create a dummy merged data file - merged_file = output_dir / "merged_data.yaml" - merged_file.write_text("test: data") + merged_file = output_dir / "merged_data.json" + merged_file.write_text('{"test": "data"}') # Create a dummy test file test_file = test_dir / "test_dummy.py" diff --git a/tests/pyats_core/common/test_base_test_controller_detection.py b/tests/pyats_core/common/test_base_test_controller_detection.py index e6cdd44a..424e4b45 100644 --- a/tests/pyats_core/common/test_base_test_controller_detection.py +++ b/tests/pyats_core/common/test_base_test_controller_detection.py @@ -21,8 +21,8 @@ def setup_test_data_file_env( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> Generator[Path, None, None]: """Create temp data file and set MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH.""" - temp_file = tmp_path / "test.yaml" - temp_file.write_text("test: data") + temp_file = tmp_path / "test.json" + temp_file.write_text('{"test": "data"}') monkeypatch.setenv("MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH", str(temp_file)) yield temp_file diff --git a/tests/pyats_core/common/test_base_test_result_collector.py b/tests/pyats_core/common/test_base_test_result_collector.py index 2f3a23f2..6c067f70 100644 --- a/tests/pyats_core/common/test_base_test_result_collector.py +++ b/tests/pyats_core/common/test_base_test_result_collector.py @@ -25,7 +25,7 @@ def test_falls_back_to_cwd_when_data_file_missing( # Point to non-existent file to trigger fallback monkeypatch.setenv( - "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH", "/nonexistent/path.yaml" + "MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH", "/nonexistent/path.json" ) class TestClass(NACTestBase): diff --git a/tests/unit/test_combined_orchestrator_controller.py b/tests/unit/test_combined_orchestrator_controller.py index e76d4246..41ef5e66 100644 --- a/tests/unit/test_combined_orchestrator_controller.py +++ b/tests/unit/test_combined_orchestrator_controller.py @@ -171,8 +171,8 @@ def test_combined_orchestrator_passes_controller_to_pyats( output_dir = tmp_path / "output" output_dir.mkdir() - merged_file = output_dir / "merged.yaml" - merged_file.write_text("merged: data") + merged_file = output_dir / "merged.json" + merged_file.write_text('{"merged": "data"}') sdwan_auth = AuthCheckResult( success=True, @@ -279,8 +279,8 @@ def test_render_only_mode_does_not_instantiate_pyats_orchestrator( output_dir = tmp_path / "output" output_dir.mkdir() - merged_file = output_dir / "merged.yaml" - merged_file.write_text("test: data") + merged_file = output_dir / "merged.json" + merged_file.write_text('{"test": "data"}') # Initialize CombinedOrchestrator with render_only=True # This should NOT raise typer.Exit despite missing credentials @@ -342,8 +342,8 @@ def test_combined_orchestrator_production_mode_passes_controller( output_dir = tmp_path / "output" output_dir.mkdir() - merged_file = output_dir / "merged.yaml" - merged_file.write_text("merged: data") + merged_file = output_dir / "merged.json" + merged_file.write_text('{"merged": "data"}') cc_auth = AuthCheckResult( success=True, diff --git a/tests/unit/utils/test_cleanup.py b/tests/unit/utils/test_cleanup.py index 76689ba8..0d4a1479 100644 --- a/tests/unit/utils/test_cleanup.py +++ b/tests/unit/utils/test_cleanup.py @@ -232,7 +232,7 @@ 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" + sensitive = tmp_path / "merged_data.json" debug_file = tmp_path / "job.py" sensitive.touch() debug_file.touch() From 0d5f2496e658bb6fe34cd768cd115b31a89109e3 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 18:44:55 +0200 Subject: [PATCH 5/7] test: consolidate PyATSTestDirs and pyats_test_dirs into tests/conftest.py The PyATSTestDirs NamedTuple and pyats_test_dirs fixture were duplicated identically in tests/pyats_core/conftest.py and tests/unit/conftest.py. Move both to the top-level tests/conftest.py so they are shared across the whole suite, and repoint the seven importers accordingly. --- tests/conftest.py | 27 ++++++++++++++ tests/pyats_core/conftest.py | 32 ++--------------- tests/pyats_core/progress/test_plugin.py | 2 +- .../test_device_executor_env_propagation.py | 3 +- .../test_orchestrator_controller_detection.py | 3 +- .../test_orchestrator_controller_param.py | 3 +- tests/pyats_core/test_orchestrator_dry_run.py | 3 +- tests/pyats_core/test_orchestrator_env_var.py | 3 +- tests/unit/conftest.py | 36 ++++--------------- .../test_orchestrator_config_error.py | 3 +- 10 files changed, 43 insertions(+), 72 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e5b011ab..ed2c4694 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,6 +14,7 @@ import tempfile from collections.abc import Generator from pathlib import Path +from typing import NamedTuple import pytest @@ -29,6 +30,14 @@ ) +class PyATSTestDirs(NamedTuple): + """Directory structure for PyATS orchestrator tests.""" + + test_dir: Path + output_dir: Path + merged_file: Path + + def assert_is_link_to(link: Path, source: Path) -> None: """Assert that link points to source as either a hard link or symlink.""" if link.is_symlink(): @@ -193,3 +202,21 @@ def cc_context() -> ControllerContext: def iosxe_context() -> ControllerContext: """Pre-built ControllerContext for IOS-XE with session auth.""" return ControllerContext(controller_type="IOSXE", auth_method=AuthMethod.SESSION) + + +@pytest.fixture() +def pyats_test_dirs(tmp_path: Path) -> PyATSTestDirs: + """Create standard directory structure for PyATS orchestrator tests. + + Returns: + PyATSTestDirs with test_dir, output_dir, and merged_file paths. + """ + test_dir = tmp_path / "tests" + test_dir.mkdir() + output_dir = tmp_path / "output" + output_dir.mkdir() + merged_file = output_dir / "merged.json" + merged_file.write_text('{"test": "data"}') + return PyATSTestDirs( + test_dir=test_dir, output_dir=output_dir, merged_file=merged_file + ) diff --git a/tests/pyats_core/conftest.py b/tests/pyats_core/conftest.py index 12aa979e..188eb359 100644 --- a/tests/pyats_core/conftest.py +++ b/tests/pyats_core/conftest.py @@ -6,23 +6,15 @@ NOTE: This module intentionally duplicates some patterns from tests/unit/conftest.py. Issue #541 will merge tests/pyats_core/ into tests/unit/, at which point these fixtures should be consolidated into a single conftest.py. -""" -from pathlib import Path -from typing import NamedTuple +The PyATSTestDirs type and the pyats_test_dirs fixture live in the top-level +tests/conftest.py so they are shared across the whole test suite. +""" import pytest from _pytest.monkeypatch import MonkeyPatch -class PyATSTestDirs(NamedTuple): - """Directory structure for PyATS orchestrator tests.""" - - test_dir: Path - output_dir: Path - merged_file: Path - - @pytest.fixture() def aci_controller_env(monkeypatch: MonkeyPatch) -> None: """Set ACI controller environment variables.""" @@ -45,21 +37,3 @@ def cc_controller_env(monkeypatch: MonkeyPatch) -> None: monkeypatch.setenv("CC_URL", "https://cc.test.com") monkeypatch.setenv("CC_USERNAME", "admin") monkeypatch.setenv("CC_PASSWORD", "password") - - -@pytest.fixture() -def pyats_test_dirs(tmp_path: Path) -> PyATSTestDirs: - """Create standard directory structure for PyATS orchestrator tests. - - Returns: - PyATSTestDirs with test_dir, output_dir, and merged_file paths. - """ - test_dir = tmp_path / "tests" - test_dir.mkdir() - output_dir = tmp_path / "output" - output_dir.mkdir() - merged_file = output_dir / "merged.yaml" - merged_file.write_text("test: data") - return PyATSTestDirs( - test_dir=test_dir, output_dir=output_dir, merged_file=merged_file - ) diff --git a/tests/pyats_core/progress/test_plugin.py b/tests/pyats_core/progress/test_plugin.py index fda85c23..80d25731 100644 --- a/tests/pyats_core/progress/test_plugin.py +++ b/tests/pyats_core/progress/test_plugin.py @@ -11,7 +11,7 @@ from nac_test.pyats_core.constants import ENV_TEST_DIR from nac_test.pyats_core.progress.plugin import ProgressReporterPlugin -from tests.pyats_core.conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs def _make_plugin( diff --git a/tests/pyats_core/test_device_executor_env_propagation.py b/tests/pyats_core/test_device_executor_env_propagation.py index f622f510..7ac179e1 100644 --- a/tests/pyats_core/test_device_executor_env_propagation.py +++ b/tests/pyats_core/test_device_executor_env_propagation.py @@ -14,8 +14,7 @@ from nac_test.pyats_core.execution.device.testbed_generator import TestbedGenerator from nac_test.pyats_core.execution.job_generator import JobGenerator from nac_test.pyats_core.execution.subprocess_runner import SubprocessRunner - -from .conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs class TestDeviceExecutorEnvPropagation: diff --git a/tests/pyats_core/test_orchestrator_controller_detection.py b/tests/pyats_core/test_orchestrator_controller_detection.py index ebe6bb06..c89c4ef9 100644 --- a/tests/pyats_core/test_orchestrator_controller_detection.py +++ b/tests/pyats_core/test_orchestrator_controller_detection.py @@ -7,8 +7,7 @@ from nac_test.core.constants import EXIT_ERROR from nac_test.pyats_core.orchestrator import PyATSOrchestrator - -from .conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs class TestOrchestratorControllerDetection: diff --git a/tests/pyats_core/test_orchestrator_controller_param.py b/tests/pyats_core/test_orchestrator_controller_param.py index 1ae53e4a..6b034099 100644 --- a/tests/pyats_core/test_orchestrator_controller_param.py +++ b/tests/pyats_core/test_orchestrator_controller_param.py @@ -7,8 +7,7 @@ from nac_test.core.types import AuthMethod, ControllerContext from nac_test.pyats_core.orchestrator import PyATSOrchestrator - -from .conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs class TestOrchestratorControllerParam: diff --git a/tests/pyats_core/test_orchestrator_dry_run.py b/tests/pyats_core/test_orchestrator_dry_run.py index 36903e2e..2b0cc0f2 100644 --- a/tests/pyats_core/test_orchestrator_dry_run.py +++ b/tests/pyats_core/test_orchestrator_dry_run.py @@ -14,8 +14,7 @@ TestFileMetadata, ) from nac_test.pyats_core.orchestrator import PyATSOrchestrator - -from .conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs def _make_execution_plan( diff --git a/tests/pyats_core/test_orchestrator_env_var.py b/tests/pyats_core/test_orchestrator_env_var.py index 079e374f..962d336d 100644 --- a/tests/pyats_core/test_orchestrator_env_var.py +++ b/tests/pyats_core/test_orchestrator_env_var.py @@ -15,8 +15,7 @@ from nac_test.pyats_core.constants import ENV_TEST_DIR from nac_test.pyats_core.execution.subprocess_runner import SubprocessRunner from nac_test.pyats_core.orchestrator import PyATSOrchestrator - -from .conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs class TestOrchestratorEnvVarProcesses: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index ad5f2cb9..f1e489e9 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,10 +1,13 @@ # SPDX-License-Identifier: MPL-2.0 # Copyright (c) 2025 Daniel Schmidt -"""Shared fixtures for unit tests.""" +"""Shared fixtures for unit tests. -from pathlib import Path -from typing import Any, NamedTuple +NOTE: The PyATSTestDirs type and the pyats_test_dirs fixture live in the +top-level tests/conftest.py so they are shared across the whole test suite. +""" + +from typing import Any from unittest.mock import AsyncMock, Mock import pytest @@ -16,15 +19,6 @@ PYATS_POST_DISCONNECT_WAIT_SECONDS, ) - -class PyATSTestDirs(NamedTuple): - """Directory structure for PyATS orchestrator tests.""" - - test_dir: Path - output_dir: Path - merged_file: Path - - AUTH_SUCCESS = AuthCheckResult( success=True, reason=AuthOutcome.SUCCESS, @@ -121,21 +115,3 @@ def cc_controller_env(monkeypatch: MonkeyPatch) -> None: monkeypatch.setenv("CC_URL", "https://cc.test.com") monkeypatch.setenv("CC_USERNAME", "admin") monkeypatch.setenv("CC_PASSWORD", "test_pass") - - -@pytest.fixture() -def pyats_test_dirs(tmp_path: Path) -> PyATSTestDirs: - """Create standard directory structure for PyATS orchestrator tests. - - Returns: - PyATSTestDirs with test_dir, output_dir, and merged_file paths. - """ - test_dir = tmp_path / "tests" - test_dir.mkdir() - output_dir = tmp_path / "output" - output_dir.mkdir() - merged_file = output_dir / "merged.yaml" - merged_file.write_text("test: data") - return PyATSTestDirs( - test_dir=test_dir, output_dir=output_dir, merged_file=merged_file - ) diff --git a/tests/unit/pyats_core/test_orchestrator_config_error.py b/tests/unit/pyats_core/test_orchestrator_config_error.py index 397fe230..a27e4d0d 100644 --- a/tests/unit/pyats_core/test_orchestrator_config_error.py +++ b/tests/unit/pyats_core/test_orchestrator_config_error.py @@ -11,8 +11,7 @@ from nac_test.core.constants import ENV_CONTROLLER_CONTEXT from nac_test.pyats_core.orchestrator import PyATSOrchestrator - -from ..conftest import PyATSTestDirs +from tests.conftest import PyATSTestDirs class TestOrchestratorSubprocessRunnerInitError: From b9a2d58fb077376aa86cf8c2a07defd7efbfc005 Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 21:54:53 +0200 Subject: [PATCH 6/7] fix: harden merged-data-model JSON serialization and unify reader json.dump now uses default=str so values ruamel's safe loader yields that JSON cannot natively encode (e.g. datetime.date from an unquoted YAML date) are stringified instead of raising TypeError and aborting the run at merge time. device_inventory discovery now reads the merged model with json.load instead of yaml.safe_load, matching base_test and making the JSON format contract explicit (it previously worked only because JSON is a YAML subset, via the slow ruamel path this migration set out to eliminate). Document the two JSON serialization differences versus the previous YAML format in the CHANGELOG breaking-change entry: non-string mapping keys (e.g. integer VLAN IDs used as keys) are coerced to strings, and non-JSON-native values are written in string form. Add unit tests covering both behaviors. --- CHANGELOG.md | 2 +- nac_test/data_merger.py | 5 +++- .../pyats_core/discovery/device_inventory.py | 5 ++-- tests/unit/test_data_merger.py | 23 +++++++++++++++++++ 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c930df95..02ce1aa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ ## Breaking Changes -- **Internal merged data model file format changed to JSON**: For performance reasons, the internal temporary file used to pass the merged data model to test subprocesses is now written as JSON (`merged_data_model_test_variables.json`, previously `.yaml`). This has no effect on your YAML data files or data model structure. The standard `self.data_model` API is unaffected. This is only breaking if your tests or scripts read the file directly via the `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` environment variable — in that case, switch from YAML parsing to `json.load()`. +- **Internal merged data model file format changed to JSON**: For performance reasons, the internal temporary file used to pass the merged data model to test subprocesses is now written as JSON (`merged_data_model_test_variables.json`, previously `.yaml`). This has no effect on your YAML data files or data model structure. The standard `self.data_model` API is unaffected. This is only breaking if your tests or scripts read the file directly via the `MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH` environment variable — in that case, switch from YAML parsing to `json.load()`. Note two JSON serialization differences versus the previous YAML format: non-string mapping keys (e.g. integer keys such as VLAN IDs used as keys) are coerced to strings, and values that JSON cannot represent natively (e.g. unquoted YAML dates parsed as `datetime.date`) are written as their string form. Quote such keys/values in your data files if you need them preserved as strings anyway. # 2.0.0 diff --git a/nac_test/data_merger.py b/nac_test/data_merger.py index 4dc010a6..e7ebf6a3 100644 --- a/nac_test/data_merger.py +++ b/nac_test/data_merger.py @@ -82,7 +82,10 @@ def write_merged_data_model( full_output_path = DataMerger.merged_data_path(output_directory) logger.info("Writing merged data model to %s", full_output_path) with open(full_output_path, "w", encoding="utf-8") as f: - json.dump(data, f) + # default=str prevents a hard crash on values ruamel's safe loader + # produces that json cannot natively serialize (e.g. datetime.date + # from an unquoted YAML date); such values are stringified. + json.dump(data, f, default=str) if not IS_WINDOWS: os.chmod(full_output_path, MERGED_DATA_FILE_MODE) diff --git a/nac_test/pyats_core/discovery/device_inventory.py b/nac_test/pyats_core/discovery/device_inventory.py index 2e411010..1264c404 100644 --- a/nac_test/pyats_core/discovery/device_inventory.py +++ b/nac_test/pyats_core/discovery/device_inventory.py @@ -8,13 +8,12 @@ """ import importlib.util +import json import logging import sys from pathlib import Path from typing import Any -from nac_test.utils.yaml import safe_load - logger = logging.getLogger(__name__) @@ -80,7 +79,7 @@ def get_device_inventory(self, test_files: list[Path]) -> list[dict[str, Any]]: return [] with open(self.merged_data_filepath, encoding="utf-8") as f: - data_model = safe_load(f) + data_model = json.load(f) # Import the first D2D test file - all D2D tests in an architecture share the same SSH base class # For SD-WAN: all tests under /d2d/ inherit from SDWANTestBase diff --git a/tests/unit/test_data_merger.py b/tests/unit/test_data_merger.py index 3ef13e2d..a55ee2c2 100644 --- a/tests/unit/test_data_merger.py +++ b/tests/unit/test_data_merger.py @@ -8,6 +8,7 @@ - write_merged_data_model: output filename, JSON roundtrip, YAML content parity """ +import datetime import json from pathlib import Path @@ -83,6 +84,28 @@ def test_yaml_content_matches_json_when_dumped(self, tmp_path: Path) -> None: f"YAML and JSON content should match.\nJSON: {json_data}\nYAML: {yaml_data}" ) + def test_non_json_native_values_do_not_crash(self, tmp_path: Path) -> None: + """Values ruamel's safe loader yields that JSON cannot natively encode + (e.g. datetime.date from an unquoted YAML date) are stringified rather + than raising TypeError, so a run is never aborted at merge time. + """ + data = {"cert_valid_until": datetime.date(2025, 1, 15)} + output_path = DataMerger.write_merged_data_model(data, tmp_path) + with open(output_path, encoding="utf-8") as f: + reloaded = json.load(f) + assert reloaded["cert_valid_until"] == "2025-01-15" + + def test_int_mapping_keys_are_coerced_to_strings(self, tmp_path: Path) -> None: + """JSON has no non-string keys: integer mapping keys (e.g. VLAN IDs used + as keys) are coerced to strings on write. This documents the known, + breaking-change behavior versus the previous YAML format. + """ + data = {"vlans": {100: "prod", 200: "stg"}} + output_path = DataMerger.write_merged_data_model(data, tmp_path) + with open(output_path, encoding="utf-8") as f: + reloaded = json.load(f) + assert reloaded["vlans"] == {"100": "prod", "200": "stg"} + def _assert_no_ruamel_types(value: object, path: str = "root") -> None: """Recursively assert no CommentedMap/CommentedSeq anywhere in the tree.""" From 498843ba52c65e3741f7ad4b7449038fc4e0244c Mon Sep 17 00:00:00 2001 From: Oliver Boehmer Date: Wed, 2 Sep 2026 21:59:12 +0200 Subject: [PATCH 7/7] remove redundant module docstring comment --- tests/pyats_core/conftest.py | 3 --- tests/unit/conftest.py | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/pyats_core/conftest.py b/tests/pyats_core/conftest.py index 188eb359..36cc1967 100644 --- a/tests/pyats_core/conftest.py +++ b/tests/pyats_core/conftest.py @@ -6,9 +6,6 @@ NOTE: This module intentionally duplicates some patterns from tests/unit/conftest.py. Issue #541 will merge tests/pyats_core/ into tests/unit/, at which point these fixtures should be consolidated into a single conftest.py. - -The PyATSTestDirs type and the pyats_test_dirs fixture live in the top-level -tests/conftest.py so they are shared across the whole test suite. """ import pytest diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index f1e489e9..8e48b93b 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,11 +1,7 @@ # SPDX-License-Identifier: MPL-2.0 # Copyright (c) 2025 Daniel Schmidt -"""Shared fixtures for unit tests. - -NOTE: The PyATSTestDirs type and the pyats_test_dirs fixture live in the -top-level tests/conftest.py so they are shared across the whole test suite. -""" +"""Shared fixtures for unit tests.""" from typing import Any from unittest.mock import AsyncMock, Mock