feat(cli): add --verbose and --loglevel flags, align env vars to NAC_TEST_ prefix

[oboehmer]

Description

Standardize internal environment variables to use NAC_TEST_PYATS_ prefix for consistency with the public NAC_TEST_ convention. Add --verbose CLI flag for easier troubleshooting. Implement --loglevel flag support to control log detail levels for both PyATS and Robot Framework.

Environment variable renames:

Old Name	New Name
PYATS_MAX_WORKERS	NAC_TEST_PYATS_PROCESSES
MAX_CONNECTIONS	NAC_TEST_PYATS_MAX_CONNECTIONS
NAC_API_CONCURRENCY	NAC_TEST_PYATS_API_CONCURRENCY
NAC_SSH_CONCURRENCY	NAC_TEST_PYATS_SSH_CONCURRENCY
PYATS_OUTPUT_BUFFER_LIMIT	NAC_TEST_PYATS_OUTPUT_BUFFER_LIMIT
KEEP_HTML_REPORT_DATA	NAC_TEST_PYATS_KEEP_REPORT_DATA
NAC_TEST_OVERFLOW_DIR	NAC_TEST_PYATS_OVERFLOW_DIR
PYATS_DEBUG	NAC_TEST_DEBUG

New --verbose flag behavior:

• Enables verbose output for pabot (--verbose) showing Robot console output
• Enables PyATS console output (without --verbose, PyATS output is suppressed)
• Implies --loglevel DEBUG unless explicitly overridden

--loglevel flag controls log detail level:

--loglevel	PyATS console output	PyATS internal loglevel	Robot --loglevel
WARNING (default)	Suppressed (unless --verbose)	WARNING	Not set (Robot default)
ERROR	Suppressed (unless --verbose)	ERROR	Not set
INFO	Suppressed (unless --verbose)	INFO	Not set
DEBUG	Suppressed (unless --verbose)	DEBUG	DEBUG

Key behaviors:

• PyATS console output requires --verbose. Without it, PyATS output is suppressed regardless of --loglevel.
• --verbose: Enables console output for both frameworks, implies --loglevel DEBUG
• --verbose --loglevel INFO: Shows PyATS console output filtered to INFO+, Robot uses default loglevel
• --verbose --loglevel ERROR: Shows PyATS console output filtered to ERROR+ only, Robot uses default loglevel
• --loglevel controls nac-test internal logging level in addition to framework output (independent of --verbose), no change in this PR

Deprecation notice:

• --verbosity is now a hidden alias for --loglevel and will be removed in a future version. Use --loglevel instead.

Breaking change:

• Robot Framework --loglevel can no longer be controlled via pass-through arguments. See README for workarounds using --variable and Set Log Level keyword.

PyATS output filtering is optimized for high-volume runs (benchmarked: 412k lines in <0.1s).

Note: To preserve intermediate archive/JSONL files for troubleshooting, set NAC_TEST_DEBUG=true or NAC_TEST_PYATS_KEEP_REPORT_DATA=true as environment variables. The --verbose CLI flag enables verbose output but does not retain these files.

The following env vars remain as undocumented internal tuning knobs, however I have moved all of them to nac_test/pyats_core/constants.py so we can manage them centrally. Due to this move I changed the respective internal constant name:

Environment Variable	Old Constant Name	New Constant Name
NAC_TEST_PYATS_PIPE_DRAIN_DELAY	PIPE_DRAIN_DELAY_SECONDS (inline parsing)	PIPE_DRAIN_DELAY_SECONDS
NAC_TEST_PYATS_PIPE_DRAIN_TIMEOUT	PIPE_DRAIN_TIMEOUT_SECONDS (inline parsing)	PIPE_DRAIN_TIMEOUT_SECONDS
NAC_TEST_PYATS_BATCH_SIZE	(inline in batching_reporter.py)	BATCH_SIZE
NAC_TEST_PYATS_BATCH_TIMEOUT	(inline in batching_reporter.py)	BATCH_TIMEOUT_SECONDS
NAC_TEST_PYATS_QUEUE_SIZE	(inline in batching_reporter.py)	OVERFLOW_QUEUE_SIZE
NAC_TEST_PYATS_MEMORY_LIMIT_MB	(inline in batching_reporter.py)	OVERFLOW_MEMORY_LIMIT_MB

