Skip to content

fix(cli): validate required args before running --diagnostic#741

Open
oboehmer wants to merge 11 commits intomainfrom
fix/664-zip-diagnostic
Open

fix(cli): validate required args before running --diagnostic#741
oboehmer wants to merge 11 commits intomainfrom
fix/664-zip-diagnostic

Conversation

@oboehmer
Copy link
Copy Markdown
Collaborator

@oboehmer oboehmer commented Apr 4, 2026

Description

This PR improves --diagnostic behavior and the bundled diagnostic collection script to make diagnostics safer, more accurate, and easier to consume.

CLI --diagnostic behavior (Fixes #740)

  • Move --diagnostic handling out of the Typer callback and into nac_test/cli/main.py so required args are validated by Typer/Click before diagnostics run.
  • Introduce run_diagnostic(...) (annotated NoReturn) that runs the diagnostic script and exits with the script’s return code.
  • Reject Windows early for --diagnostic.

Diagnostic script improvements (Fixes #664 + follow-ups)

  • Fail fast with a clear message when zip command is missing (observed on WSL2 environment)
  • Fix wrapped command exit code reporting (avoid incorrect PIPESTATUS usage).
  • Create zip archives with relative paths (avoid embedding full /var/... prefixes in the archive contents).
  • Use pyats version check instead of pyats.__version__ for reliable version capture.
  • Expect the wrapped nac-test command as a single quoted argument ($1) and enforce the contract.
  • Update Robot Framework artifact collection to use $OUTPUT_DIR/robot_results, and collect robot outputs via a single find pass.
  • Clarify log messaging around where nac-test output is captured.

Tests

  • Update tests/unit/cli/test_diagnostic.py to match the new run_diagnostic entrypoint.
  • Tighten _reconstruct_command coverage with strict shlex.join/shlex.split round-trip assertions.
  • Add fixtures for a canonical argv including required options and a variant containing spaces.
  • Silence a mypy false-positive around NoReturn in pytest.raises.

Closes

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature
  • Breaking change
  • Refactoring / Technical debt
  • Documentation update
  • Chore
  • Other (please describe):

Test Framework Affected

  • PyATS
  • Robot Framework
  • Both
  • N/A (not test-framework specific)

NaC Architecture Affected

  • N/A (architecture-agnostic)

Platform Tested

  • macOS (local dev)
  • Linux (distro/version tested: )

Key Changes

  • Validate required CLI args before running diagnostics (--diagnostic no longer bypasses missing options).
  • Ensure diagnostic summary reports the wrapped command’s real exit code.
  • Improve diagnostic archive portability (relative zip paths) and robustness (zip preflight, stricter command arg handling, PyATS version capture).
  • Adjust artifact collection paths for Robot Framework.

Testing Done

  • Unit tests added/updated
  • Integration tests performed
  • Manual testing performed:
    • PyATS tests executed successfully
    • Robot Framework tests executed successfully
    • HTML reports generated correctly
  • All existing tests pass (pytest / pre-commit run -a)

Test Commands Used

python -m pytest -q
pre-commit run -a

Additional Notes

  • Diagnostic command is passed to the script as a single shell-safe string (from shlex.join), and the script enforces that it receives exactly one command argument.
  • Robot outputs are now expected under $OUTPUT_DIR/robot_results.

oboehmer added 10 commits April 3, 2026 22:15
- Add early `zip` preflight in diagnostic collection script (with install hints)
- Hard-error on Windows when `--diagnostic` is used (exit code 2)
- Clarify `--diagnostic` help text as Linux/macOS-only
- Add unit test to ensure Windows gating avoids running subprocess
Handle --diagnostic inside main() after Typer/Click parsing so missing required
options (-d/-t) fail normally. Run the diagnostic script via run_diagnostic()
and exit with its return code; hard-error on Windows.
Add targeted `type: ignore[unreachable]` on assertions after calling
run_diagnostic (annotated NoReturn) inside pytest.raises blocks, so the
pre-commit mypy hook passes.
Introduce base_argv/base_argv_with_spaces fixtures and tighten reconstruct
tests to assert shlex round-trips exactly. Reuse base_argv in TestRunDiagnostic
to avoid ad-hoc argv construction.
Create the diagnostic zip from the parent directory so the archive contains
only the diagnostic folder (no full /var/... path prefixes).
Require the nac-test command to be passed as a single quoted argument ($1)
and fail fast otherwise. Also clarify the log message that nac-test output is
not shown on screen and is written to 100_nac_test_execution.txt.
Compute Robot Framework output file list once from $OUTPUT_DIR/robot_results
and reuse it for both logging and copying, avoiding duplicate filesystem scans.
@oboehmer oboehmer marked this pull request as ready for review April 6, 2026 14:20
@oboehmer oboehmer requested a review from aitestino April 6, 2026 14:21
@oboehmer oboehmer self-assigned this Apr 8, 2026
@oboehmer oboehmer added the bug Something isn't working label Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: --diagnostic bypasses required args and reports wrong exit code [bug] v 2.0.0a2 - Diagnostic collection fails

1 participant