fix(pyats): display clean relative test names instead of absolute paths (#653)#655
Merged
fix(pyats): display clean relative test names instead of absolute paths (#653)#655
Conversation
…hs (#653) PyATS test names now show relative paths (e.g., "nrfu.verify_device") instead of full absolute paths. The fix passes test_dir from the orchestrator to the plugin subprocess via NAC_TEST_TEST_DIR environment variable, enabling proper relative path computation. - orchestrator.py: Set NAC_TEST_TEST_DIR env var for API tests, pass test_dir to ArchiveInspector - device_executor.py: Set NAC_TEST_TEST_DIR env var for D2D tests - plugin.py: Use NAC_TEST_TEST_DIR to compute relative test names; remove dead "templates" fallback code - archive_inspector.py: Accept test_dir as required parameter; fix redundant exception handling and docstring - test_e2e_scenarios.py: Add E2E test validating relative test names using section markers to isolate PyATS output
Replace resolve() with absolute() throughout the pyats_core stack so that symlinked test files are not dereferenced before relative_to() comparisons. resolve() follows symlinks to their target, which breaks the path prefix check when the target lives outside test_dir — exactly the case for the new pyats_api_only e2e fixture where the test file is a symlink into a parent dir. Changed files: - orchestrator.py: absolute() for test_dir and base_output_dir - plugin.py: absolute() in _get_test_name() - archive_inspector.py: absolute() in _derive_test_name_from_path() - job_generator.py: absolute() when embedding test file paths in job scripts - test_discovery.py: absolute() for exclude_paths and discovered test_files - test_type_resolver.py: absolute() for test_root and cache key normalisation Also incorporates review fixes from #653: - plugin.py: drop the hardcoded "templates" title fallback; reuse the already-computed test_name (relative dot-name, or bare stem as fallback) - archive_inspector.py: narrow broad `except (ValueError, Exception)` to `except ValueError` — the only exception relative_to() raises - archive_inspector.py: fix stale "Optional" wording in extract_test_results() docstring; test_dir is now a required parameter E2E fixture addition: - Move verify_aci_apic_appliance_operational_status.py to the pyats_api_only fixture root and replace it with a symlink inside templates/tests/, so the e2e suite exercises the absolute()-vs-resolve() path during a real run Test fix: - test_test_type_resolver.py: add mock.absolute.return_value = mock to _create_mock_path() to match the resolve() → absolute() refactor; drop the now-unused mock.resolve.return_value = mock line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was
linked to
issues
Mar 17, 2026
Closed
oboehmer
added
bug
- Enhanced plugin.py logging to show absolute path when relative_to() fails This helps debug edge cases where path resolution behaves unexpectedly - Stricter regex pattern in E2E tests to require test names start with word char Pattern changed from r'[\w.-]+' to r'\w[\w.-]*' to reject leading dots/hyphens Addresses code review feedback on PR #655
The license-headers.sh script previously rejected symlinks with an error, causing pre-commit checks to fail when symlinked test files were added. Changes: - Modified validate_file() to return code 2 for symlinks instead of error - Updated file processing loop to skip symlinks with a warning message - Symlinks are now skipped silently since their targets are checked independently - Uses || pattern to work around bash set -e behavior with non-zero returns This allows the E2E test fixtures to use symlinks (e.g., for testing the absolute() vs resolve() path handling in PR #655) without breaking CI.
Merged
oboehmer
changed the base branch from
release/pyats-integration-v1.1-beta
to
main
This was referenced Mar 23, 2026
Closed
aitestino
approved these changes
Mar 23, 2026
Collaborator
aitestino
left a comment
aitestino
left a comment
There was a problem hiding this comment.
Hey @oboehmer, thank you for the PR — this is a solid fix for the absolute-path test names in PyATS output.
Per our offline conversation, we're approving this PR with follow-up tracking issues for a few refinements identified during review. Nothing here blocks merge — they're all incremental improvements we can pick up after.
Opened tracking issues:
- #706 — refactor: extract shared test-name computation to
path_utils.py(prio: medium) —plugin.py::_get_test_name()andarchive_inspector.py::_derive_test_name_from_path()duplicate the samerelative_to()→ dot-notation logic - #707 — refactor: extract
NAC_TEST_TEST_DIRto named constant (prio: low) — the env var string appears in 4+ files as a bare literal - #708 — tests: unit tests for
ProgressReporterPlugin._get_test_name()(prio: high) — the core bug-fix method has zero unit test coverage - #709 — tests: unit tests for
NAC_TEST_TEST_DIRenv var propagation (prio: medium) — verify orchestrator and device_executor actually set the env var for subprocesses - #710 — perf: cache
test_dir_pathin plugin (prio: low) —_get_test_name()re-parses and re-resolves the env var on every call - #711 — fix: validate
NAC_TEST_TEST_DIRbefore use (prio: medium) — no check for empty string, non-existent path, or non-directory values - #712 — refactor: consistent error handling between plugin and archive_inspector (prio: medium) — plugin logs a warning + falls back on
ValueError, archive_inspector lets it propagate; related to #706 - #713 — chore: clarify
resolve()vsabsolute()forMERGED_DATA_MODELpath (prio: low) — orchestrator switched most paths toabsolute()but keptresolve()for the merged data model filepath - #714 — fix: harden
license-headers.shsymlink handling (prio: low) — theset -e+||workaround for symlink skipping is fragile - #715 — tests: unit test for symlinked test file in archive_inspector (prio: medium) — the archive inspector path now uses
absolute()but has no symlink-specific test coverage
None of these block the PR — they're all incremental refinements that can be addressed in follow-up PRs. Nice work on this one. 👍
P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂
Replace bare "NAC_TEST_TEST_DIR" string literals in plugin.py, orchestrator.py and device_executor.py with a single named constant ENV_TEST_DIR defined in nac_test/pyats_core/constants.py. Closes #707 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_get_test_name() was re-reading os.environ and re-constructing the Path object on every pre_task/post_task call. Resolve and store test_dir_path once at plugin initialisation (env var is static during a PyATS job). Also move the "env var not set" warning to __init__ so it fires once rather than once per test. Closes #710 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously the plugin accepted any non-empty string from the env var and would silently fall back on ValueError from relative_to(). Now the cached test_dir_path is validated at plugin initialisation: - Non-existent path or non-directory → warning + None (filename-only fallback) These are internal bug-guards only; neither condition should fire in production since test_dir is Typer-validated and ENV_TEST_DIR is set exclusively by our orchestrator/device_executor. Closes #711 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_utils.py plugin.py::_get_test_name() and archive_inspector.py::_derive_test_name_from_path() duplicated the same relative_to() → dot-notation logic. Extract it to a single derive_test_name() function in nac_test/utils/path_utils.py and update both callers to use it. Remove the now-redundant _derive_test_name_from_path() static method from ArchiveInspector. Move TestDeriveTestNameFromPath tests from test_archive_inspector.py to a new tests/unit/utils/test_path_utils.py targeting derive_test_name() directly. Closes #706 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both callers of derive_test_name() silently fell back on path mismatch. Add a debug log in derive_test_name() itself so both the plugin and archive_inspector get consistent visibility without duplication. Closes #712 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add tests/pyats_core/progress/test_plugin.py covering: - dot-notation derivation for one-level, nested, and root-level paths - stem-only fallback when ENV_TEST_DIR is unset - stem-only fallback when script is outside test_dir - caching: ENV_TEST_DIR changes after __init__ have no effect Uses the existing pyats_test_dirs fixture for directory setup. Closes #708 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verify that both the orchestrator (_execute_api_tests_standard) and DeviceExecutor (run_device_job_with_semaphore) include ENV_TEST_DIR in the environment dict passed to the subprocess runner. TestOrchestratorEnvPropagation is placed alongside the existing TestOrchestratorEnvVarProcesses in test_orchestrator_env_var.py. TestDeviceExecutorEnvPropagation lives in its own module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verify that derive_test_name handles a symlinked test_dir correctly. absolute() preserves symlinks on both sides of the relative_to comparison, whereas resolve() would follow them and could break the path matching. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the fragile `|| validate_result=$? / validate_result=${validate_result:-0}`
double-assignment pattern with a clean if/else block. No behaviour change;
set -e protection is preserved everywhere else.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
thanks for the review, @aitestino .. resolved all issues. |
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
Fix PyATS progress reporter to display clean relative test names when the templates directory doesn't contain a
testssubdirectory.Previously, when
parts.index("tests")failed, the code fell back to using the entire absolute path, resulting in output like:Now test names are computed relative to the
templates_dirpassed to the orchestrator:This is a leftover from the pre-#511 era when PyATS tests were required to be under a
tests/directory.Symlink safety:
absolute()instead ofresolve()(#656)A secondary but important fix applied throughout the PyATS stack: all
Path.resolve()calls used for path comparison (i.e.relative_to()) have been replaced withPath.absolute().Why this matters:
resolve()dereferences symlinks to their real filesystem location. When a test file insidetest_diris itself a symlink whose target lives outsidetest_dir,resolve()returns the target path — andrelative_to(test_dir)then raisesValueError, causing the test name to fall back to the bare stem or the full absolute path.absolute()makes the path absolute without following symlinks, so the logical structure undertest_diris preserved andrelative_to()works correctly regardless of where symlink targets live.Affected files:
orchestrator.py,job_generator.py,test_discovery.py,test_type_resolver.py,plugin.py,archive_inspector.py.Closes
Closes #706, Closes #707, Closes #708, Closes #709, Closes #710, Closes #711, Closes #712, Closes #713, Closes #714, Closes #715
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
NAC_TEST_TEST_DIRenvironment variable for API tests; passtest_dirtoArchiveInspector; useabsolute()instead ofresolve()fortest_dirandbase_output_dirto preserve symlinksNAC_TEST_TEST_DIRenvironment variable for D2D testsNAC_TEST_TEST_DIRto compute relative test names viaPath.absolute().relative_to(); removed dead "templates" fallback code; replacedresolve()withabsolute()test_diras required parameter instead of searching for hardcoded"tests"directory in path; narrowedexcept (ValueError, Exception)toexcept ValueError; replacedresolve()withabsolute()resolve()withabsolute()when embedding test file paths in generated job scriptsresolve()withabsolute()for exclude paths and discovered test filesresolve()withabsolute()fortest_rootand cache key normalisationtest_stdout_pyats_test_names_are_relative()validating relative test names using section markers to isolate PyATS output from pabot; added symlink fixture (pyats_api_only) to exercise theabsolute()fixTesting Done
Test Commands Used
Checklist
Screenshots (if applicable)
Before (when templates dir has no tests/ subdirectory):
After:
Additional Notes
The fix passes
test_dir(the templates path where tests are discovered) from the orchestrator to the plugin subprocess via theNAC_TEST_TEST_DIRenvironment variable. This enables proper relative path computation regardless of directory structure, eliminating the hardcoded dependency on atests/subdirectory.The
absolute()overresolve()change is applied consistently across the entire PyATS stack wherever paths are used forrelative_to()comparisons. The E2E fixture forpyats_api_onlyincludes a symlink that points outsidetest_dirto directly exercise this code path.The E2E test uses section markers (
Running PyATS tests→Running Robot Framework tests) to extract only PyATS output, avoiding false positives from pabot messages that use a similar format.