feat(robot): upgrade pabot to 5.2.2 and write outputs directly to robot_results#560
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
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.
…ot_results - Update robotframework-pabot dependency to >= 5.2.2 - Use --output, --log, --report options to write artifacts directly to robot_results/ - Remove _move_robot_results_to_subdirectory() workaround from orchestrator - Update unit tests to reflect new behavior
315db8e to
11c3a28
Compare
Part of #560 - included here for branch self-containment.
aitestino
left a comment
There was a problem hiding this comment.
Hey @oboehmer, clean improvement — replacing the shutil.move workaround with native pabot 5.2 output flags is the right call, and the net code reduction is nice. A few things:
-
Same pattern as the directory names on #557 — the filenames
"output.xml","log.html","report.html","xunit.xml"are hardcoded inpabot.py,orchestrator.py,robot_generator.py,xunit_merger.py, and several test files. We already haveROBOT_RESULTS_DIRNAMEinconstants.py, so it'd be consistent to add filename constants there too (ROBOT_OUTPUT_XML,ROBOT_LOG_HTML, etc.) and reference them everywhere instead of scattering the strings. -
The phase numbering in
orchestrator.pygoes 1, 2, 3, 4, 5.5 after removing old Phase 4 — looks like Phase 5.5 just needs to become Phase 5. -
The comment
# (pabot 5.2+ writes directly to robot_results/, so no file moving needed)is slightly misleading — pabot doesn't do it automatically, we're telling it to via the explicit--output/--log/--reportflags. Something like# (output files written directly to robot_results/ via --output/--log/--report flags)would be more precise.
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.
Addresses review comment from PR #560.
…eta' into feature/pabot-5.2-upgrade
There was a problem hiding this comment.
Thanks for the review! All four points addressed:
-
Filename constants: Added
OUTPUT_XML,LOG_HTML,REPORT_HTMLtoconstants.pyand replaced all hardcoded strings across production code and tests (1ce1694, 89171fa, b339915).I went with the shorter names (e.g.,
OUTPUT_XMLinstead ofROBOT_OUTPUT_XML) as their role is apparent to everyone, andXUNIT_XMLis a shared name across both Robot and PyATS, soROBOT_XUNIT_XMLwould be incorrect/incomplete. -
Phase numbering: Fixed 5.5 → 5 (89171fa).
-
Comment clarification: Updated to
# (output files written directly to robot_results/ via --output/--log/--report flags)(89171fa).
aitestino
left a comment
There was a problem hiding this comment.
Thanks! Great job. Upon a 2nd glance spotted these but they are truly nitpicks, opened the issues as to not look the other way...but this is good to merge imo.
- #585 (ValueError test for non-numeric attributes)
- #586 (unexpected root element test)
- #587 (OSError handling test)
P.S. — This comment was drafted using voice-to-text. 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! 🙂
* test: Add automated tests for hostname display in D2D reporting 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 * feat: merge Robot and PyATS xunit.xml files for CI/CD integration (#555) 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 * refactor(robot): use Path for xunit output path construction 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. * tests: add defaults.apic.version to e2e fixtures to make ACI defaults check happy * refactor(e2e): remove unused hostname extraction functions 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. * refactor: consolidate hostname sanitization into shared utility 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. * refactor(e2e): validate expected_d2d_hostnames in E2EScenario 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. * refactor(xunit_merger): use directory constants instead of hardcoded 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. * fix(xunit_merger): handle ValueError for malformed xunit attributes 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. * fix(orchestrator): add resilient error handling for xunit merge 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. * refactor(subprocess_runner): extract command building into _build_command 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. * refactor: use XUNIT_XML constant instead of hardcoded "xunit.xml" Addresses review comment from PR #560.
…eta' into feature/pabot-5.2-upgrade
19e8130
into
release/pyats-integration-v1.1-beta
* Move archive discovery and report timing messages to logger.info Move 'Found API/D2D/legacy archive' messages and report generation timing from stdout to logger.info level. These diagnostic messages are procedural and only needed when debugging - users can see them with -v INFO flag. Fixes #549 * Move archive and report paths to logger.info, consolidate to combined summary Remove duplicate archive/report path output from stdout: - PyATS Output Files block (summary_printer.py) - HTML Reports Generated block (orchestrator.py) Paths now logged at INFO level (visible with -v INFO): - Archive paths, results JSON/XML, summary XML - HTML report directories and summary reports The combined summary remains as the single source of truth for finding test artifacts, reducing output clutter in CI pipelines. Fixes #545 * Consolidate test counts to combined summary only Remove individual framework summaries from stdout, keeping only the combined summary. Individual results (PyATS API/D2D, Robot) are now logged at INFO level, visible with -v INFO. - Delete SummaryPrinter class (no longer needed after #545/#549) - Add format_test_summary() to terminal.py with colored numbers - Refactor combined_orchestrator to use new formatter - Log individual framework results at INFO level Deleting SummaryPrinter also removes duplicate timing output (Total testing / Elapsed time), leaving only the single Total runtime at the end. Fixes #544 Fixes #546 * Streamline execution summary output - Move "Generating..." messages to logger.info - Simplify output section to show key result files only - Use file existence checks for robustness - Clean up separators (= for frame, - for divisions) Follow-up to #545 * Consolidate PyATS discovery output (#564) Reduce discovery output from 4 lines to 2 by including api/d2d breakdown in the discovery message and removing redundant "Found X API/D2D test(s)" messages. Before: Discovered 2 PyATS test files Running with 5 parallel workers Found 1 API test(s) - using standard execution Found 1 D2D test(s) - using device-centric execution After: Discovered 2 PyATS test files (1 api, 1 d2d) Running with 5 parallel workers * Move PyATS command output to logger.info Remove redundant print() of PyATS command - already logged via logger.info(), visible with -v INFO. * Remove redundant Robot Framework completion message The combined summary already shows test completion status. Individual framework completion messages are now suppressed to reduce stdout noise. * Add E2E and unit tests for #540 output streamlining Test philosophy: - E2E tests validate stdout structure, not exact wording - Tests use filtered_stdout (logger lines stripped) to isolate actual program output from debug logging - Liberal pattern matching catches regressions without breaking on minor wording changes Unit tests (test_terminal.py): - format_test_summary() coloring: numbers colored only when > 0 - Labels (passed/failed/skipped) never colored - NO_COLOR environment variable support - Error message formatting for controller auto-detection E2E stdout tests: - Combined Summary header present - Stats line in correct section (not pabot's duplicate) - No individual framework completion messages (robot/pyats) - Archive discovery messages moved to logger (not in stdout) - PyATS discovery shows consolidated count Also consolidates test_terminal_error_messages.py into test_terminal.py for single module per source file. * Use absolute paths in Combined Summary output Resolves paths to absolute when printing artifact locations, matching pabot's output style for consistency. * Upgrade robotframework-pabot to >=5.2.2 Part of #560 - included here for branch self-containment. * Fix test assertion for updated _print_execution_summary signature * suppress Template rendering completed/Creating merged data model msgs * add comment why we use absolute paths in our summary * Add visual spacing around Combined Summary block Add two blank lines before and after the Combined Summary block to improve readability and separate it from pabot output above and "Total runtime" below. - Add blank lines before opening '======' separator - Add blank line after closing '======' separator - Add E2E test to verify spacing is maintained * Remove timestamps from data model merging output Simplify CLI output by removing timestamps from the data merging messages, keeping only the duration. * Remove stray comment line in _print_execution_summary * Add 'other' count to format_test_summary when > 0 Display 'other' count (blocked/aborted/passx/info) in the Combined Summary when present. Uses magenta color for visibility. - Only shown when other > 0 to avoid clutter - Add unit tests for other count display * fix: remove merge regression of typer.echo in orchestrator The typer.echo("✅ Robot Framework tests completed") was intentionally removed as part of the output streamlining work, but crept back in during a merge from the parent branch. Our e2e tests caught this :-) Other typer.echo statements introduced by recent merges (error messages in main.py, UI banner module) are intentional and unaffected.
Description
Upgrade
robotframework-pabotto version 5.2.2 and eliminate the file-moving workaround by having pabot write output artifacts directly to therobot_results/subdirectory.Stack: This PR is stacked on top of #557 (xunit merger), which is stacked on top of #543 (tests for hostname display). the commit to review is 11c3a28
Closes
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
robotframework-pabotdependency from>=4.3.2to>=5.2.2--output,--log,--reportCLI options to direct artifacts torobot_results/subdirectory_move_robot_results_to_subdirectory()workaround from orchestratorshutilimport (no longer needed)Testing Done
pytest/pre-commit run -a)Test Commands Used
Checklist
pre-commit run -apasses)Screenshots (if applicable)
N/A
Additional Notes
Pabot 5.2+ supports specifying individual output file paths via
--output,--log,--report,--xunitoptions. This allows us to eliminate the post-execution file moving workaround that was previously needed to place artifacts in therobot_results/subdirectory.The
--outputdirmust remain pointing to the base path where robot test files are located, as pabot uses this for test discovery.