feat(pyats): add architecture-agnostic defaults resolution to NACTestBase#551
Merged
aitestino merged 33 commits intorelease/pyats-integration-v1.1-betafrom Mar 19, 2026
Conversation
aitestino
added
enhancement
Create unit test directory structure for pyats_core components to organize comprehensive test coverage for core framework utilities. Part of defaults resolution infrastructure.
Add pure utility module for reading default values from merged NAC data models:
- ensure_defaults_block_exists(): Validates defaults block presence with
clear error messages
- get_default_value(): Single-path and cascade lookup with required/optional
support, using JMESPath for data model traversal
Architecture-agnostic design - all parameters (prefix, error message) passed
by caller. No PyATS dependencies. Supports all NAC architectures (ACI, SD-WAN,
Catalyst Center) through configuration.
Features:
- Cascade/fallback support across multiple JMESPaths
- Handles falsy values correctly (False, 0, "", [], {})
- Comprehensive docstrings with usage examples
- Type-safe with Python 3.10+ annotations
Add defaults resolution capability to NACTestBase, making it available to all architecture test classes through opt-in configuration: Class Attributes (subclasses configure): - DEFAULTS_PREFIX: Architecture-specific path (e.g., "defaults.apic") - DEFAULTS_MISSING_ERROR: Custom error message for missing defaults Instance Method: - get_default_value(*paths, required=True): Wrapper that delegates to defaults_resolver utilities, providing class-level configuration Opt-in architecture: - DEFAULTS_PREFIX defaults to None - Raises NotImplementedError if called without configuration - Architectures enable by setting DEFAULTS_PREFIX class attribute Also includes minor cleanup: - Remove obsolete type: ignore comments from markdown/yaml imports - Update decorator type ignore comments to use 'misc' category This enables test scripts to access default values from data model with same cascade/fallback behavior as Jinja2 templates.
Add 51 unit tests (864 lines) for defaults_resolver utility functions,
organized into focused test classes:
TestEnsureDefaultsBlockExists (7 tests):
- Valid defaults block detection
- Missing defaults block error handling
- Architecture-specific prefix validation
TestGetDefaultValueSinglePath (13 tests):
- Single-path lookups with nested paths
- Falsy value handling (False, 0, "", [], {})
- Required vs optional behavior
- Return type verification (str, int, dict, list)
TestGetDefaultValueCascade (8 tests):
- Multi-path fallback behavior
- First-found-wins semantics
- All-missing scenarios (required/optional)
TestGetDefaultValueErrorHandling (5 tests):
- Missing paths TypeError
- Custom error message propagation
- Detailed error messages with attempted paths
TestArchitectureAgnostic (7 tests):
- APIC, SD-WAN, Catalyst Center prefix support
- Custom architecture prefixes
- Architecture-specific error messages
TestEdgeCases (11 tests):
- Deep nesting (5+ levels)
- Special characters in keys
- Large nested structures
- Explicit None values
- Data model immutability verification
Test execution: All 51 tests pass in 0.06s
Add 12 integration tests (364 lines) for NACTestBase defaults resolution wrapper, verifying correct delegation to defaults_resolver utilities: Test Coverage: - DEFAULTS_PREFIX=None raises NotImplementedError with clear message - DEFAULTS_PREFIX configured enables functionality - Custom error messages propagate correctly - Subclass override behavior (APIC vs SD-WAN vs CatC) - Cascade/fallback behavior through base class method - Optional (required=False) lookup returns None correctly - Deeply nested path lookups - Required=True raises ValueError for missing values - Falsy value handling (False, 0, "") through wrapper - Dict value returns Testing Strategy: - Minimal testable class mimics NACTestBase behavior - PyATS mocked via fixture to avoid import dependencies - Architecture-specific data models (APIC, SD-WAN) as fixtures - Verifies wrapper provides proper class-level configuration Test execution: All 12 tests pass in 0.08s Combined with defaults_resolver tests: 63 total tests in 0.14s
Remove type: ignore comments that are no longer needed due to updated type stubs for importlib.metadata, yaml, and aiofiles packages.
Replace type: ignore[no-any-return] with explicit str() cast for Jinja2 template.render() return values, improving type safety.
Remove type: ignore comments from Jinja2 and Robot Framework imports, no longer needed with updated type stubs.
Remove type: ignore comments that are no longer needed with updated type stubs for importlib.resources and test fixtures.
Move the defaults_resolver import to the module's top-level import section. This prepares for the next commit which removes the lazy import from inside the get_default_value() method body. The lazy import was originally added as a precaution against circular imports, but defaults_resolver.py is a pure utility module with no PyATS dependencies and no imports back into base_test, so there is no circular import risk. Top-level imports are preferred because they make dependencies explicit, fail fast at import time rather than at call time, and are consistent with the rest of this module's imports.
Extract the apic_data_model and sdwan_data_model fixtures into a shared conftest.py file at tests/unit/pyats_core/common/. These two fixtures were duplicated identically across both test_defaults_resolver.py and test_base_test_defaults.py. Centralizing them in conftest.py follows pytest's fixture sharing convention and ensures a single source of truth for the sample data models used across all defaults-related tests. Both test files will have their local copies removed in subsequent commits.
Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py added in the previous commit. The remaining fixtures (catc_data_model, data_model_with_falsy_values, deeply_nested_data_model, empty_data_model, partial_data_model) are specific to this file's test scenarios and stay here. Updated the section comment to note where the shared fixtures live, so future contributors know not to re-add them.
Add test_malformed_jmespath_expression_propagates to verify that invalid JMESPath syntax (e.g., "[invalid") raises a jmespath ParseError that propagates directly to the caller. This behavior is by design: malformed path expressions indicate a programming error in the caller (wrong syntax), not a missing default value. Letting the ParseError propagate gives developers an immediate, clear traceback pointing at the malformed expression rather than a misleading "value not found" error that would waste debugging time. This test documents and locks in the current error propagation contract so that future refactors don't accidentally swallow these errors with a bare except or a catch-all handler.
Add test_empty_string_path_raises_jmespath_error to document and verify the behavior when an empty string "" is passed as a default_path argument. When the path is empty, get_default_value constructs the full JMESPath expression as "defaults.apic." (with a trailing dot), which is syntactically invalid JMESPath. This correctly raises a ParseError rather than silently returning None or the entire defaults block. This is the desired behavior because an empty path is almost certainly a bug at the call site. A hard error forces the developer to fix the path rather than silently getting unexpected results.
Remove the temp_data_model_file fixture from test_base_test_defaults.py along with its associated imports (json, os, tempfile). This fixture was never referenced by any test in the file — it appears to have been scaffolded during initial development but was never wired into any test case. Removing dead code keeps the test file focused and avoids confusing future contributors who might wonder what tests use it or whether removing it would break something.
Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py at the same directory level. This completes the fixture deduplication across both defaults test files. Both test_defaults_resolver.py and test_base_test_defaults.py now consume the same fixture instances from conftest.py, ensuring the sample data models stay consistent if they ever need to change.
Replace the TestableNACTestBase stand-in class with tests that exercise the real NACTestBase.get_default_value() method. This is a significant improvement to test quality. Problem with the previous approach: The old tests created a TestableNACTestBase inner class that manually reimplemented the get_default_value() logic (DEFAULTS_PREFIX guard, delegation to defaults_resolver, argument threading). This meant the tests were verifying the stand-in's behavior, not the actual production code. If someone changed NACTestBase.get_default_value() in base_test.py, these tests would still pass because they never exercised the real method. New approach — delegation contract tests: The rewritten tests import and instantiate the real NACTestBase class (with PyATS mocked out via sys.modules patching) and verify: 1. DEFAULTS_PREFIX=None guard: Calling get_default_value() on a class with no DEFAULTS_PREFIX raises NotImplementedError with the concrete class name in the message. 2. Delegation contract: When DEFAULTS_PREFIX is set, the method delegates to defaults_resolver.get_default_value (_resolve) with the correct positional args (data_model, *paths) and keyword args (defaults_prefix, missing_error, required). 3. Class attribute threading: DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR are correctly passed through to the resolver. The default error message is used when DEFAULTS_MISSING_ERROR is not overridden. 4. Subclass overrides: Architecture subclasses (e.g., SDWANTestBase) can override both class attributes and the correct values are threaded through. Why delegation tests are sufficient: The cascade behavior, falsy value handling, error messages, and edge cases are all thoroughly tested in test_defaults_resolver.py (51 tests). Since get_default_value() on NACTestBase is a thin wrapper that delegates to the same function, re-testing all those scenarios here would be redundant. These 7 tests focus exclusively on what the wrapper adds: the opt-in guard and argument threading. PyATS mock hierarchy: The mock_pyats fixture now covers the full import chain that NACTestBase requires: - pyats (top-level package) - pyats.aetest (Testcase base class, setup decorator) - pyats.aetest.steps (imported by step_interceptor) - pyats.aetest.steps.implementation (Step class) The mocks are wired hierarchically so that both `from pyats import aetest` and `import pyats.aetest` resolve to the same mock object.
aitestino
force-pushed
the
feature/defaults-resolution-in-base-class-v2
branch
from
5aafa97 to
50d22d0
Compare
The Any | None type hint syntax fails in Python 3.10 because the typing module's Any type doesn't support the | operator without postponed evaluation of annotations. Add `from __future__ import annotations` to enable postponed annotation evaluation, which allows the modern union syntax to work across all supported Python versions (3.10+). This resolves the test failures on Python 3.10 while maintaining compatibility with 3.11, 3.12, and 3.13.
The SSHTestBase unit tests were failing because NACTestBase.setup() requires controller environment variables to detect the controller type. These tests were mocking NACTestBase.setup() but the mock wasn't applied before the setup method was called, causing controller detection to fail. Add IOSXE controller credentials to the test environment setup for all three SSH validation tests to satisfy the controller detection requirement. Fixes test failures: - test_validation_called_for_valid_device - test_validation_fails_for_missing_fields - test_validation_not_called_for_json_parse_error
Add cleanup of controller environment variables (IOSXE_URL, IOSXE_USERNAME, IOSXE_PASSWORD) in the finally blocks of all SSH validation tests. Without cleanup, these environment variables persist across test runs, causing "Multiple controller credentials detected" errors in subsequent tests that set different controller credentials (ACI, SDWAN, etc.). This was causing 4 test failures and 3 errors in the test suite: - test_end_to_end_controller_detection (SDWAN + IOSXE detected) - test_controller_switch_scenario (ACI + IOSXE detected) - test_falls_back_to_cwd_when_data_file_missing (ACI + IOSXE detected) - Several orchestrator tests (exit code 255)
oboehmer
reviewed
Mar 5, 2026
Collaborator
oboehmer
left a comment
oboehmer
left a comment
There was a problem hiding this comment.
Thanks for this.. I made a few minor observations, main concern is about performance penalty of repeated ensure_defaults_block_exists calls, I feel our existing nac-test default block checks should (and partially already do) catch this early?
…lts resolution This commit adds automatic controller type detection for defaults resolution, eliminating the need for per-architecture configuration in test base classes. Key Changes: - Added CONTROLLER_TO_DEFAULTS_PREFIX mapping in controller.py that maps controller types (ACI, SDWAN, CC, etc.) to their defaults block prefixes (defaults.apic, defaults.sdwan, defaults.catc, etc.) - Rewrote NACTestBase.get_default_value() to auto-detect controller type from environment variables and look up the appropriate defaults prefix - Removed need for DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR class attributes - Optimized defaults_resolver.py by removing redundant validation calls (CLI validators from PR #525 handle pre-flight validation) - Updated test fixtures to include iosxe_controller_env for consistency - Updated 6 unit tests to match new behavior (no redundant validation) Architecture Mapping: ACI (ACI_URL) → defaults.apic SD-WAN (SDWAN_URL) → defaults.sdwan Catalyst Center (CC_URL) → defaults.catc IOS-XE (IOSXE_URL) → defaults.iosxe Meraki (MERAKI_URL) → defaults.meraki FMC (FMC_URL) → defaults.fmc ISE (ISE_URL) → defaults.ise Performance Impact: Eliminated 600+ redundant JMESPath searches per test run by removing ensure_defaults_block_exists() calls from get_default_value(). CLI validators handle this check once before test execution begins. Co-Authored-By: Oliver Frolovs <noreply@example.com>
…ution-in-base-class-v2 Brings in critical upstream improvements from v2.0.0a1 release: - Fail-fast authentication validation (#531) - Dry-run support for PyATS (#554) - Clean up stale test artifacts (#526, #571) - Improved CLI with --verbose and --loglevel flags (#599) - Better test isolation for parallel execution (#569) - GitHub templates for issues and PRs (#510) - Multiple dependency updates and bug fixes These changes provide the test infrastructure improvements needed for the defaults resolution feature, particularly the controller auth validation that addresses test fixture concerns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces manual DEFAULTS_PREFIX configuration tests with auto-detection tests that verify controller type detection from environment variables. Changes: - Tests now verify auto-detection of ACI, SD-WAN, CC, and IOS-XE controllers - Added controller environment fixtures for all supported architectures - Tests verify correct defaults prefix mapping (ACI→apic, SDWAN→sdwan, etc.) - Tests verify graceful error handling when no controller credentials found - Removed obsolete NotImplementedError tests for manual configuration The new tests align with the auto-detection design where the framework automatically determines the defaults prefix based on detected controller type (ACI_URL → defaults.apic, SDWAN_URL → defaults.sdwan, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
IOSXE is unique among supported controllers - it only requires IOSXE_URL because D2D tests use device-specific credentials from the device inventory, not controller credentials. Before this fix, setup() unconditionally tried to read USERNAME/PASSWORD from environment variables, causing KeyError for valid IOSXE configurations. Changes: - Changed os.environ[...] to os.environ.get(...) for USERNAME/PASSWORD - Added comprehensive tests for IOSXE optional credentials - Tests verify both scenarios: * IOSXE with only URL (no username/password) - now works * IOSXE with URL + username/password - still works * ACI with incomplete credentials - fails at detection (correct behavior) The fix allows IOSXE D2D tests to run with only IOSXE_URL set, while still supporting controller-based architectures (ACI, SD-WAN, CC) that require all three credentials. Fixes code review issue #1 (BLOCKING). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit addresses all remaining code review findings, implementing comprehensive fixes for code quality, performance, type safety, and test coverage. Fixes Implemented: Issue #3: Deleted dead class attributes - Removed unused DEFAULTS_PREFIX class attribute - Removed unused DEFAULTS_MISSING_ERROR class attribute - These were leftover from refactoring and never used Issue #4: Eliminated redundant controller type detection - Changed get_default_value() to use cached self.controller_type - Previously called detect_controller_type() on every invocation (21 env lookups) - Now uses value set once in setup(), dramatically improving performance - Updated test_base_test_defaults.py to manually set controller_type to simulate setup() Issue #5: Removed duplicate imports - Removed detect_controller_type from get_default_value() imports - Only CONTROLLER_TO_DEFAULTS_PREFIX needed now that we use cached value Issue #6: Removed unused function parameter - Deleted partial_credentials parameter from _format_no_credentials_error() - Parameter was never read, function works without it - Updated call site at line 162 Issue #7: Removed unreachable None check - Changed from CONTROLLER_TO_DEFAULTS_PREFIX.get() to direct subscript - None check was unreachable because all ControllerTypeKey values exist in dict - Direct subscript is more correct and eliminates dead code branch Issue #8: Replaced chr(10) workaround with '\n' - Changed chr(10) to '\n' literal in _format_multiple_credentials_error() - No need for ASCII code workaround in modern Python - More readable and idiomatic Issue #9: Deleted commented-out code - Removed 33 lines of commented __init_subclass__ implementation - Code was kept "for reference" but never needed - Clean codebase removes commented code Issue #10: Removed redundant .upper() call - Removed controller_type.upper() in get_default_value() - controller_type is already uppercase (from ControllerTypeKey Literal) - Redundant string operation eliminated Issue #11: Type safety already implemented - Confirmed ControllerTypeKey Literal already exists in nac_test/core/types.py - No changes needed, type safety already correct Issue #12: Added TypedDict for structured return type - Created CredentialSetStatus TypedDict in controller.py - Replaced untyped dict[str, Any] with proper TypedDict structure - Updated _find_credential_sets() return type annotation - Provides better type safety and IDE support IOSXE Optional Credentials Test Coverage: Added comprehensive test file test_base_test_iosxe_credentials.py to verify IOSXE controller's unique optional credentials behavior: - test_iosxe_setup_works_without_username_password: Verifies real-world scenario where only IOSXE_URL is set (D2D tests use device-specific credentials) - test_iosxe_setup_works_with_username_password: Verifies optional credentials are accepted if provided - test_aci_setup_requires_username_password: Verifies normal 3-credential pattern still works for controller-based architectures - test_aci_setup_fails_without_username: Verifies incomplete credentials are caught by detect_controller_type() before setup() reads them Added controller environment fixtures to tests/unit/conftest.py: - aci_controller_env: Sets ACI_URL, ACI_USERNAME, ACI_PASSWORD - sdwan_controller_env: Sets SDWAN_URL, SDWAN_USERNAME, SDWAN_PASSWORD - cc_controller_env: Sets CC_URL, CC_USERNAME, CC_PASSWORD These fixtures provide consistent test data and replace manual monkeypatch setup in individual tests. Test Updates: Updated test_base_test_defaults.py to work with performance optimization: - All test methods now manually set instance.controller_type before calling get_default_value() to simulate what setup() does - Changed test_no_controller_credentials_raises to expect AttributeError when controller_type not set (instead of ValueError from detection) - Updated docstrings to reflect that tests verify cached controller_type usage rather than auto-detection behavior All tests pass (618 tests in unit suite).
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
- Extend ControllerConfig with defaults_prefix field for JMESPath prefix mapping - Add get_defaults_prefix() helper function for controller-to-prefix lookup - Configure defaults_prefix for all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Enable automatic defaults resolution without per-architecture configuration This consolidates controller configuration metadata into a single source of truth, eliminating the need for separate mapping dictionaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unused missing_error parameter from function signature - Function generates its own clear, detailed error messages - Keep missing_error in ensure_defaults_block_exists() where it is used - Simplify function interface by removing unused parameter The parameter was accepted but never referenced in the function body. CLI validators perform pre-flight validation, so get_default_value() only needs to report which specific paths were not found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unused get_display_name import - Remove generation of unused missing_error parameter - Call _resolve() with simplified parameter list - Leverage controller type detection from setup() The function now delegates cleanly to defaults_resolver without constructing parameters that are never used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract common PyATS mock fixtures from individual test files - Provide nac_test_base_class fixture for consistent test setup - Eliminate duplicated mocking code across test modules - Enable proper module isolation in PyATS-dependent tests This shared infrastructure prevents import errors and ensures consistent PyATS mocking across all pyats_core common tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 18 integration tests verifying controller-to-prefix mappings - Test all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Verify end-to-end flow from detection to defaults resolution - Test error propagation for missing defaults and invalid paths - Validate unique defaults_prefix for each controller type These tests ensure the complete integration between controller detection and defaults resolution works correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove assertions checking for missing_error in get_default_value() calls - Update test expectations to match simplified function interface - Add missing_error to ensure_defaults_block_exists() calls - Remove duplicated PyATS mocking code (now in conftest.py) Tests now correctly validate the updated function signatures where get_default_value() does not accept missing_error parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aitestino
force-pushed
the
feature/defaults-resolution-in-base-class-v2
branch
from
f6df1e1 to
3baacc2
Compare
Python 3.10+ natively supports PEP 604 (str | None) and PEP 585 (dict[str, Any]) without requiring 'from __future__ import annotations'. The import was cargo-culted from other files but serves no purpose in base_test.py and defaults_resolver.py: - No forward references (self-referencing class types) - No circular import issues - All type hints use native 3.10+ syntax Verified by running full test suite: 711 tests pass (636 unit + 75 integration).
oboehmer
approved these changes
Mar 12, 2026
Collaborator
oboehmer
left a comment
oboehmer
left a comment
There was a problem hiding this comment.
thanks for addressing my performance concern during my earlier review.. production code looks great with only a few comments. Some tests could be optimized (consistent use of monkeypatch and tmp_path fixtures to avoid the cleanup, and also pytest.mark.parametrize options to test the resolver code.
I am still approving, but please give Claude another go at those :)
| # Note: Environment validation happens in orchestrator pre-flight check | ||
| # Detect controller type based on environment variables | ||
| try: | ||
| self.controller_type = detect_controller_type() |
Collaborator
There was a problem hiding this comment.
I know this is not related to this PR, but when looking at this: detecting the same controller type by every tests looks architecturally redundant as the value is constant for the entire run.. I know the performance impact here is negligible, but maybe we can instead inject its value into the execution differently? LIke having JobGenerator injecting it as runtime.controller_type = "..." in generated job files, and test scripts can access it via from pyats.easypy import runtime; runtime.controller_type. This looks like a cleaner pattern, and could also be leveraged for other vars like MERGED_DATA_MODEL_TEST_VARIABLES_FILEPATH.
I raised #637 to track it..
…ents - Rename get_default_value() to resolve_default_value() in defaults_resolver.py to avoid confusing private alias in base_test.py - Add IOSXE_HOST as alternative URL env var alongside IOSXE_URL via new alt_url_env_vars field on ControllerConfig and get_controller_url() - Parametrize repetitive test clusters in test_defaults_resolver.py (falsy values, architecture prefixes, error messages) - Replace tempfile.NamedTemporaryFile + manual cleanup with tmp_path and monkeypatch in test_base_test_iosxe_credentials.py and test_ssh_base_test_validation.py - Clean up docstrings in test_controller_defaults_integration.py
aitestino
merged commit a263781
into
release/pyats-integration-v1.1-beta
7 checks passed
oboehmer
added a commit
that referenced
this pull request
Mar 19, 2026
…Base (#551) * test(structure): add pyats_core unit test directory structure Create unit test directory structure for pyats_core components to organize comprehensive test coverage for core framework utilities. Part of defaults resolution infrastructure. * feat(pyats): add architecture-agnostic defaults resolution utilities Add pure utility module for reading default values from merged NAC data models: - ensure_defaults_block_exists(): Validates defaults block presence with clear error messages - get_default_value(): Single-path and cascade lookup with required/optional support, using JMESPath for data model traversal Architecture-agnostic design - all parameters (prefix, error message) passed by caller. No PyATS dependencies. Supports all NAC architectures (ACI, SD-WAN, Catalyst Center) through configuration. Features: - Cascade/fallback support across multiple JMESPaths - Handles falsy values correctly (False, 0, "", [], {}) - Comprehensive docstrings with usage examples - Type-safe with Python 3.10+ annotations * feat(pyats): add get_default_value() method to NACTestBase Add defaults resolution capability to NACTestBase, making it available to all architecture test classes through opt-in configuration: Class Attributes (subclasses configure): - DEFAULTS_PREFIX: Architecture-specific path (e.g., "defaults.apic") - DEFAULTS_MISSING_ERROR: Custom error message for missing defaults Instance Method: - get_default_value(*paths, required=True): Wrapper that delegates to defaults_resolver utilities, providing class-level configuration Opt-in architecture: - DEFAULTS_PREFIX defaults to None - Raises NotImplementedError if called without configuration - Architectures enable by setting DEFAULTS_PREFIX class attribute Also includes minor cleanup: - Remove obsolete type: ignore comments from markdown/yaml imports - Update decorator type ignore comments to use 'misc' category This enables test scripts to access default values from data model with same cascade/fallback behavior as Jinja2 templates. * test(pyats): add comprehensive unit tests for defaults_resolver module Add 51 unit tests (864 lines) for defaults_resolver utility functions, organized into focused test classes: TestEnsureDefaultsBlockExists (7 tests): - Valid defaults block detection - Missing defaults block error handling - Architecture-specific prefix validation TestGetDefaultValueSinglePath (13 tests): - Single-path lookups with nested paths - Falsy value handling (False, 0, "", [], {}) - Required vs optional behavior - Return type verification (str, int, dict, list) TestGetDefaultValueCascade (8 tests): - Multi-path fallback behavior - First-found-wins semantics - All-missing scenarios (required/optional) TestGetDefaultValueErrorHandling (5 tests): - Missing paths TypeError - Custom error message propagation - Detailed error messages with attempted paths TestArchitectureAgnostic (7 tests): - APIC, SD-WAN, Catalyst Center prefix support - Custom architecture prefixes - Architecture-specific error messages TestEdgeCases (11 tests): - Deep nesting (5+ levels) - Special characters in keys - Large nested structures - Explicit None values - Data model immutability verification Test execution: All 51 tests pass in 0.06s * test(pyats): add integration tests for NACTestBase.get_default_value() Add 12 integration tests (364 lines) for NACTestBase defaults resolution wrapper, verifying correct delegation to defaults_resolver utilities: Test Coverage: - DEFAULTS_PREFIX=None raises NotImplementedError with clear message - DEFAULTS_PREFIX configured enables functionality - Custom error messages propagate correctly - Subclass override behavior (APIC vs SD-WAN vs CatC) - Cascade/fallback behavior through base class method - Optional (required=False) lookup returns None correctly - Deeply nested path lookups - Required=True raises ValueError for missing values - Falsy value handling (False, 0, "") through wrapper - Dict value returns Testing Strategy: - Minimal testable class mimics NACTestBase behavior - PyATS mocked via fixture to avoid import dependencies - Architecture-specific data models (APIC, SD-WAN) as fixtures - Verifies wrapper provides proper class-level configuration Test execution: All 12 tests pass in 0.08s Combined with defaults_resolver tests: 63 total tests in 0.14s * refactor: remove obsolete type ignore comments from imports Remove type: ignore comments that are no longer needed due to updated type stubs for importlib.metadata, yaml, and aiofiles packages. * refactor(reporting): improve type safety in template rendering Replace type: ignore[no-any-return] with explicit str() cast for Jinja2 template.render() return values, improving type safety. * refactor(robot): remove obsolete type ignore comments Remove type: ignore comments from Jinja2 and Robot Framework imports, no longer needed with updated type stubs. * test: remove obsolete type ignore comments from test files Remove type: ignore comments that are no longer needed with updated type stubs for importlib.resources and test fixtures. * refactor(pyats): add top-level import for defaults_resolver in base_test Move the defaults_resolver import to the module's top-level import section. This prepares for the next commit which removes the lazy import from inside the get_default_value() method body. The lazy import was originally added as a precaution against circular imports, but defaults_resolver.py is a pure utility module with no PyATS dependencies and no imports back into base_test, so there is no circular import risk. Top-level imports are preferred because they make dependencies explicit, fail fast at import time rather than at call time, and are consistent with the rest of this module's imports. * test(pyats): add conftest.py with shared defaults test fixtures Extract the apic_data_model and sdwan_data_model fixtures into a shared conftest.py file at tests/unit/pyats_core/common/. These two fixtures were duplicated identically across both test_defaults_resolver.py and test_base_test_defaults.py. Centralizing them in conftest.py follows pytest's fixture sharing convention and ensures a single source of truth for the sample data models used across all defaults-related tests. Both test files will have their local copies removed in subsequent commits. * test(pyats): remove duplicate fixtures from test_defaults_resolver Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py added in the previous commit. The remaining fixtures (catc_data_model, data_model_with_falsy_values, deeply_nested_data_model, empty_data_model, partial_data_model) are specific to this file's test scenarios and stay here. Updated the section comment to note where the shared fixtures live, so future contributors know not to re-add them. * test(pyats): add test for malformed JMESPath expression propagation Add test_malformed_jmespath_expression_propagates to verify that invalid JMESPath syntax (e.g., "[invalid") raises a jmespath ParseError that propagates directly to the caller. This behavior is by design: malformed path expressions indicate a programming error in the caller (wrong syntax), not a missing default value. Letting the ParseError propagate gives developers an immediate, clear traceback pointing at the malformed expression rather than a misleading "value not found" error that would waste debugging time. This test documents and locks in the current error propagation contract so that future refactors don't accidentally swallow these errors with a bare except or a catch-all handler. * test(pyats): add test for empty string path JMESPath behavior Add test_empty_string_path_raises_jmespath_error to document and verify the behavior when an empty string "" is passed as a default_path argument. When the path is empty, get_default_value constructs the full JMESPath expression as "defaults.apic." (with a trailing dot), which is syntactically invalid JMESPath. This correctly raises a ParseError rather than silently returning None or the entire defaults block. This is the desired behavior because an empty path is almost certainly a bug at the call site. A hard error forces the developer to fix the path rather than silently getting unexpected results. * test(pyats): remove unused temp_data_model_file fixture and imports Remove the temp_data_model_file fixture from test_base_test_defaults.py along with its associated imports (json, os, tempfile). This fixture was never referenced by any test in the file — it appears to have been scaffolded during initial development but was never wired into any test case. Removing dead code keeps the test file focused and avoids confusing future contributors who might wonder what tests use it or whether removing it would break something. * test(pyats): remove duplicate fixtures from test_base_test_defaults Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py at the same directory level. This completes the fixture deduplication across both defaults test files. Both test_defaults_resolver.py and test_base_test_defaults.py now consume the same fixture instances from conftest.py, ensuring the sample data models stay consistent if they ever need to change. * test(pyats): rewrite base_test_defaults to test real NACTestBase class Replace the TestableNACTestBase stand-in class with tests that exercise the real NACTestBase.get_default_value() method. This is a significant improvement to test quality. Problem with the previous approach: The old tests created a TestableNACTestBase inner class that manually reimplemented the get_default_value() logic (DEFAULTS_PREFIX guard, delegation to defaults_resolver, argument threading). This meant the tests were verifying the stand-in's behavior, not the actual production code. If someone changed NACTestBase.get_default_value() in base_test.py, these tests would still pass because they never exercised the real method. New approach — delegation contract tests: The rewritten tests import and instantiate the real NACTestBase class (with PyATS mocked out via sys.modules patching) and verify: 1. DEFAULTS_PREFIX=None guard: Calling get_default_value() on a class with no DEFAULTS_PREFIX raises NotImplementedError with the concrete class name in the message. 2. Delegation contract: When DEFAULTS_PREFIX is set, the method delegates to defaults_resolver.get_default_value (_resolve) with the correct positional args (data_model, *paths) and keyword args (defaults_prefix, missing_error, required). 3. Class attribute threading: DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR are correctly passed through to the resolver. The default error message is used when DEFAULTS_MISSING_ERROR is not overridden. 4. Subclass overrides: Architecture subclasses (e.g., SDWANTestBase) can override both class attributes and the correct values are threaded through. Why delegation tests are sufficient: The cascade behavior, falsy value handling, error messages, and edge cases are all thoroughly tested in test_defaults_resolver.py (51 tests). Since get_default_value() on NACTestBase is a thin wrapper that delegates to the same function, re-testing all those scenarios here would be redundant. These 7 tests focus exclusively on what the wrapper adds: the opt-in guard and argument threading. PyATS mock hierarchy: The mock_pyats fixture now covers the full import chain that NACTestBase requires: - pyats (top-level package) - pyats.aetest (Testcase base class, setup decorator) - pyats.aetest.steps (imported by step_interceptor) - pyats.aetest.steps.implementation (Step class) The mocks are wired hierarchically so that both `from pyats import aetest` and `import pyats.aetest` resolve to the same mock object. * fix(pyats): add future annotations import for Python 3.10 compatibility The Any | None type hint syntax fails in Python 3.10 because the typing module's Any type doesn't support the | operator without postponed evaluation of annotations. Add `from __future__ import annotations` to enable postponed annotation evaluation, which allows the modern union syntax to work across all supported Python versions (3.10+). This resolves the test failures on Python 3.10 while maintaining compatibility with 3.11, 3.12, and 3.13. * fix(tests): add controller credentials to SSH validation unit tests The SSHTestBase unit tests were failing because NACTestBase.setup() requires controller environment variables to detect the controller type. These tests were mocking NACTestBase.setup() but the mock wasn't applied before the setup method was called, causing controller detection to fail. Add IOSXE controller credentials to the test environment setup for all three SSH validation tests to satisfy the controller detection requirement. Fixes test failures: - test_validation_called_for_valid_device - test_validation_fails_for_missing_fields - test_validation_not_called_for_json_parse_error * fix(tests): cleanup environment variables in SSH validation tests Add cleanup of controller environment variables (IOSXE_URL, IOSXE_USERNAME, IOSXE_PASSWORD) in the finally blocks of all SSH validation tests. Without cleanup, these environment variables persist across test runs, causing "Multiple controller credentials detected" errors in subsequent tests that set different controller credentials (ACI, SDWAN, etc.). This was causing 4 test failures and 3 errors in the test suite: - test_end_to_end_controller_detection (SDWAN + IOSXE detected) - test_controller_switch_scenario (ACI + IOSXE detected) - test_falls_back_to_cwd_when_data_file_missing (ACI + IOSXE detected) - Several orchestrator tests (exit code 255) * feat(defaults): implement auto-detection of controller type for defaults resolution This commit adds automatic controller type detection for defaults resolution, eliminating the need for per-architecture configuration in test base classes. Key Changes: - Added CONTROLLER_TO_DEFAULTS_PREFIX mapping in controller.py that maps controller types (ACI, SDWAN, CC, etc.) to their defaults block prefixes (defaults.apic, defaults.sdwan, defaults.catc, etc.) - Rewrote NACTestBase.get_default_value() to auto-detect controller type from environment variables and look up the appropriate defaults prefix - Removed need for DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR class attributes - Optimized defaults_resolver.py by removing redundant validation calls (CLI validators from PR #525 handle pre-flight validation) - Updated test fixtures to include iosxe_controller_env for consistency - Updated 6 unit tests to match new behavior (no redundant validation) Architecture Mapping: ACI (ACI_URL) → defaults.apic SD-WAN (SDWAN_URL) → defaults.sdwan Catalyst Center (CC_URL) → defaults.catc IOS-XE (IOSXE_URL) → defaults.iosxe Meraki (MERAKI_URL) → defaults.meraki FMC (FMC_URL) → defaults.fmc ISE (ISE_URL) → defaults.ise Performance Impact: Eliminated 600+ redundant JMESPath searches per test run by removing ensure_defaults_block_exists() calls from get_default_value(). CLI validators handle this check once before test execution begins. Co-Authored-By: Oliver Frolovs <noreply@example.com> * Rewrite base test defaults tests for auto-detection design Replaces manual DEFAULTS_PREFIX configuration tests with auto-detection tests that verify controller type detection from environment variables. Changes: - Tests now verify auto-detection of ACI, SD-WAN, CC, and IOS-XE controllers - Added controller environment fixtures for all supported architectures - Tests verify correct defaults prefix mapping (ACI→apic, SDWAN→sdwan, etc.) - Tests verify graceful error handling when no controller credentials found - Removed obsolete NotImplementedError tests for manual configuration The new tests align with the auto-detection design where the framework automatically determines the defaults prefix based on detected controller type (ACI_URL → defaults.apic, SDWAN_URL → defaults.sdwan, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): make USERNAME/PASSWORD optional for IOSXE controller IOSXE is unique among supported controllers - it only requires IOSXE_URL because D2D tests use device-specific credentials from the device inventory, not controller credentials. Before this fix, setup() unconditionally tried to read USERNAME/PASSWORD from environment variables, causing KeyError for valid IOSXE configurations. Changes: - Changed os.environ[...] to os.environ.get(...) for USERNAME/PASSWORD - Added comprehensive tests for IOSXE optional credentials - Tests verify both scenarios: * IOSXE with only URL (no username/password) - now works * IOSXE with URL + username/password - still works * ACI with incomplete credentials - fails at detection (correct behavior) The fix allows IOSXE D2D tests to run with only IOSXE_URL set, while still supporting controller-based architectures (ACI, SD-WAN, CC) that require all three credentials. Fixes code review issue #1 (BLOCKING). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Resolve code review issues #3-#12 from PR #551 This commit addresses all remaining code review findings, implementing comprehensive fixes for code quality, performance, type safety, and test coverage. Fixes Implemented: Issue #3: Deleted dead class attributes - Removed unused DEFAULTS_PREFIX class attribute - Removed unused DEFAULTS_MISSING_ERROR class attribute - These were leftover from refactoring and never used Issue #4: Eliminated redundant controller type detection - Changed get_default_value() to use cached self.controller_type - Previously called detect_controller_type() on every invocation (21 env lookups) - Now uses value set once in setup(), dramatically improving performance - Updated test_base_test_defaults.py to manually set controller_type to simulate setup() Issue #5: Removed duplicate imports - Removed detect_controller_type from get_default_value() imports - Only CONTROLLER_TO_DEFAULTS_PREFIX needed now that we use cached value Issue #6: Removed unused function parameter - Deleted partial_credentials parameter from _format_no_credentials_error() - Parameter was never read, function works without it - Updated call site at line 162 Issue #7: Removed unreachable None check - Changed from CONTROLLER_TO_DEFAULTS_PREFIX.get() to direct subscript - None check was unreachable because all ControllerTypeKey values exist in dict - Direct subscript is more correct and eliminates dead code branch Issue #8: Replaced chr(10) workaround with '\n' - Changed chr(10) to '\n' literal in _format_multiple_credentials_error() - No need for ASCII code workaround in modern Python - More readable and idiomatic Issue #9: Deleted commented-out code - Removed 33 lines of commented __init_subclass__ implementation - Code was kept "for reference" but never needed - Clean codebase removes commented code Issue #10: Removed redundant .upper() call - Removed controller_type.upper() in get_default_value() - controller_type is already uppercase (from ControllerTypeKey Literal) - Redundant string operation eliminated Issue #11: Type safety already implemented - Confirmed ControllerTypeKey Literal already exists in nac_test/core/types.py - No changes needed, type safety already correct Issue #12: Added TypedDict for structured return type - Created CredentialSetStatus TypedDict in controller.py - Replaced untyped dict[str, Any] with proper TypedDict structure - Updated _find_credential_sets() return type annotation - Provides better type safety and IDE support IOSXE Optional Credentials Test Coverage: Added comprehensive test file test_base_test_iosxe_credentials.py to verify IOSXE controller's unique optional credentials behavior: - test_iosxe_setup_works_without_username_password: Verifies real-world scenario where only IOSXE_URL is set (D2D tests use device-specific credentials) - test_iosxe_setup_works_with_username_password: Verifies optional credentials are accepted if provided - test_aci_setup_requires_username_password: Verifies normal 3-credential pattern still works for controller-based architectures - test_aci_setup_fails_without_username: Verifies incomplete credentials are caught by detect_controller_type() before setup() reads them Added controller environment fixtures to tests/unit/conftest.py: - aci_controller_env: Sets ACI_URL, ACI_USERNAME, ACI_PASSWORD - sdwan_controller_env: Sets SDWAN_URL, SDWAN_USERNAME, SDWAN_PASSWORD - cc_controller_env: Sets CC_URL, CC_USERNAME, CC_PASSWORD These fixtures provide consistent test data and replace manual monkeypatch setup in individual tests. Test Updates: Updated test_base_test_defaults.py to work with performance optimization: - All test methods now manually set instance.controller_type before calling get_default_value() to simulate what setup() does - Changed test_no_controller_credentials_raises to expect AttributeError when controller_type not set (instead of ValueError from detection) - Updated docstrings to reflect that tests verify cached controller_type usage rather than auto-detection behavior All tests pass (618 tests in unit suite). * Add defaults_prefix to ControllerConfig dataclass - Extend ControllerConfig with defaults_prefix field for JMESPath prefix mapping - Add get_defaults_prefix() helper function for controller-to-prefix lookup - Configure defaults_prefix for all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Enable automatic defaults resolution without per-architecture configuration This consolidates controller configuration metadata into a single source of truth, eliminating the need for separate mapping dictionaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove dead missing_error parameter from get_default_value() - Remove unused missing_error parameter from function signature - Function generates its own clear, detailed error messages - Keep missing_error in ensure_defaults_block_exists() where it is used - Simplify function interface by removing unused parameter The parameter was accepted but never referenced in the function body. CLI validators perform pre-flight validation, so get_default_value() only needs to report which specific paths were not found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update get_default_value() to use new interface - Remove unused get_display_name import - Remove generation of unused missing_error parameter - Call _resolve() with simplified parameter list - Leverage controller type detection from setup() The function now delegates cleanly to defaults_resolver without constructing parameters that are never used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add shared PyATS mocking infrastructure to conftest.py - Extract common PyATS mock fixtures from individual test files - Provide nac_test_base_class fixture for consistent test setup - Eliminate duplicated mocking code across test modules - Enable proper module isolation in PyATS-dependent tests This shared infrastructure prevents import errors and ensures consistent PyATS mocking across all pyats_core common tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add integration tests for controller defaults mapping - Add 18 integration tests verifying controller-to-prefix mappings - Test all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Verify end-to-end flow from detection to defaults resolution - Test error propagation for missing defaults and invalid paths - Validate unique defaults_prefix for each controller type These tests ensure the complete integration between controller detection and defaults resolution works correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update tests to remove missing_error parameter assertions - Remove assertions checking for missing_error in get_default_value() calls - Update test expectations to match simplified function interface - Add missing_error to ensure_defaults_block_exists() calls - Remove duplicated PyATS mocking code (now in conftest.py) Tests now correctly validate the updated function signatures where get_default_value() does not accept missing_error parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unnecessary __future__ import from new files Python 3.10+ natively supports PEP 604 (str | None) and PEP 585 (dict[str, Any]) without requiring 'from __future__ import annotations'. The import was cargo-culted from other files but serves no purpose in base_test.py and defaults_resolver.py: - No forward references (self-referencing class types) - No circular import issues - All type hints use native 3.10+ syntax Verified by running full test suite: 711 tests pass (636 unit + 75 integration). * Address PR review feedback: rename, IOSXE_HOST support, test improvements - Rename get_default_value() to resolve_default_value() in defaults_resolver.py to avoid confusing private alias in base_test.py - Add IOSXE_HOST as alternative URL env var alongside IOSXE_URL via new alt_url_env_vars field on ControllerConfig and get_controller_url() - Parametrize repetitive test clusters in test_defaults_resolver.py (falsy values, architecture prefixes, error messages) - Replace tempfile.NamedTemporaryFile + manual cleanup with tmp_path and monkeypatch in test_base_test_iosxe_credentials.py and test_ssh_base_test_validation.py - Clean up docstrings in test_controller_defaults_integration.py --------- Co-authored-by: Oliver Frolovs <noreply@example.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
oboehmer
pushed a commit
that referenced
this pull request
Mar 19, 2026
…Base (#551) * test(structure): add pyats_core unit test directory structure Create unit test directory structure for pyats_core components to organize comprehensive test coverage for core framework utilities. Part of defaults resolution infrastructure. * feat(pyats): add architecture-agnostic defaults resolution utilities Add pure utility module for reading default values from merged NAC data models: - ensure_defaults_block_exists(): Validates defaults block presence with clear error messages - get_default_value(): Single-path and cascade lookup with required/optional support, using JMESPath for data model traversal Architecture-agnostic design - all parameters (prefix, error message) passed by caller. No PyATS dependencies. Supports all NAC architectures (ACI, SD-WAN, Catalyst Center) through configuration. Features: - Cascade/fallback support across multiple JMESPaths - Handles falsy values correctly (False, 0, "", [], {}) - Comprehensive docstrings with usage examples - Type-safe with Python 3.10+ annotations * feat(pyats): add get_default_value() method to NACTestBase Add defaults resolution capability to NACTestBase, making it available to all architecture test classes through opt-in configuration: Class Attributes (subclasses configure): - DEFAULTS_PREFIX: Architecture-specific path (e.g., "defaults.apic") - DEFAULTS_MISSING_ERROR: Custom error message for missing defaults Instance Method: - get_default_value(*paths, required=True): Wrapper that delegates to defaults_resolver utilities, providing class-level configuration Opt-in architecture: - DEFAULTS_PREFIX defaults to None - Raises NotImplementedError if called without configuration - Architectures enable by setting DEFAULTS_PREFIX class attribute Also includes minor cleanup: - Remove obsolete type: ignore comments from markdown/yaml imports - Update decorator type ignore comments to use 'misc' category This enables test scripts to access default values from data model with same cascade/fallback behavior as Jinja2 templates. * test(pyats): add comprehensive unit tests for defaults_resolver module Add 51 unit tests (864 lines) for defaults_resolver utility functions, organized into focused test classes: TestEnsureDefaultsBlockExists (7 tests): - Valid defaults block detection - Missing defaults block error handling - Architecture-specific prefix validation TestGetDefaultValueSinglePath (13 tests): - Single-path lookups with nested paths - Falsy value handling (False, 0, "", [], {}) - Required vs optional behavior - Return type verification (str, int, dict, list) TestGetDefaultValueCascade (8 tests): - Multi-path fallback behavior - First-found-wins semantics - All-missing scenarios (required/optional) TestGetDefaultValueErrorHandling (5 tests): - Missing paths TypeError - Custom error message propagation - Detailed error messages with attempted paths TestArchitectureAgnostic (7 tests): - APIC, SD-WAN, Catalyst Center prefix support - Custom architecture prefixes - Architecture-specific error messages TestEdgeCases (11 tests): - Deep nesting (5+ levels) - Special characters in keys - Large nested structures - Explicit None values - Data model immutability verification Test execution: All 51 tests pass in 0.06s * test(pyats): add integration tests for NACTestBase.get_default_value() Add 12 integration tests (364 lines) for NACTestBase defaults resolution wrapper, verifying correct delegation to defaults_resolver utilities: Test Coverage: - DEFAULTS_PREFIX=None raises NotImplementedError with clear message - DEFAULTS_PREFIX configured enables functionality - Custom error messages propagate correctly - Subclass override behavior (APIC vs SD-WAN vs CatC) - Cascade/fallback behavior through base class method - Optional (required=False) lookup returns None correctly - Deeply nested path lookups - Required=True raises ValueError for missing values - Falsy value handling (False, 0, "") through wrapper - Dict value returns Testing Strategy: - Minimal testable class mimics NACTestBase behavior - PyATS mocked via fixture to avoid import dependencies - Architecture-specific data models (APIC, SD-WAN) as fixtures - Verifies wrapper provides proper class-level configuration Test execution: All 12 tests pass in 0.08s Combined with defaults_resolver tests: 63 total tests in 0.14s * refactor: remove obsolete type ignore comments from imports Remove type: ignore comments that are no longer needed due to updated type stubs for importlib.metadata, yaml, and aiofiles packages. * refactor(reporting): improve type safety in template rendering Replace type: ignore[no-any-return] with explicit str() cast for Jinja2 template.render() return values, improving type safety. * refactor(robot): remove obsolete type ignore comments Remove type: ignore comments from Jinja2 and Robot Framework imports, no longer needed with updated type stubs. * test: remove obsolete type ignore comments from test files Remove type: ignore comments that are no longer needed with updated type stubs for importlib.resources and test fixtures. * refactor(pyats): add top-level import for defaults_resolver in base_test Move the defaults_resolver import to the module's top-level import section. This prepares for the next commit which removes the lazy import from inside the get_default_value() method body. The lazy import was originally added as a precaution against circular imports, but defaults_resolver.py is a pure utility module with no PyATS dependencies and no imports back into base_test, so there is no circular import risk. Top-level imports are preferred because they make dependencies explicit, fail fast at import time rather than at call time, and are consistent with the rest of this module's imports. * test(pyats): add conftest.py with shared defaults test fixtures Extract the apic_data_model and sdwan_data_model fixtures into a shared conftest.py file at tests/unit/pyats_core/common/. These two fixtures were duplicated identically across both test_defaults_resolver.py and test_base_test_defaults.py. Centralizing them in conftest.py follows pytest's fixture sharing convention and ensures a single source of truth for the sample data models used across all defaults-related tests. Both test files will have their local copies removed in subsequent commits. * test(pyats): remove duplicate fixtures from test_defaults_resolver Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py added in the previous commit. The remaining fixtures (catc_data_model, data_model_with_falsy_values, deeply_nested_data_model, empty_data_model, partial_data_model) are specific to this file's test scenarios and stay here. Updated the section comment to note where the shared fixtures live, so future contributors know not to re-add them. * test(pyats): add test for malformed JMESPath expression propagation Add test_malformed_jmespath_expression_propagates to verify that invalid JMESPath syntax (e.g., "[invalid") raises a jmespath ParseError that propagates directly to the caller. This behavior is by design: malformed path expressions indicate a programming error in the caller (wrong syntax), not a missing default value. Letting the ParseError propagate gives developers an immediate, clear traceback pointing at the malformed expression rather than a misleading "value not found" error that would waste debugging time. This test documents and locks in the current error propagation contract so that future refactors don't accidentally swallow these errors with a bare except or a catch-all handler. * test(pyats): add test for empty string path JMESPath behavior Add test_empty_string_path_raises_jmespath_error to document and verify the behavior when an empty string "" is passed as a default_path argument. When the path is empty, get_default_value constructs the full JMESPath expression as "defaults.apic." (with a trailing dot), which is syntactically invalid JMESPath. This correctly raises a ParseError rather than silently returning None or the entire defaults block. This is the desired behavior because an empty path is almost certainly a bug at the call site. A hard error forces the developer to fix the path rather than silently getting unexpected results. * test(pyats): remove unused temp_data_model_file fixture and imports Remove the temp_data_model_file fixture from test_base_test_defaults.py along with its associated imports (json, os, tempfile). This fixture was never referenced by any test in the file — it appears to have been scaffolded during initial development but was never wired into any test case. Removing dead code keeps the test file focused and avoids confusing future contributors who might wonder what tests use it or whether removing it would break something. * test(pyats): remove duplicate fixtures from test_base_test_defaults Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py at the same directory level. This completes the fixture deduplication across both defaults test files. Both test_defaults_resolver.py and test_base_test_defaults.py now consume the same fixture instances from conftest.py, ensuring the sample data models stay consistent if they ever need to change. * test(pyats): rewrite base_test_defaults to test real NACTestBase class Replace the TestableNACTestBase stand-in class with tests that exercise the real NACTestBase.get_default_value() method. This is a significant improvement to test quality. Problem with the previous approach: The old tests created a TestableNACTestBase inner class that manually reimplemented the get_default_value() logic (DEFAULTS_PREFIX guard, delegation to defaults_resolver, argument threading). This meant the tests were verifying the stand-in's behavior, not the actual production code. If someone changed NACTestBase.get_default_value() in base_test.py, these tests would still pass because they never exercised the real method. New approach — delegation contract tests: The rewritten tests import and instantiate the real NACTestBase class (with PyATS mocked out via sys.modules patching) and verify: 1. DEFAULTS_PREFIX=None guard: Calling get_default_value() on a class with no DEFAULTS_PREFIX raises NotImplementedError with the concrete class name in the message. 2. Delegation contract: When DEFAULTS_PREFIX is set, the method delegates to defaults_resolver.get_default_value (_resolve) with the correct positional args (data_model, *paths) and keyword args (defaults_prefix, missing_error, required). 3. Class attribute threading: DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR are correctly passed through to the resolver. The default error message is used when DEFAULTS_MISSING_ERROR is not overridden. 4. Subclass overrides: Architecture subclasses (e.g., SDWANTestBase) can override both class attributes and the correct values are threaded through. Why delegation tests are sufficient: The cascade behavior, falsy value handling, error messages, and edge cases are all thoroughly tested in test_defaults_resolver.py (51 tests). Since get_default_value() on NACTestBase is a thin wrapper that delegates to the same function, re-testing all those scenarios here would be redundant. These 7 tests focus exclusively on what the wrapper adds: the opt-in guard and argument threading. PyATS mock hierarchy: The mock_pyats fixture now covers the full import chain that NACTestBase requires: - pyats (top-level package) - pyats.aetest (Testcase base class, setup decorator) - pyats.aetest.steps (imported by step_interceptor) - pyats.aetest.steps.implementation (Step class) The mocks are wired hierarchically so that both `from pyats import aetest` and `import pyats.aetest` resolve to the same mock object. * fix(pyats): add future annotations import for Python 3.10 compatibility The Any | None type hint syntax fails in Python 3.10 because the typing module's Any type doesn't support the | operator without postponed evaluation of annotations. Add `from __future__ import annotations` to enable postponed annotation evaluation, which allows the modern union syntax to work across all supported Python versions (3.10+). This resolves the test failures on Python 3.10 while maintaining compatibility with 3.11, 3.12, and 3.13. * fix(tests): add controller credentials to SSH validation unit tests The SSHTestBase unit tests were failing because NACTestBase.setup() requires controller environment variables to detect the controller type. These tests were mocking NACTestBase.setup() but the mock wasn't applied before the setup method was called, causing controller detection to fail. Add IOSXE controller credentials to the test environment setup for all three SSH validation tests to satisfy the controller detection requirement. Fixes test failures: - test_validation_called_for_valid_device - test_validation_fails_for_missing_fields - test_validation_not_called_for_json_parse_error * fix(tests): cleanup environment variables in SSH validation tests Add cleanup of controller environment variables (IOSXE_URL, IOSXE_USERNAME, IOSXE_PASSWORD) in the finally blocks of all SSH validation tests. Without cleanup, these environment variables persist across test runs, causing "Multiple controller credentials detected" errors in subsequent tests that set different controller credentials (ACI, SDWAN, etc.). This was causing 4 test failures and 3 errors in the test suite: - test_end_to_end_controller_detection (SDWAN + IOSXE detected) - test_controller_switch_scenario (ACI + IOSXE detected) - test_falls_back_to_cwd_when_data_file_missing (ACI + IOSXE detected) - Several orchestrator tests (exit code 255) * feat(defaults): implement auto-detection of controller type for defaults resolution This commit adds automatic controller type detection for defaults resolution, eliminating the need for per-architecture configuration in test base classes. Key Changes: - Added CONTROLLER_TO_DEFAULTS_PREFIX mapping in controller.py that maps controller types (ACI, SDWAN, CC, etc.) to their defaults block prefixes (defaults.apic, defaults.sdwan, defaults.catc, etc.) - Rewrote NACTestBase.get_default_value() to auto-detect controller type from environment variables and look up the appropriate defaults prefix - Removed need for DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR class attributes - Optimized defaults_resolver.py by removing redundant validation calls (CLI validators from PR #525 handle pre-flight validation) - Updated test fixtures to include iosxe_controller_env for consistency - Updated 6 unit tests to match new behavior (no redundant validation) Architecture Mapping: ACI (ACI_URL) → defaults.apic SD-WAN (SDWAN_URL) → defaults.sdwan Catalyst Center (CC_URL) → defaults.catc IOS-XE (IOSXE_URL) → defaults.iosxe Meraki (MERAKI_URL) → defaults.meraki FMC (FMC_URL) → defaults.fmc ISE (ISE_URL) → defaults.ise Performance Impact: Eliminated 600+ redundant JMESPath searches per test run by removing ensure_defaults_block_exists() calls from get_default_value(). CLI validators handle this check once before test execution begins. Co-Authored-By: Oliver Frolovs <noreply@example.com> * Rewrite base test defaults tests for auto-detection design Replaces manual DEFAULTS_PREFIX configuration tests with auto-detection tests that verify controller type detection from environment variables. Changes: - Tests now verify auto-detection of ACI, SD-WAN, CC, and IOS-XE controllers - Added controller environment fixtures for all supported architectures - Tests verify correct defaults prefix mapping (ACI→apic, SDWAN→sdwan, etc.) - Tests verify graceful error handling when no controller credentials found - Removed obsolete NotImplementedError tests for manual configuration The new tests align with the auto-detection design where the framework automatically determines the defaults prefix based on detected controller type (ACI_URL → defaults.apic, SDWAN_URL → defaults.sdwan, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): make USERNAME/PASSWORD optional for IOSXE controller IOSXE is unique among supported controllers - it only requires IOSXE_URL because D2D tests use device-specific credentials from the device inventory, not controller credentials. Before this fix, setup() unconditionally tried to read USERNAME/PASSWORD from environment variables, causing KeyError for valid IOSXE configurations. Changes: - Changed os.environ[...] to os.environ.get(...) for USERNAME/PASSWORD - Added comprehensive tests for IOSXE optional credentials - Tests verify both scenarios: * IOSXE with only URL (no username/password) - now works * IOSXE with URL + username/password - still works * ACI with incomplete credentials - fails at detection (correct behavior) The fix allows IOSXE D2D tests to run with only IOSXE_URL set, while still supporting controller-based architectures (ACI, SD-WAN, CC) that require all three credentials. Fixes code review issue #1 (BLOCKING). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Resolve code review issues #3-#12 from PR #551 This commit addresses all remaining code review findings, implementing comprehensive fixes for code quality, performance, type safety, and test coverage. Fixes Implemented: Issue #3: Deleted dead class attributes - Removed unused DEFAULTS_PREFIX class attribute - Removed unused DEFAULTS_MISSING_ERROR class attribute - These were leftover from refactoring and never used Issue #4: Eliminated redundant controller type detection - Changed get_default_value() to use cached self.controller_type - Previously called detect_controller_type() on every invocation (21 env lookups) - Now uses value set once in setup(), dramatically improving performance - Updated test_base_test_defaults.py to manually set controller_type to simulate setup() Issue #5: Removed duplicate imports - Removed detect_controller_type from get_default_value() imports - Only CONTROLLER_TO_DEFAULTS_PREFIX needed now that we use cached value Issue #6: Removed unused function parameter - Deleted partial_credentials parameter from _format_no_credentials_error() - Parameter was never read, function works without it - Updated call site at line 162 Issue #7: Removed unreachable None check - Changed from CONTROLLER_TO_DEFAULTS_PREFIX.get() to direct subscript - None check was unreachable because all ControllerTypeKey values exist in dict - Direct subscript is more correct and eliminates dead code branch Issue #8: Replaced chr(10) workaround with '\n' - Changed chr(10) to '\n' literal in _format_multiple_credentials_error() - No need for ASCII code workaround in modern Python - More readable and idiomatic Issue #9: Deleted commented-out code - Removed 33 lines of commented __init_subclass__ implementation - Code was kept "for reference" but never needed - Clean codebase removes commented code Issue #10: Removed redundant .upper() call - Removed controller_type.upper() in get_default_value() - controller_type is already uppercase (from ControllerTypeKey Literal) - Redundant string operation eliminated Issue #11: Type safety already implemented - Confirmed ControllerTypeKey Literal already exists in nac_test/core/types.py - No changes needed, type safety already correct Issue #12: Added TypedDict for structured return type - Created CredentialSetStatus TypedDict in controller.py - Replaced untyped dict[str, Any] with proper TypedDict structure - Updated _find_credential_sets() return type annotation - Provides better type safety and IDE support IOSXE Optional Credentials Test Coverage: Added comprehensive test file test_base_test_iosxe_credentials.py to verify IOSXE controller's unique optional credentials behavior: - test_iosxe_setup_works_without_username_password: Verifies real-world scenario where only IOSXE_URL is set (D2D tests use device-specific credentials) - test_iosxe_setup_works_with_username_password: Verifies optional credentials are accepted if provided - test_aci_setup_requires_username_password: Verifies normal 3-credential pattern still works for controller-based architectures - test_aci_setup_fails_without_username: Verifies incomplete credentials are caught by detect_controller_type() before setup() reads them Added controller environment fixtures to tests/unit/conftest.py: - aci_controller_env: Sets ACI_URL, ACI_USERNAME, ACI_PASSWORD - sdwan_controller_env: Sets SDWAN_URL, SDWAN_USERNAME, SDWAN_PASSWORD - cc_controller_env: Sets CC_URL, CC_USERNAME, CC_PASSWORD These fixtures provide consistent test data and replace manual monkeypatch setup in individual tests. Test Updates: Updated test_base_test_defaults.py to work with performance optimization: - All test methods now manually set instance.controller_type before calling get_default_value() to simulate what setup() does - Changed test_no_controller_credentials_raises to expect AttributeError when controller_type not set (instead of ValueError from detection) - Updated docstrings to reflect that tests verify cached controller_type usage rather than auto-detection behavior All tests pass (618 tests in unit suite). * Add defaults_prefix to ControllerConfig dataclass - Extend ControllerConfig with defaults_prefix field for JMESPath prefix mapping - Add get_defaults_prefix() helper function for controller-to-prefix lookup - Configure defaults_prefix for all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Enable automatic defaults resolution without per-architecture configuration This consolidates controller configuration metadata into a single source of truth, eliminating the need for separate mapping dictionaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove dead missing_error parameter from get_default_value() - Remove unused missing_error parameter from function signature - Function generates its own clear, detailed error messages - Keep missing_error in ensure_defaults_block_exists() where it is used - Simplify function interface by removing unused parameter The parameter was accepted but never referenced in the function body. CLI validators perform pre-flight validation, so get_default_value() only needs to report which specific paths were not found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update get_default_value() to use new interface - Remove unused get_display_name import - Remove generation of unused missing_error parameter - Call _resolve() with simplified parameter list - Leverage controller type detection from setup() The function now delegates cleanly to defaults_resolver without constructing parameters that are never used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add shared PyATS mocking infrastructure to conftest.py - Extract common PyATS mock fixtures from individual test files - Provide nac_test_base_class fixture for consistent test setup - Eliminate duplicated mocking code across test modules - Enable proper module isolation in PyATS-dependent tests This shared infrastructure prevents import errors and ensures consistent PyATS mocking across all pyats_core common tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add integration tests for controller defaults mapping - Add 18 integration tests verifying controller-to-prefix mappings - Test all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Verify end-to-end flow from detection to defaults resolution - Test error propagation for missing defaults and invalid paths - Validate unique defaults_prefix for each controller type These tests ensure the complete integration between controller detection and defaults resolution works correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update tests to remove missing_error parameter assertions - Remove assertions checking for missing_error in get_default_value() calls - Update test expectations to match simplified function interface - Add missing_error to ensure_defaults_block_exists() calls - Remove duplicated PyATS mocking code (now in conftest.py) Tests now correctly validate the updated function signatures where get_default_value() does not accept missing_error parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unnecessary __future__ import from new files Python 3.10+ natively supports PEP 604 (str | None) and PEP 585 (dict[str, Any]) without requiring 'from __future__ import annotations'. The import was cargo-culted from other files but serves no purpose in base_test.py and defaults_resolver.py: - No forward references (self-referencing class types) - No circular import issues - All type hints use native 3.10+ syntax Verified by running full test suite: 711 tests pass (636 unit + 75 integration). * Address PR review feedback: rename, IOSXE_HOST support, test improvements - Rename get_default_value() to resolve_default_value() in defaults_resolver.py to avoid confusing private alias in base_test.py - Add IOSXE_HOST as alternative URL env var alongside IOSXE_URL via new alt_url_env_vars field on ControllerConfig and get_controller_url() - Parametrize repetitive test clusters in test_defaults_resolver.py (falsy values, architecture prefixes, error messages) - Replace tempfile.NamedTemporaryFile + manual cleanup with tmp_path and monkeypatch in test_base_test_iosxe_credentials.py and test_ssh_base_test_validation.py - Clean up docstrings in test_controller_defaults_integration.py --------- Co-authored-by: Oliver Frolovs <noreply@example.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
oboehmer
pushed a commit
that referenced
this pull request
Mar 19, 2026
…Base (#551) * test(structure): add pyats_core unit test directory structure Create unit test directory structure for pyats_core components to organize comprehensive test coverage for core framework utilities. Part of defaults resolution infrastructure. * feat(pyats): add architecture-agnostic defaults resolution utilities Add pure utility module for reading default values from merged NAC data models: - ensure_defaults_block_exists(): Validates defaults block presence with clear error messages - get_default_value(): Single-path and cascade lookup with required/optional support, using JMESPath for data model traversal Architecture-agnostic design - all parameters (prefix, error message) passed by caller. No PyATS dependencies. Supports all NAC architectures (ACI, SD-WAN, Catalyst Center) through configuration. Features: - Cascade/fallback support across multiple JMESPaths - Handles falsy values correctly (False, 0, "", [], {}) - Comprehensive docstrings with usage examples - Type-safe with Python 3.10+ annotations * feat(pyats): add get_default_value() method to NACTestBase Add defaults resolution capability to NACTestBase, making it available to all architecture test classes through opt-in configuration: Class Attributes (subclasses configure): - DEFAULTS_PREFIX: Architecture-specific path (e.g., "defaults.apic") - DEFAULTS_MISSING_ERROR: Custom error message for missing defaults Instance Method: - get_default_value(*paths, required=True): Wrapper that delegates to defaults_resolver utilities, providing class-level configuration Opt-in architecture: - DEFAULTS_PREFIX defaults to None - Raises NotImplementedError if called without configuration - Architectures enable by setting DEFAULTS_PREFIX class attribute Also includes minor cleanup: - Remove obsolete type: ignore comments from markdown/yaml imports - Update decorator type ignore comments to use 'misc' category This enables test scripts to access default values from data model with same cascade/fallback behavior as Jinja2 templates. * test(pyats): add comprehensive unit tests for defaults_resolver module Add 51 unit tests (864 lines) for defaults_resolver utility functions, organized into focused test classes: TestEnsureDefaultsBlockExists (7 tests): - Valid defaults block detection - Missing defaults block error handling - Architecture-specific prefix validation TestGetDefaultValueSinglePath (13 tests): - Single-path lookups with nested paths - Falsy value handling (False, 0, "", [], {}) - Required vs optional behavior - Return type verification (str, int, dict, list) TestGetDefaultValueCascade (8 tests): - Multi-path fallback behavior - First-found-wins semantics - All-missing scenarios (required/optional) TestGetDefaultValueErrorHandling (5 tests): - Missing paths TypeError - Custom error message propagation - Detailed error messages with attempted paths TestArchitectureAgnostic (7 tests): - APIC, SD-WAN, Catalyst Center prefix support - Custom architecture prefixes - Architecture-specific error messages TestEdgeCases (11 tests): - Deep nesting (5+ levels) - Special characters in keys - Large nested structures - Explicit None values - Data model immutability verification Test execution: All 51 tests pass in 0.06s * test(pyats): add integration tests for NACTestBase.get_default_value() Add 12 integration tests (364 lines) for NACTestBase defaults resolution wrapper, verifying correct delegation to defaults_resolver utilities: Test Coverage: - DEFAULTS_PREFIX=None raises NotImplementedError with clear message - DEFAULTS_PREFIX configured enables functionality - Custom error messages propagate correctly - Subclass override behavior (APIC vs SD-WAN vs CatC) - Cascade/fallback behavior through base class method - Optional (required=False) lookup returns None correctly - Deeply nested path lookups - Required=True raises ValueError for missing values - Falsy value handling (False, 0, "") through wrapper - Dict value returns Testing Strategy: - Minimal testable class mimics NACTestBase behavior - PyATS mocked via fixture to avoid import dependencies - Architecture-specific data models (APIC, SD-WAN) as fixtures - Verifies wrapper provides proper class-level configuration Test execution: All 12 tests pass in 0.08s Combined with defaults_resolver tests: 63 total tests in 0.14s * refactor: remove obsolete type ignore comments from imports Remove type: ignore comments that are no longer needed due to updated type stubs for importlib.metadata, yaml, and aiofiles packages. * refactor(reporting): improve type safety in template rendering Replace type: ignore[no-any-return] with explicit str() cast for Jinja2 template.render() return values, improving type safety. * refactor(robot): remove obsolete type ignore comments Remove type: ignore comments from Jinja2 and Robot Framework imports, no longer needed with updated type stubs. * test: remove obsolete type ignore comments from test files Remove type: ignore comments that are no longer needed with updated type stubs for importlib.resources and test fixtures. * refactor(pyats): add top-level import for defaults_resolver in base_test Move the defaults_resolver import to the module's top-level import section. This prepares for the next commit which removes the lazy import from inside the get_default_value() method body. The lazy import was originally added as a precaution against circular imports, but defaults_resolver.py is a pure utility module with no PyATS dependencies and no imports back into base_test, so there is no circular import risk. Top-level imports are preferred because they make dependencies explicit, fail fast at import time rather than at call time, and are consistent with the rest of this module's imports. * test(pyats): add conftest.py with shared defaults test fixtures Extract the apic_data_model and sdwan_data_model fixtures into a shared conftest.py file at tests/unit/pyats_core/common/. These two fixtures were duplicated identically across both test_defaults_resolver.py and test_base_test_defaults.py. Centralizing them in conftest.py follows pytest's fixture sharing convention and ensures a single source of truth for the sample data models used across all defaults-related tests. Both test files will have their local copies removed in subsequent commits. * test(pyats): remove duplicate fixtures from test_defaults_resolver Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py added in the previous commit. The remaining fixtures (catc_data_model, data_model_with_falsy_values, deeply_nested_data_model, empty_data_model, partial_data_model) are specific to this file's test scenarios and stay here. Updated the section comment to note where the shared fixtures live, so future contributors know not to re-add them. * test(pyats): add test for malformed JMESPath expression propagation Add test_malformed_jmespath_expression_propagates to verify that invalid JMESPath syntax (e.g., "[invalid") raises a jmespath ParseError that propagates directly to the caller. This behavior is by design: malformed path expressions indicate a programming error in the caller (wrong syntax), not a missing default value. Letting the ParseError propagate gives developers an immediate, clear traceback pointing at the malformed expression rather than a misleading "value not found" error that would waste debugging time. This test documents and locks in the current error propagation contract so that future refactors don't accidentally swallow these errors with a bare except or a catch-all handler. * test(pyats): add test for empty string path JMESPath behavior Add test_empty_string_path_raises_jmespath_error to document and verify the behavior when an empty string "" is passed as a default_path argument. When the path is empty, get_default_value constructs the full JMESPath expression as "defaults.apic." (with a trailing dot), which is syntactically invalid JMESPath. This correctly raises a ParseError rather than silently returning None or the entire defaults block. This is the desired behavior because an empty path is almost certainly a bug at the call site. A hard error forces the developer to fix the path rather than silently getting unexpected results. * test(pyats): remove unused temp_data_model_file fixture and imports Remove the temp_data_model_file fixture from test_base_test_defaults.py along with its associated imports (json, os, tempfile). This fixture was never referenced by any test in the file — it appears to have been scaffolded during initial development but was never wired into any test case. Removing dead code keeps the test file focused and avoids confusing future contributors who might wonder what tests use it or whether removing it would break something. * test(pyats): remove duplicate fixtures from test_base_test_defaults Remove the apic_data_model and sdwan_data_model fixture definitions that were duplicated locally in this file. These fixtures are now provided by the shared conftest.py at the same directory level. This completes the fixture deduplication across both defaults test files. Both test_defaults_resolver.py and test_base_test_defaults.py now consume the same fixture instances from conftest.py, ensuring the sample data models stay consistent if they ever need to change. * test(pyats): rewrite base_test_defaults to test real NACTestBase class Replace the TestableNACTestBase stand-in class with tests that exercise the real NACTestBase.get_default_value() method. This is a significant improvement to test quality. Problem with the previous approach: The old tests created a TestableNACTestBase inner class that manually reimplemented the get_default_value() logic (DEFAULTS_PREFIX guard, delegation to defaults_resolver, argument threading). This meant the tests were verifying the stand-in's behavior, not the actual production code. If someone changed NACTestBase.get_default_value() in base_test.py, these tests would still pass because they never exercised the real method. New approach — delegation contract tests: The rewritten tests import and instantiate the real NACTestBase class (with PyATS mocked out via sys.modules patching) and verify: 1. DEFAULTS_PREFIX=None guard: Calling get_default_value() on a class with no DEFAULTS_PREFIX raises NotImplementedError with the concrete class name in the message. 2. Delegation contract: When DEFAULTS_PREFIX is set, the method delegates to defaults_resolver.get_default_value (_resolve) with the correct positional args (data_model, *paths) and keyword args (defaults_prefix, missing_error, required). 3. Class attribute threading: DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR are correctly passed through to the resolver. The default error message is used when DEFAULTS_MISSING_ERROR is not overridden. 4. Subclass overrides: Architecture subclasses (e.g., SDWANTestBase) can override both class attributes and the correct values are threaded through. Why delegation tests are sufficient: The cascade behavior, falsy value handling, error messages, and edge cases are all thoroughly tested in test_defaults_resolver.py (51 tests). Since get_default_value() on NACTestBase is a thin wrapper that delegates to the same function, re-testing all those scenarios here would be redundant. These 7 tests focus exclusively on what the wrapper adds: the opt-in guard and argument threading. PyATS mock hierarchy: The mock_pyats fixture now covers the full import chain that NACTestBase requires: - pyats (top-level package) - pyats.aetest (Testcase base class, setup decorator) - pyats.aetest.steps (imported by step_interceptor) - pyats.aetest.steps.implementation (Step class) The mocks are wired hierarchically so that both `from pyats import aetest` and `import pyats.aetest` resolve to the same mock object. * fix(pyats): add future annotations import for Python 3.10 compatibility The Any | None type hint syntax fails in Python 3.10 because the typing module's Any type doesn't support the | operator without postponed evaluation of annotations. Add `from __future__ import annotations` to enable postponed annotation evaluation, which allows the modern union syntax to work across all supported Python versions (3.10+). This resolves the test failures on Python 3.10 while maintaining compatibility with 3.11, 3.12, and 3.13. * fix(tests): add controller credentials to SSH validation unit tests The SSHTestBase unit tests were failing because NACTestBase.setup() requires controller environment variables to detect the controller type. These tests were mocking NACTestBase.setup() but the mock wasn't applied before the setup method was called, causing controller detection to fail. Add IOSXE controller credentials to the test environment setup for all three SSH validation tests to satisfy the controller detection requirement. Fixes test failures: - test_validation_called_for_valid_device - test_validation_fails_for_missing_fields - test_validation_not_called_for_json_parse_error * fix(tests): cleanup environment variables in SSH validation tests Add cleanup of controller environment variables (IOSXE_URL, IOSXE_USERNAME, IOSXE_PASSWORD) in the finally blocks of all SSH validation tests. Without cleanup, these environment variables persist across test runs, causing "Multiple controller credentials detected" errors in subsequent tests that set different controller credentials (ACI, SDWAN, etc.). This was causing 4 test failures and 3 errors in the test suite: - test_end_to_end_controller_detection (SDWAN + IOSXE detected) - test_controller_switch_scenario (ACI + IOSXE detected) - test_falls_back_to_cwd_when_data_file_missing (ACI + IOSXE detected) - Several orchestrator tests (exit code 255) * feat(defaults): implement auto-detection of controller type for defaults resolution This commit adds automatic controller type detection for defaults resolution, eliminating the need for per-architecture configuration in test base classes. Key Changes: - Added CONTROLLER_TO_DEFAULTS_PREFIX mapping in controller.py that maps controller types (ACI, SDWAN, CC, etc.) to their defaults block prefixes (defaults.apic, defaults.sdwan, defaults.catc, etc.) - Rewrote NACTestBase.get_default_value() to auto-detect controller type from environment variables and look up the appropriate defaults prefix - Removed need for DEFAULTS_PREFIX and DEFAULTS_MISSING_ERROR class attributes - Optimized defaults_resolver.py by removing redundant validation calls (CLI validators from PR #525 handle pre-flight validation) - Updated test fixtures to include iosxe_controller_env for consistency - Updated 6 unit tests to match new behavior (no redundant validation) Architecture Mapping: ACI (ACI_URL) → defaults.apic SD-WAN (SDWAN_URL) → defaults.sdwan Catalyst Center (CC_URL) → defaults.catc IOS-XE (IOSXE_URL) → defaults.iosxe Meraki (MERAKI_URL) → defaults.meraki FMC (FMC_URL) → defaults.fmc ISE (ISE_URL) → defaults.ise Performance Impact: Eliminated 600+ redundant JMESPath searches per test run by removing ensure_defaults_block_exists() calls from get_default_value(). CLI validators handle this check once before test execution begins. Co-Authored-By: Oliver Frolovs <noreply@example.com> * Rewrite base test defaults tests for auto-detection design Replaces manual DEFAULTS_PREFIX configuration tests with auto-detection tests that verify controller type detection from environment variables. Changes: - Tests now verify auto-detection of ACI, SD-WAN, CC, and IOS-XE controllers - Added controller environment fixtures for all supported architectures - Tests verify correct defaults prefix mapping (ACI→apic, SDWAN→sdwan, etc.) - Tests verify graceful error handling when no controller credentials found - Removed obsolete NotImplementedError tests for manual configuration The new tests align with the auto-detection design where the framework automatically determines the defaults prefix based on detected controller type (ACI_URL → defaults.apic, SDWAN_URL → defaults.sdwan, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): make USERNAME/PASSWORD optional for IOSXE controller IOSXE is unique among supported controllers - it only requires IOSXE_URL because D2D tests use device-specific credentials from the device inventory, not controller credentials. Before this fix, setup() unconditionally tried to read USERNAME/PASSWORD from environment variables, causing KeyError for valid IOSXE configurations. Changes: - Changed os.environ[...] to os.environ.get(...) for USERNAME/PASSWORD - Added comprehensive tests for IOSXE optional credentials - Tests verify both scenarios: * IOSXE with only URL (no username/password) - now works * IOSXE with URL + username/password - still works * ACI with incomplete credentials - fails at detection (correct behavior) The fix allows IOSXE D2D tests to run with only IOSXE_URL set, while still supporting controller-based architectures (ACI, SD-WAN, CC) that require all three credentials. Fixes code review issue #1 (BLOCKING). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Resolve code review issues #3-#12 from PR #551 This commit addresses all remaining code review findings, implementing comprehensive fixes for code quality, performance, type safety, and test coverage. Fixes Implemented: Issue #3: Deleted dead class attributes - Removed unused DEFAULTS_PREFIX class attribute - Removed unused DEFAULTS_MISSING_ERROR class attribute - These were leftover from refactoring and never used Issue #4: Eliminated redundant controller type detection - Changed get_default_value() to use cached self.controller_type - Previously called detect_controller_type() on every invocation (21 env lookups) - Now uses value set once in setup(), dramatically improving performance - Updated test_base_test_defaults.py to manually set controller_type to simulate setup() Issue #5: Removed duplicate imports - Removed detect_controller_type from get_default_value() imports - Only CONTROLLER_TO_DEFAULTS_PREFIX needed now that we use cached value Issue #6: Removed unused function parameter - Deleted partial_credentials parameter from _format_no_credentials_error() - Parameter was never read, function works without it - Updated call site at line 162 Issue #7: Removed unreachable None check - Changed from CONTROLLER_TO_DEFAULTS_PREFIX.get() to direct subscript - None check was unreachable because all ControllerTypeKey values exist in dict - Direct subscript is more correct and eliminates dead code branch Issue #8: Replaced chr(10) workaround with '\n' - Changed chr(10) to '\n' literal in _format_multiple_credentials_error() - No need for ASCII code workaround in modern Python - More readable and idiomatic Issue #9: Deleted commented-out code - Removed 33 lines of commented __init_subclass__ implementation - Code was kept "for reference" but never needed - Clean codebase removes commented code Issue #10: Removed redundant .upper() call - Removed controller_type.upper() in get_default_value() - controller_type is already uppercase (from ControllerTypeKey Literal) - Redundant string operation eliminated Issue #11: Type safety already implemented - Confirmed ControllerTypeKey Literal already exists in nac_test/core/types.py - No changes needed, type safety already correct Issue #12: Added TypedDict for structured return type - Created CredentialSetStatus TypedDict in controller.py - Replaced untyped dict[str, Any] with proper TypedDict structure - Updated _find_credential_sets() return type annotation - Provides better type safety and IDE support IOSXE Optional Credentials Test Coverage: Added comprehensive test file test_base_test_iosxe_credentials.py to verify IOSXE controller's unique optional credentials behavior: - test_iosxe_setup_works_without_username_password: Verifies real-world scenario where only IOSXE_URL is set (D2D tests use device-specific credentials) - test_iosxe_setup_works_with_username_password: Verifies optional credentials are accepted if provided - test_aci_setup_requires_username_password: Verifies normal 3-credential pattern still works for controller-based architectures - test_aci_setup_fails_without_username: Verifies incomplete credentials are caught by detect_controller_type() before setup() reads them Added controller environment fixtures to tests/unit/conftest.py: - aci_controller_env: Sets ACI_URL, ACI_USERNAME, ACI_PASSWORD - sdwan_controller_env: Sets SDWAN_URL, SDWAN_USERNAME, SDWAN_PASSWORD - cc_controller_env: Sets CC_URL, CC_USERNAME, CC_PASSWORD These fixtures provide consistent test data and replace manual monkeypatch setup in individual tests. Test Updates: Updated test_base_test_defaults.py to work with performance optimization: - All test methods now manually set instance.controller_type before calling get_default_value() to simulate what setup() does - Changed test_no_controller_credentials_raises to expect AttributeError when controller_type not set (instead of ValueError from detection) - Updated docstrings to reflect that tests verify cached controller_type usage rather than auto-detection behavior All tests pass (618 tests in unit suite). * Add defaults_prefix to ControllerConfig dataclass - Extend ControllerConfig with defaults_prefix field for JMESPath prefix mapping - Add get_defaults_prefix() helper function for controller-to-prefix lookup - Configure defaults_prefix for all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Enable automatic defaults resolution without per-architecture configuration This consolidates controller configuration metadata into a single source of truth, eliminating the need for separate mapping dictionaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove dead missing_error parameter from get_default_value() - Remove unused missing_error parameter from function signature - Function generates its own clear, detailed error messages - Keep missing_error in ensure_defaults_block_exists() where it is used - Simplify function interface by removing unused parameter The parameter was accepted but never referenced in the function body. CLI validators perform pre-flight validation, so get_default_value() only needs to report which specific paths were not found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update get_default_value() to use new interface - Remove unused get_display_name import - Remove generation of unused missing_error parameter - Call _resolve() with simplified parameter list - Leverage controller type detection from setup() The function now delegates cleanly to defaults_resolver without constructing parameters that are never used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add shared PyATS mocking infrastructure to conftest.py - Extract common PyATS mock fixtures from individual test files - Provide nac_test_base_class fixture for consistent test setup - Eliminate duplicated mocking code across test modules - Enable proper module isolation in PyATS-dependent tests This shared infrastructure prevents import errors and ensures consistent PyATS mocking across all pyats_core common tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add integration tests for controller defaults mapping - Add 18 integration tests verifying controller-to-prefix mappings - Test all 7 controller types (ACI, SDWAN, CC, MERAKI, FMC, ISE, IOSXE) - Verify end-to-end flow from detection to defaults resolution - Test error propagation for missing defaults and invalid paths - Validate unique defaults_prefix for each controller type These tests ensure the complete integration between controller detection and defaults resolution works correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update tests to remove missing_error parameter assertions - Remove assertions checking for missing_error in get_default_value() calls - Update test expectations to match simplified function interface - Add missing_error to ensure_defaults_block_exists() calls - Remove duplicated PyATS mocking code (now in conftest.py) Tests now correctly validate the updated function signatures where get_default_value() does not accept missing_error parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unnecessary __future__ import from new files Python 3.10+ natively supports PEP 604 (str | None) and PEP 585 (dict[str, Any]) without requiring 'from __future__ import annotations'. The import was cargo-culted from other files but serves no purpose in base_test.py and defaults_resolver.py: - No forward references (self-referencing class types) - No circular import issues - All type hints use native 3.10+ syntax Verified by running full test suite: 711 tests pass (636 unit + 75 integration). * Address PR review feedback: rename, IOSXE_HOST support, test improvements - Rename get_default_value() to resolve_default_value() in defaults_resolver.py to avoid confusing private alias in base_test.py - Add IOSXE_HOST as alternative URL env var alongside IOSXE_URL via new alt_url_env_vars field on ControllerConfig and get_controller_url() - Parametrize repetitive test clusters in test_defaults_resolver.py (falsy values, architecture prefixes, error messages) - Replace tempfile.NamedTemporaryFile + manual cleanup with tmp_path and monkeypatch in test_base_test_iosxe_credentials.py and test_ssh_base_test_validation.py - Clean up docstrings in test_controller_defaults_integration.py --------- Co-authored-by: Oliver Frolovs <noreply@example.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds architecture-agnostic defaults resolution utilities to
NACTestBase, making it available to all NAC architectures (ACI, SD-WAN, Catalyst Center) through opt-in configuration.Previously, defaults resolution was implemented in nac-test-pyats-common as ACI-specific code. This refactor moves the core resolution logic to the base framework, enabling all architectures to leverage defaults files while maintaining architecture-specific prefixes and error messages.
Closes
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
New module:
nac_test/pyats_core/common/defaults_resolver.pyensure_defaults_block_exists(): Validates defaults block presenceget_default_value(): Single-path and cascade lookup with JMESPathNACTestBase enhancements:
nac_test/pyats_core/common/base_test.pyDEFAULTS_PREFIX,DEFAULTS_MISSING_ERRORget_default_value(*paths, required=True)NotImplementedErrorifDEFAULTS_PREFIXisNoneComprehensive test coverage: 63 tests (51 unit + 12 integration)
Testing Done
pytest/pre-commit run -a)Test Commands Used
Checklist
pre-commit run -apasses)Screenshots (if applicable)
N/A - This is infrastructure code with no UI changes.
Additional Notes
Architecture Design
This implementation follows the opt-in pattern used elsewhere in nac-test:
DEFAULTS_PREFIXclass attributeNotImplementedErrorif feature is used without configurationMigration Path for nac-test-pyats-common
After this PR merges, nac-test-pyats-common will:
nac_test_pyats_common/aci/defaults_resolver.pyAPICTestBaseto setDEFAULTS_PREFIX = "defaults.apic"get_default_value()fromNACTestBaseCascade/Fallback Support
Supports multiple JMESPaths with first-found-wins semantics:
Type Safety
Return type is
Any | Nonebecause JMESPath can return any type (str, int, bool, dict, list). This is intentional - the type depends on the data model structure at the queried path.