Skip to content

Commit 6956343

Browse files
memadi-nvmemadi-nv
authored
feat: add exit code tests for CLI error paths (#446)
<!-- SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> <!-- SPDX-License-Identifier: Apache-2.0 --> <!-- Thank you for contributing to Safe Synthesizer! --> # Summary Adds regression tests that assert the CLI returns a non-zero exit code on common error paths for `run` and `config validate`. Follow-up to PR #359 / issue #333, which fixed `artifacts clean` silently exiting 0 on failure -- this PR extends the same guarantee to the rest of the CLI. ## Tests added `tests/cli/test_run.py` -- new `TestRunErrorPathExitCodes` class: - `run` with no `--data-source` (ClickException) - `run --data-source missing.csv` (FileNotFoundError) - `run --data-source bad.xyz` (unsupported extension, ValueError) - `run generate --run-path /nonexistent` (ClickException) `tests/cli/test_config.py` -- new `TestConfigValidateErrorPathExitCodes` class: - `config validate --config nonexistent.yaml` (FileNotFoundError) - `config validate --config malformed.yaml` (ValidationError -> `sys.exit(1)`) - `config validate --config broken.yaml` (yaml.YAMLError) - `config validate` with no `--config` (Click UsageError) - `config validate --config valid.yaml` (positive control, exits 0) ## Notes - Tests assert `exit_code != 0` rather than a specific value, since failures legitimately surface as `1` (ClickException / unhandled) or `2` (UsageError) depending on the path. `FileNotFoundError` and `ValueError` paths additionally pin the exception type via `isinstance(result.exception, ...)` to catch regressions where the exception is swallowed or wrapped. - The new `run` error-path tests skip `patched_run_dependencies` so they exercise the real `common_setup` flow. An autouse fixture resets the `_INITIALIZED_OBSERVABILITY` module flag and clears the `NSS_PHASE` env var between tests, since `common_setup` mutates both as process-global state and would otherwise leak across xdist workers. ## Pre-Review Checklist - [x] `make format && make check` / prek - [x] `make test` passes locally (`tests/cli/` -- 192 passed) - [ ] `make test-e2e` passes locally - [ ] `make test-ci-container` passes locally - [ ] GPU CI status check passes ## Pre-Merge Checklist - [x] New tests for the behavior being protected - [ ] Updated documentation (n/a -- tests only) ## Other Notes <!-- Please add the issue number that should be closed when this PR is merged. --> - Closes #383 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Tests * Added validation tests for the `config` command covering missing files, malformed YAML, unparseable YAML, and missing required options * Added error handling tests for the `run` command across multiple failure scenarios <!-- review_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/NVIDIA-NeMo/Safe-Synthesizer/pull/446) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: memadi <memadi@nvidia.com>
1 parent 92d4375 commit 6956343

2 files changed

Lines changed: 223 additions & 0 deletions

File tree

‎tests/cli/test_config.py‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
# SPDX-License-Identifier: Apache-2.0
3+
4+
"""Tests for the ``safe-synthesizer config`` command group.
5+
6+
Focuses on error-path exit codes: every failure mode must produce a non-zero
7+
exit code so automation and scripts that check ``$?`` can detect problems.
8+
"""
9+
10+
from __future__ import annotations
11+
12+
from pathlib import Path
13+
14+
import pytest
15+
from click.testing import CliRunner
16+
17+
from nemo_safe_synthesizer.cli.config import config
18+
19+
20+
@pytest.fixture
21+
def cli_runner() -> CliRunner:
22+
"""Create a Click CLI test runner."""
23+
return CliRunner()
24+
25+
26+
class TestConfigValidateErrorPathExitCodes:
27+
"""Tests that config validate error paths exit with non-zero status."""
28+
29+
def test_validate_nonexistent_config_exits_nonzero(
30+
self,
31+
cli_runner: CliRunner,
32+
tmp_path: Path,
33+
):
34+
"""`config validate --config nonexistent.yaml` must fail when the file doesn't exist."""
35+
missing_config = tmp_path / "does_not_exist.yaml"
36+
37+
result = cli_runner.invoke(
38+
config,
39+
[
40+
"validate",
41+
"--config",
42+
str(missing_config),
43+
],
44+
)
45+
46+
assert result.exit_code != 0
47+
assert isinstance(result.exception, FileNotFoundError)
48+
49+
def test_validate_malformed_config_exits_nonzero(
50+
self,
51+
cli_runner: CliRunner,
52+
tmp_path: Path,
53+
):
54+
"""`config validate --config malformed.yaml` must fail when the YAML fails schema validation."""
55+
malformed_config = tmp_path / "malformed.yaml"
56+
# YAML parses cleanly, but the field values violate SafeSynthesizerParameters:
57+
# ``training.batch_size`` must be a positive integer, and ``data.holdout`` must
58+
# be a number in [0, 1]. Either one alone is enough to trigger a ValidationError.
59+
malformed_config.write_text("training:\n batch_size: -1\ndata:\n holdout: not_a_number\n")
60+
61+
result = cli_runner.invoke(
62+
config,
63+
[
64+
"validate",
65+
"--config",
66+
str(malformed_config),
67+
],
68+
)
69+
70+
assert result.exit_code != 0
71+
72+
def test_validate_unparseable_yaml_exits_nonzero(
73+
self,
74+
cli_runner: CliRunner,
75+
tmp_path: Path,
76+
):
77+
"""`config validate --config garbage.yaml` must fail when the file is not valid YAML."""
78+
broken_yaml = tmp_path / "broken.yaml"
79+
# Unbalanced brackets trigger yaml.YAMLError during safe_load.
80+
broken_yaml.write_text("training: {batch_size: 1\n")
81+
82+
result = cli_runner.invoke(
83+
config,
84+
[
85+
"validate",
86+
"--config",
87+
str(broken_yaml),
88+
],
89+
)
90+
91+
assert result.exit_code != 0
92+
93+
def test_validate_missing_required_config_flag_exits_nonzero(
94+
self,
95+
cli_runner: CliRunner,
96+
):
97+
"""`config validate` without `--config` must fail because the option is required."""
98+
result = cli_runner.invoke(config, ["validate"])
99+
100+
assert result.exit_code != 0
101+
102+
def test_validate_valid_config_exits_zero(
103+
self,
104+
cli_runner: CliRunner,
105+
tmp_path: Path,
106+
fixture_yaml_config_str: str,
107+
):
108+
"""Positive control: a valid YAML config must exit 0.
109+
110+
Guards against a regression where ``validate`` starts returning a
111+
non-zero exit code for correct input, which would invert the contract
112+
these error-path tests protect.
113+
"""
114+
valid_config = tmp_path / "valid.yaml"
115+
valid_config.write_text(fixture_yaml_config_str)
116+
117+
result = cli_runner.invoke(
118+
config,
119+
[
120+
"validate",
121+
"--config",
122+
str(valid_config),
123+
],
124+
)
125+
126+
assert result.exit_code == 0

‎tests/cli/test_run.py‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import pytest
1111
from click.testing import CliRunner
1212

13+
import nemo_safe_synthesizer.observability as obs
1314
import nemo_safe_synthesizer.sdk.library_builder # noqa: F401 - ensure submodule is loaded for mock.patch
1415
from nemo_safe_synthesizer.cli.run import run
1516
from nemo_safe_synthesizer.cli.settings import CLISettings
@@ -909,3 +910,99 @@ def test_auto_param_override_reaches_params_object(
909910
resolved = getattr(resolved, key)
910911
assert resolved == expected
911912
assert type(resolved) is type(expected)
913+
914+
915+
class TestRunErrorPathExitCodes:
916+
"""Tests that run command error paths exit with non-zero status."""
917+
918+
@pytest.fixture(autouse=True)
919+
def _reset_observability(self, monkeypatch: pytest.MonkeyPatch) -> None:
920+
# These tests invoke the real common_setup, which calls
921+
# initialize_observability() and flips the module-level
922+
# _INITIALIZED_OBSERVABILITY flag. Without this reset, get_logger()
923+
# in subsequent tests on the same xdist worker returns a
924+
# CategoryLogger wrapping a structlog BoundLogger, and stdlib
925+
# LoggerAdapter.isEnabledFor() then fails with AttributeError.
926+
monkeypatch.setattr(obs, "_INITIALIZED_OBSERVABILITY", False)
927+
monkeypatch.delenv("NSS_PHASE", raising=False)
928+
929+
def test_run_with_no_data_source_exits_nonzero(
930+
self,
931+
cli_runner: CliRunner,
932+
tmp_path: Path,
933+
):
934+
"""`run` without --data-source must fail with a ClickException."""
935+
result = cli_runner.invoke(
936+
run,
937+
[
938+
"--artifact-path",
939+
str(tmp_path / "artifacts"),
940+
],
941+
)
942+
943+
assert result.exit_code != 0
944+
945+
def test_run_with_nonexistent_data_source_exits_nonzero(
946+
self,
947+
cli_runner: CliRunner,
948+
tmp_path: Path,
949+
):
950+
"""`run --data-source missing.csv` must fail when the file doesn't exist."""
951+
missing_csv = tmp_path / "does_not_exist.csv"
952+
953+
result = cli_runner.invoke(
954+
run,
955+
[
956+
"--data-source",
957+
str(missing_csv),
958+
"--artifact-path",
959+
str(tmp_path / "artifacts"),
960+
],
961+
)
962+
963+
assert result.exit_code != 0
964+
assert isinstance(result.exception, FileNotFoundError)
965+
966+
def test_run_with_unsupported_data_source_extension_exits_nonzero(
967+
self,
968+
cli_runner: CliRunner,
969+
tmp_path: Path,
970+
):
971+
"""`run --data-source bad.xyz` must fail for an unsupported file extension."""
972+
bad_source = tmp_path / "bad_source.xyz"
973+
bad_source.write_text("irrelevant contents")
974+
975+
result = cli_runner.invoke(
976+
run,
977+
[
978+
"--data-source",
979+
str(bad_source),
980+
"--artifact-path",
981+
str(tmp_path / "artifacts"),
982+
],
983+
)
984+
985+
assert result.exit_code != 0
986+
assert isinstance(result.exception, ValueError)
987+
988+
def test_generate_with_nonexistent_run_path_exits_nonzero(
989+
self,
990+
cli_runner: CliRunner,
991+
dummy_csv: Path,
992+
tmp_path: Path,
993+
):
994+
"""`run generate --run-path /nonexistent` must fail with a ClickException."""
995+
missing_run = tmp_path / "no_such_run"
996+
997+
result = cli_runner.invoke(
998+
run,
999+
[
1000+
"generate",
1001+
"--data-source",
1002+
str(dummy_csv),
1003+
"--run-path",
1004+
str(missing_run),
1005+
],
1006+
)
1007+
1008+
assert result.exit_code != 0

0 commit comments

Comments
 (0)