Note: SENTINEL_TIMEOUT_SECONDS / NAC_TEST_PYATS_SENTINEL_TIMEOUT were introduced in d1c3865 but never wired into the subprocess runner. Removed entirely in commit 3ab23b3 within this PR — see that commit message for the full reasoning.

Closes

• Fixes Align environment variable naming conventions before FCS #512

Related Issue(s)

• Refactor: Decouple internal batching debug mode from user-facing NAC_TEST_DEBUG #600 (refactor batching reporter debug mode - post-FCS)

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactoring / Technical debt (internal improvements with no user-facing changes)
• Documentation update
• Chore (build process, CI, tooling, dependencies)
• Other (please describe):

Test Framework Affected

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

Network as Code (NaC) Architecture Affected

• ACI (APIC)
• NDO (Nexus Dashboard Orchestrator)
• NDFC / VXLAN-EVPN (Nexus Dashboard Fabric Controller)
• Catalyst SD-WAN (SDWAN Manager / vManage)
• Catalyst Center (DNA Center)
• ISE (Identity Services Engine)
• FMC (Firepower Management Center)
• Meraki (Cloud-managed)
• NX-OS (Nexus Direct-to-Device)
• IOS-XE (Direct-to-Device)
• IOS-XR (Direct-to-Device)
• Hyperfabric
• All architectures
• N/A (architecture-agnostic)

Platform Tested

• macOS (version tested: 15.3)
• Linux (distro/version tested: )

Key Changes

• Add --verbose CLI flag with env var support (NAC_TEST_VERBOSE)
• --verbose enables verbose output for both pabot and PyATS execution
• --verbose implies DEBUG loglevel unless --loglevel is explicitly set
• Add --loglevel CLI argument to control log level (DEBUG, INFO, WARNING, ERROR, CRITICAL)
• Deprecate --verbosity argument (hidden alias for --loglevel, will be removed in future version)
• PyATS: Console output only shown when --verbose is set; --loglevel filters what is shown
• PyATS: Respect --loglevel in generated job files via managed_handlers.screen.setLevel()
• Robot: Decouple verbose (pabot console) from loglevel (Robot --loglevel)
• Robot: Set --loglevel DEBUG only when nac-test --loglevel DEBUG
• BREAKING: Robot Framework --loglevel pass-through no longer supported; use --variable approach documented in README
• Add LogLevel enum class with comparison operators
• Update README with Verbose Mode and Advanced Environment Variables documentation
• Add unit tests for verbose/loglevel interaction, job generation, loglevel precedence
• Add E2E test scenarios to verify --verbose and --verbose --loglevel INFO behavior

Testing Done

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

Test Commands Used

uv run pytest -n auto --dist loadscope  # Full test suite
uv run pytest tests/unit/cli/ -v
uv run pytest tests/unit/robot/ -v
uv run pytest tests/unit/pyats_core/execution/ -v
uv run pytest tests/unit/utils/test_logging.py -v
uv run pytest tests/unit/test_pabot_args.py -v
uv run pytest tests/e2e -k verbose -v
uv run pre-commit run --all-files

Checklist

• Code follows project style guidelines (pre-commit run -a passes)
• Self-review of code completed
• Code is commented where necessary (especially complex logic)
• Documentation updated (if applicable)
• No new warnings introduced
• Changes work on both macOS and Linux
• CHANGELOG.md updated (if applicable)

[aitestino]

Hey @oboehmer, thank you for the PR — the motivation here is solid. Standardizing the env var naming to the NAC_TEST_* prefix, adding --verbose, and introducing --loglevel are all moves in the right direction for consistency and usability. THANK YOU!!!

I did a comprehensive review on this, and I think we need some changes before merge. The implementation introduces a few patterns that don't align with our existing codebase conventions, and more critically, there are some runtime-breaking issues and incomplete refactoring steps that would cause the application to fail on startup.

TLDR-ish:

1. Runtime TypeError: configure_logging() signature mismatch

The diff shows configure_logging(level: str | LogLevel) -> None (removing the error_handler parameter), but the current utils/logging.py on disk still has the 2-parameter signature with error_handler: errorhandler.ErrorHandler. The diff also doesn't show removal of the error_handler.reset() call in the function body (line 57-58 of current file).

Meanwhile, cli/main.py in the diff calls it with just one argument: configure_logging(effective_loglevel).

If we apply this diff as-is, we'll get either a TypeError (wrong number of arguments) or NameError: name 'error_handler' is not defined at runtime.

What I'm wondering: Was the error_handler integration intentionally removed? If so, we need to:

