feat: merge Robot and PyATS xunit.xml files for CI/CD integration#557
feat: merge Robot and PyATS xunit.xml files for CI/CD integration#557oboehmer merged 19 commits intorelease/pyats-integration-v1.1-betafrom
Conversation
Add comprehensive e2e tests to validate hostname display across all D2D test reporting locations as specified in issue #482. Changes: - Add expected_d2d_hostnames field to E2EScenario dataclass - Configure hostnames for D2D scenarios (SUCCESS, ALL_FAIL, MIXED, PYATS_D2D_ONLY, PYATS_CC) - Add 6 hostname validation test methods to E2ECombinedTestBase: * Console output format: "Test Name (hostname) | STATUS |" * HTML summary tables and detail page headers * HTML filenames with sanitized hostnames * API test separation (no hostnames shown) * Character sanitization validation - Add HTML parsing helpers for hostname extraction and validation - Use exact sanitization logic matching base_test.py (r"[^a-zA-Z0-9]" → "_") - Liberal pattern matching for resilience to format changes - Smart skip logic for scenarios without D2D tests Test coverage validates all aspects of hostname display implemented in PR #478: - Console output displays hostnames in parentheses format - HTML reports show hostnames in summary and detail views - Filenames include properly sanitized hostnames - API tests correctly exclude hostnames - Cross-report consistency maintained Resolves #482
Create a combined xunit.xml at the output root by merging results from Robot Framework and PyATS test executions. This enables CI/CD pipelines (Jenkins, GitLab) to consume a single test results file. Changes: - Add xunit_merger module using lxml.etree for efficient XML parsing - Move Robot xunit.xml output to robot_results/ subdirectory - Integrate merger into combined orchestrator post-execution - Add lxml>=4.5.1 as direct dependency - Add 17 unit tests for merger functionality - Extend E2E tests to verify merged xunit.xml in all scenarios
oboehmer
added
enhancement
Use robot_results_dir Path object for xunit.xml path instead of f-string with constant, consistent with how output paths will be constructed in pabot 5.2 upgrade.
oboehmer
force-pushed
the
feature/xunit-merger
branch
3 times, most recently
from
bce28a1 to
75ec9a7
Compare
aitestino
left a comment
aitestino
left a comment
There was a problem hiding this comment.
Hey Oliver, nice work on this one — the xunit merger is clean and the E2E test coverage is solid. A few things I noticed:
-
collect_xunit_files()hardcodes"robot_results"and"pyats_results"as string literals, but we already haveROBOT_RESULTS_DIRNAMEandPYATS_RESULTS_DIRNAMEinnac_test/core/constants.py— and every other module in the codebase imports them from there. Would be good to stay consistent so if those paths ever change, this module doesn't silently break. -
In
_extract_testsuite_stats(), theint()andfloat()calls on XML attributes will raiseValueErrorif a malformed xunit file has non-numeric values. The caller already catchesET.ParseErrorandOSError, butValueErrorslips through. Adding it to that same exception handling block would keep the graceful-degradation behavior consistent. -
The
merge_xunit_results()call incombined_orchestrator.pydoesn't have a try/except around it — if something unexpected happens during the merge (permissions, disk full, etc.), it would crash the entire orchestrator run. A simple try/except with a warning log would make it resilient, since a failed merge shouldn't take down the test results. -
Minor one — the
"--xunit"flag was added to bothexecute_job()andexecute_job_with_testbed()insubprocess_runner.py. Those two methods already share a lot of duplicated command-building logic (pre-existing), but this adds one more thing to keep in sync. Not blocking, just worth noting for a future cleanup pass.
What do you think?
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! 🙂
…eta' into test/482-hostname-display
Remove HostnameDisplayInfo dataclass and four extraction functions (extract_hostname_from_display_text, extract_hostnames_from_summary_table, extract_hostname_from_detail_page_header, extract_hostnames_from_filenames) that were added during development but never used in the final tests.
Create sanitize_hostname() in nac_test/utils/strings.py using pattern [^a-zA-Z0-9_] to clearly indicate underscores are safe characters. Update base_test.py and job_generator.py to use the shared utility instead of duplicating the regex pattern inline.
Add validation in E2EScenario.validate() ensuring expected_d2d_hostnames is populated when has_pyats_d2d_tests > 0. This makes the redundant "or not expected_d2d_hostnames" skip condition checks unnecessary in the 5 hostname test methods, simplifying the test logic.
…eta' into feature/xunit-merger
…strings Replace hardcoded "robot_results" and "pyats_results" string literals with ROBOT_RESULTS_DIRNAME and PYATS_RESULTS_DIRNAME constants from nac_test/core/constants.py for consistency with the rest of the codebase.
Add ValueError to exception handling in merge_xunit_files() to gracefully handle xunit files with non-numeric attribute values (e.g., tests="abc"). This maintains consistent graceful-degradation behavior alongside the existing ParseError and OSError handling.
Wrap merge_xunit_results() call in try/except to prevent unexpected errors (permissions, disk full, etc.) from crashing the entire test run. A failed merge now logs a warning instead of terminating the orchestrator, since test results are still valid without the merged file.
…mand helper Consolidate duplicated PyATS command construction from execute_job() and execute_job_with_testbed() into a single _build_command() method. This reduces code duplication and ensures consistent command building including --verbose/--quiet flag handling. Related tech debt tracked in #570.
|
Thanks again, @aitestino, you were on a run on a Sunday 😊. Addressed all four points: 1. Constants for directory names (16080f5) 2. ValueError handling in 3. try/except around 4. Command-building duplication in During the refactoring I also noticed some related tech debt around config file creation (duplicated code + an unused Would you mind taking another look when you get a chance? |
Addresses review comment from PR #560.
…eta' into feature/xunit-merger
oboehmer
merged commit a434700
into
release/pyats-integration-v1.1-beta
2 participants
Description
Implements combined xunit.xml generation by merging Robot Framework and PyATS test results into a single file at the output root directory. This enables CI/CD pipelines (Jenkins, GitLab) to consume a unified test results file.
Closes
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
xunit_mergermodule usinglxml.etreefor efficient XML parsingrobot_results/subdirectory (no longer at root)lxml>=4.5.1as direct dependency (was already indirect)Testing Done
pytest/pre-commit run -a)Test Commands Used
Checklist
pre-commit run -apasses)Screenshots (if applicable)
N/A
Additional Notes
Output structure change: The root
xunit.xmlis now a merged file (not a symlink). Individual xunit files remain in their respective subdirectories:robot_results/xunit.xml- Robot Framework resultspyats_results/api/xunit.xml- PyATS API resultspyats_results/d2d/<hostname>/xunit.xml- PyATS D2D results per deviceWhy lxml? Selected over stdlib
xml.etreefor better performance with large XML files. The dependency was already pulled in transitively.