• Remove the error_handler.reset() call from the function body
• Remove the import errorhandler from logging.py
• Remove the error_handler = ErrorHandler() instantiation from main.py (line 377 currently checks error_handler.fired)

Or if we still need it, restore the parameter to the signature.

---

2. Runtime TypeError: CombinedOrchestrator doesn't accept loglevel= and verbose= kwargs

The diff adds to cli/main.py:

orchestrator = CombinedOrchestrator(
    ...
    loglevel=effective_loglevel,  # NEW
    verbose=verbose,              # NEW
)

But the current CombinedOrchestrator.__init__() signature accepts verbosity: VerbosityLevel, not loglevel or verbose.

This will raise TypeError: __init__() got unexpected keyword argument 'loglevel' on startup.

Options:

• Update CombinedOrchestrator.__init__ to accept loglevel and verbose, OR
• Pass verbosity=effective_loglevel instead (though we'd need to ensure type compatibility since effective_loglevel is LogLevel and the signature expects VerbosityLevel)

---

3. Six new constants defined but never wired up to their consumers (dead code)

The PR adds these constants to pyats_core/constants.py:

PYATS_OUTPUT_BUFFER_LIMIT = get_positive_numeric_env("NAC_TEST_PYATS_OUTPUT_BUFFER_LIMIT", ...)
BATCH_SIZE = get_positive_numeric_env("NAC_TEST_PYATS_BATCH_SIZE", ...)
BATCH_TIMEOUT_SECONDS = get_positive_numeric_env("NAC_TEST_PYATS_BATCH_TIMEOUT", ...)
OVERFLOW_QUEUE_SIZE = get_positive_numeric_env("NAC_TEST_PYATS_QUEUE_SIZE", ...)
OVERFLOW_MEMORY_LIMIT_MB = get_positive_numeric_env("NAC_TEST_PYATS_MEMORY_LIMIT_MB", ...)
OVERFLOW_DIR_OVERRIDE = os.environ.get("NAC_TEST_PYATS_OVERFLOW_DIR")

And exports them in __all__.

But: The actual consumers aren't updated to import/use them:

• subprocess_runner.py still imports DEFAULT_BUFFER_LIMIT and reads os.environ.get("PYATS_OUTPUT_BUFFER_LIMIT") (the old env var name)
• batching_reporter.py still reads os.environ.get("NAC_TEST_BATCH_SIZE") directly (note: different env var name than what the constant reads — NAC_TEST_BATCH_SIZE vs NAC_TEST_PYATS_BATCH_SIZE)

Impact: If a user sets NAC_TEST_PYATS_OUTPUT_BUFFER_LIMIT or NAC_TEST_PYATS_BATCH_SIZE, those values are silently ignored because nothing actually consumes these constants.

What do you think? We could either:

1. Wire up all 6 constants to their consumers in this PR (update imports in subprocess_runner.py and batching_reporter.py), OR
2. Remove them from this PR and add them when we're ready to actually integrate them

Also, we need to align on env var naming: should it be NAC_TEST_BATCH_SIZE or NAC_TEST_PYATS_BATCH_SIZE?

---

4. NO_TESTLEVELSPLIT constant has wrong semantics and is never imported

The PR adds:

# core/constants.py
NO_TESTLEVELSPLIT = bool(os.environ.get("NAC_TEST_NO_TESTLEVELSPLIT"))

But robot/orchestrator.py still uses:

if "NAC_TEST_NO_TESTLEVELSPLIT" not in os.environ:

These have different semantics:

# User sets empty string
export NAC_TEST_NO_TESTLEVELSPLIT=""

# Old code: "VAR" in os.environ → True (split disabled)
# New constant: bool(os.environ.get("VAR")) → False (split enabled!)

Since the constant is never actually imported by robot/orchestrator.py, it's dead code right now. But if we wire it up later, behavior will silently change for the empty-string case.

What I'm suggesting:

• Use "NAC_TEST_NO_TESTLEVELSPLIT" in os.environ for semantic equivalence with the current behavior, OR
• Document that empty string now means "enabled" (breaking change), OR
• Just delete the constant if we're not ready to wire it up yet

Also: align with the DEBUG_MODE pattern in the same file, which uses .lower() in ("true", "1", "yes") instead of bool().

---

5. Incomplete VerbosityLevel → LogLevel rename causing ImportError

The rename is applied to:

• nac_test/utils/logging.py (class definition)
• nac_test/cli/main.py (import and usage)
• nac_test/utils/__init__.py (export)

But NOT applied to:

• nac_test/robot/orchestrator.py — still imports VerbosityLevel
• nac_test/combined_orchestrator.py — still imports VerbosityLevel
• tests/unit/robot/test_orchestrator.py — still imports VerbosityLevel

Result: ImportError: cannot import name 'VerbosityLevel' from 'nac_test.utils.logging'

What I'm suggesting (pick one):

Option A (Recommended): Add a backward compatibility alias:

# nac_test/utils/logging.py
class LogLevel(str, Enum):
    # ... implementation

# Backward compatibility - DEPRECATED, remove in v3.0.0
VerbosityLevel = LogLevel

Option B: Expand this PR to complete the global rename across all files

Option C: Split into 2 PRs: first add the alias, then migrate all imports

From a best practices standpoint, I think Option A is safest — it gives us backward compatibility during the transition period and we can remove the alias in a future major version.

---

6. Missing test coverage for new LogLevel comparison operators

The PR adds __le__, __lt__, __ge__, __gt__ methods to LogLevel, but there are no tests verifying:

• Comparison correctness: LogLevel.DEBUG < LogLevel.INFO
• Integration with Python logging: int(LogLevel.DEBUG) == logging.DEBUG
• Edge cases: LogLevel.DEBUG <= LogLevel.DEBUG

What I'm suggesting: Add a new test file tests/unit/utils/test_logging.py:

def test_loglevel_ordering():
    assert LogLevel.DEBUG < LogLevel.INFO < LogLevel.WARNING < LogLevel.ERROR < LogLevel.CRITICAL

def test_loglevel_int_conversion():
    assert int(LogLevel.DEBUG) == 10
    assert int(LogLevel.INFO) == 20

---

7. get_positive_numeric_env() silently ignores invalid input

When a user sets an invalid env var value:

export NAC_TEST_PYATS_API_CONCURRENCY=abc

The helper silently returns the default (55) with no warning or feedback.

What I'm suggesting: Add optional warning logging:

def get_positive_numeric_env(
    env_var: str, default: T, value_type: type[T], warn_on_invalid: bool = True
) -> T:
    env_value = os.environ.get(env_var)
    if env_value is None:
        return default
    
    try:
        value = value_type(env_value)
        if value > 0:
            return value
        if warn_on_invalid:
            logger.warning(f"{env_var}={env_value} is not positive, using default {default}")
    except (ValueError, TypeError):
        if warn_on_invalid:
            logger.warning(f"{env_var}={env_value} is not a valid {value_type.__name__}, using default {default}")
    return default

This follows the "fail visibly" principle — users should know when their configuration is being ignored.

---

8. get_positive_numeric_env() placement in constants.py violates SRP

The function is placed in core/constants.py, and the comment even acknowledges this: "lives here (not in utils/) to avoid circular imports".

Circular imports are an architectural symptom, not a justification for putting functions in constants files. constants.py should only contain constants per our conventions.

What I'm suggesting: Create nac_test/utils/env.py and move the function there. If there's a circular import issue, let's solve it properly rather than work around it by polluting the constants module.

---

9. Type annotation coverage: 26 constants missing annotations

Looking at core/constants.py and pyats_core/constants.py, there are 26 constants without type annotations. I thiiiink general project conventions we have been following dictate something like: "Named Constants: Type-annotated, SCREAMING_SNAKE_CASE."

For example:

# Current (unannotated):
RETRY_MAX_ATTEMPTS = 3
DEFAULT_TEST_TIMEOUT = 21600

# Required:
RETRY_MAX_ATTEMPTS: int = 3
DEFAULT_TEST_TIMEOUT: int = 21600

Only IS_MACOS: bool and IS_UNSUPPORTED_MACOS_PYTHON: bool have annotations currently.

I realize these might be pre-existing, but since we're touching both constants files in this PR, would be good to bring them into compliance.

---

10. Consider @functools.total_ordering to reduce type: ignore comments

LogLevel has four comparison methods with four # type: ignore[override] comments. We can reduce this to just one by using @functools.total_ordering:

from functools import total_ordering

@total_ordering
class LogLevel(str, Enum):
    def __lt__(self, other: "LogLevel") -> bool:  # type: ignore[override]
        return int(self) < int(other)
    # __le__, __gt__, __ge__ auto-generated by decorator

This reduces mypy blind spots from 4 locations to 1.

---

11. Missing module docstring and function docstrings

• cli/main.py is missing a module-level docstring (all other files in this batch have one)
• version_callback() is missing a docstring

Both are required per our conventions.

---

12. Minor: NO_TESTLEVELSPLIT boolean parsing inconsistency

NO_TESTLEVELSPLIT uses bool(os.environ.get(...)) while DEBUG_MODE in the same file uses .lower() == "true".

The bool() approach has surprising behavior — any non-empty string is truthy, including "0" and "false".

For consistency: Use the pattern from DEBUG_MODE:

NO_TESTLEVELSPLIT = os.environ.get("NAC_TEST_NO_TESTLEVELSPLIT", "").lower() in ("true", "1", "yes")

---

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!

[aitestino]

See above.

[oboehmer]

Thanks for the review, @aitestino ! I've addressed comments 7-12:

Comment 7 - Warning logging for invalid env vars: Added warning logs to get_positive_numeric_env() when env var is set but invalid/non-positive. Controlled via warn_on_invalid parameter (default: True) 023482e

Comment 8 - Relocate helper: Moved get_positive_numeric_env() to nac_test/_env.py at package root. Placed there instead of utils/ to avoid circular import issues with utils/__init__.py re-exports. I also raised #610 to review the __all__ re-export strategy as they aren't consistently applied across the codebase (023482e)

Comment 9 - Type annotations: Added explicit type annotations to all public constants in core/constants.py and pyats_core/constants.py (139cf97)

Comment 10 - @functools.total_ordering: Investigated this - unfortunately it doesn't work with (str, Enum) inheritance. The decorator adds generated comparison methods, but str already defines __gt__, __ge__, etc. and takes MRO precedence. With @total_ordering applied, LogLevel.CRITICAL > LogLevel.ERROR returns False (alphabetic comparison) instead of using our severity-based ordering. The explicit overrides with # type: ignore[override] seem to be required.

Comment 11 - Docstrings: Added module docstring and version_callback() docstring to cli/main.py (50d68ab)

Comment 12 - get_bool_env() and constant rename: Added get_bool_env() helper for consistent boolean parsing (accepts true/yes/1). Renamed NO_TESTLEVELSPLIT → DISABLE_TESTLEVELSPLIT for clarity. (4f80bce) Also added BATCHING_REPORTER_ENABLED constant to pyats_core/constants.py (64926a9)

---

Regarding comments 1-6: Not sure what state you were reviewing, but none of these issues are present on the current branch. For example, configure_logging() has only the single level parameter, CombinedOrchestrator.__init__() already accepts loglevel= and verbose=, and VerbosityLevel has been fully renamed to LogLevel across all files including robot/orchestrator.py and combined_orchestrator.py.

I would appreciate a quick re-check so we can unblock the alpha release.

[aitestino]

Hey @oboehmer, thank you for the detailed response and for the patience on this.

You're right about comments 1-6 — I went back and verified each one against e960c6c, and the code was already in the correct state when I reviewed. It looks like my review was comparing against a stale base rather than looking at the actual branch files. That's my mistake, and I apologize for the noise and the delay it caused.

Your work on comments 7-12 looks great:

1. get_positive_numeric_env() warning logging (comment 7) — Clean implementation with the warn_on_invalid kwarg. Tests cover all four paths (valid, invalid, non-positive, disabled).

2. _env.py relocation (comment 8) — The underscore-prefix convention at package root is a pragmatic solution given the real circular import constraint. The comment explaining why it lives there instead of utils/ is appreciated, and filing #610 for the broader re-export discussion is the right call.

3. Type annotations (comment 9) — All 26 constants annotated across both files. Complete.

4. @total_ordering investigation (comment 10) — You're correct that it doesn't work with (str, Enum) inheritance. I verified independently — str.__gt__ takes MRO precedence over the decorator-generated method, so LogLevel.CRITICAL > LogLevel.ERROR returns False (alphabetic). The explicit overrides are necessary.

5. Docstrings (comment 11) — Complete.

6. get_bool_env() and DISABLE_TESTLEVELSPLIT (comment 12) — This went beyond the fix by also improving the constant name and properly documenting the breaking change in CHANGELOG. The parametrized tests covering 13 input combinations are solid.

Also noticed the BATCHING_REPORTER_ENABLED constant wiring into step_interceptor.py — good consistency with the overall centralization goal.

LGTM — let's unblock the alpha release.

---

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! 🙂