From fa6f000873a5eaad70b5be1ffd6664ca4f8f2d2e Mon Sep 17 00:00:00 2001 From: Nico Loesch Date: Fri, 10 Jul 2026 04:54:14 +0000 Subject: [PATCH 1/2] Render error better and include test cases for it --- omop_alchemy/maintenance/_cli_utils.py | 9 ++++++ tests/test_cli_error_handling.py | 39 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 tests/test_cli_error_handling.py diff --git a/omop_alchemy/maintenance/_cli_utils.py b/omop_alchemy/maintenance/_cli_utils.py index 2abe24b..576e547 100644 --- a/omop_alchemy/maintenance/_cli_utils.py +++ b/omop_alchemy/maintenance/_cli_utils.py @@ -123,6 +123,15 @@ def ensure_schema(engine: sa.Engine, schema: str | None) -> None: def handle_error(exc: Exception) -> None: + if isinstance(exc, FileNotFoundError): + console.print( + render_error( + "No omop-alchemy configuration found. " + "Run `omop-config configure omop_alchemy` to set it up.", + title="Not configured", + ) + ) + raise typer.Exit(code=1) from exc if isinstance(exc, BackendNotSupportedError): console.print(render_error(f"Not supported: {exc}")) raise typer.Exit(code=1) from exc diff --git a/tests/test_cli_error_handling.py b/tests/test_cli_error_handling.py new file mode 100644 index 0000000..c6461da --- /dev/null +++ b/tests/test_cli_error_handling.py @@ -0,0 +1,39 @@ +"""Tests for handle_error's FileNotFoundError branch (missing oa-configurator config).""" + +import pytest +import typer + +from omop_alchemy.maintenance._cli_utils import handle_error +from omop_alchemy.maintenance.ui import console + + +@pytest.fixture(autouse=True) +def _wide_console(): + """Rich wraps/truncates Panel text at the console's default 80-column + width, which is narrower than this message under pytest's captured, + non-tty output. Widen it so assertions can match the full text.""" + original_width = console.width + console.width = 200 + yield + console.width = original_width + + +def test_file_not_found_exits_with_code_1(): + with pytest.raises(typer.Exit) as exc_info: + handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) + assert exc_info.value.exit_code == 1 + + +def test_file_not_found_prints_configure_hint(capsys): + with pytest.raises(typer.Exit): + handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) + captured = capsys.readouterr() + assert "omop-config configure omop_alchemy" in captured.out + + +def test_file_not_found_does_not_leak_raw_traceback_path(capsys): + """The friendly message should not surface the raw exception text.""" + with pytest.raises(typer.Exit): + handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) + captured = capsys.readouterr() + assert "/home/cava/.config/omop/config.toml" not in captured.out From a1595b4976347905e0e08a8dc54060b39edf04a7 Mon Sep 17 00:00:00 2001 From: Nico Loesch Date: Mon, 20 Jul 2026 01:00:10 +0000 Subject: [PATCH 2/2] Raise the error at the right location instead of generic FileNotFoundError --- omop_alchemy/config.py | 13 +++++- omop_alchemy/maintenance/_cli_utils.py | 9 ---- omop_alchemy/maintenance/cli_backup.py | 10 ++++ tests/test_cli_error_handling.py | 63 ++++++++++++++++++++------ 4 files changed, 71 insertions(+), 24 deletions(-) diff --git a/omop_alchemy/config.py b/omop_alchemy/config.py index 1f8f0c9..8839fe0 100644 --- a/omop_alchemy/config.py +++ b/omop_alchemy/config.py @@ -86,8 +86,19 @@ def get_cdm_context() -> tuple[OmopAlchemyConfig, ResolvedResource]: The resource is taken from tools.omop_alchemy.default_resource when set; otherwise falls back to the canonical CDM_DB resource name. + + Raises + ------ + RuntimeError + If no oa-configurator stack config file exists yet. """ - stack = load_stack_config() + try: + stack = load_stack_config() + except FileNotFoundError as exc: + raise RuntimeError( + "No omop-alchemy configuration found. " + "Run `omop-config configure omop_alchemy` to set it up." + ) from exc pkg_config = OmopAlchemyConfig.from_stack(stack) tool = stack.tools.get(OmopAlchemyConfig.tool_name) resource_name = (tool.default_resource if tool else None) or OmopAlchemyConfig.CDM_DB.semantic_name diff --git a/omop_alchemy/maintenance/_cli_utils.py b/omop_alchemy/maintenance/_cli_utils.py index 576e547..2abe24b 100644 --- a/omop_alchemy/maintenance/_cli_utils.py +++ b/omop_alchemy/maintenance/_cli_utils.py @@ -123,15 +123,6 @@ def ensure_schema(engine: sa.Engine, schema: str | None) -> None: def handle_error(exc: Exception) -> None: - if isinstance(exc, FileNotFoundError): - console.print( - render_error( - "No omop-alchemy configuration found. " - "Run `omop-config configure omop_alchemy` to set it up.", - title="Not configured", - ) - ) - raise typer.Exit(code=1) from exc if isinstance(exc, BackendNotSupportedError): console.print(render_error(f"Not supported: {exc}")) raise typer.Exit(code=1) from exc diff --git a/omop_alchemy/maintenance/cli_backup.py b/omop_alchemy/maintenance/cli_backup.py index 8adbda6..abb28d5 100644 --- a/omop_alchemy/maintenance/cli_backup.py +++ b/omop_alchemy/maintenance/cli_backup.py @@ -83,6 +83,11 @@ def create_database_backup( raise RuntimeError( "Database backup failed via `pg_dump`." + (f" {stderr}" if stderr else "") ) from exc + except FileNotFoundError as exc: + raise RuntimeError( + f"`pg_dump` executable not found at {tool_path!r}. " + "It may have been removed from PATH after being resolved." + ) from exc return BackupResult( file_path=str(resolved_output_path), @@ -123,6 +128,11 @@ def restore_database_backup( raise RuntimeError( "Database restore failed." + (f" {stderr}" if stderr else "") ) from exc + except FileNotFoundError as exc: + raise RuntimeError( + f"Restore executable not found at {tool_path!r}. " + "It may have been removed from PATH after being resolved." + ) from exc return BackupResult( file_path=str(resolved_input_path), diff --git a/tests/test_cli_error_handling.py b/tests/test_cli_error_handling.py index c6461da..0916979 100644 --- a/tests/test_cli_error_handling.py +++ b/tests/test_cli_error_handling.py @@ -1,8 +1,16 @@ -"""Tests for handle_error's FileNotFoundError branch (missing oa-configurator config).""" +"""Tests for missing-config error handling. + +get_cdm_context() converts a missing oa-configurator stack file into a clear +RuntimeError at the source (config.py), rather than handle_error() pattern- +matching on the raw FileNotFoundError type. This keeps handle_error's +FileNotFoundError-shaped catch from also swallowing unrelated missing-file +errors raised elsewhere inside a command body (e.g. a missing pg_dump binary). +""" import pytest import typer +from omop_alchemy.config import get_cdm_context from omop_alchemy.maintenance._cli_utils import handle_error from omop_alchemy.maintenance.ui import console @@ -18,22 +26,49 @@ def _wide_console(): console.width = original_width -def test_file_not_found_exits_with_code_1(): - with pytest.raises(typer.Exit) as exc_info: - handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) - assert exc_info.value.exit_code == 1 +def _raise_file_not_found(): + raise FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml") -def test_file_not_found_prints_configure_hint(capsys): - with pytest.raises(typer.Exit): - handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) - captured = capsys.readouterr() - assert "omop-config configure omop_alchemy" in captured.out +def test_get_cdm_context_raises_runtime_error_when_config_missing(monkeypatch): + monkeypatch.setattr("omop_alchemy.config.load_stack_config", _raise_file_not_found) + with pytest.raises(RuntimeError, match="omop-config configure omop_alchemy"): + get_cdm_context() -def test_file_not_found_does_not_leak_raw_traceback_path(capsys): +def test_get_cdm_context_runtime_error_does_not_leak_raw_path(monkeypatch): """The friendly message should not surface the raw exception text.""" - with pytest.raises(typer.Exit): - handle_error(FileNotFoundError("Config file not found: /home/cava/.config/omop/config.toml")) + monkeypatch.setattr("omop_alchemy.config.load_stack_config", _raise_file_not_found) + with pytest.raises(RuntimeError) as exc_info: + get_cdm_context() + assert "/home/cava/.config/omop/config.toml" not in str(exc_info.value) + + +def test_get_cdm_context_runtime_error_chains_original_exception(monkeypatch): + monkeypatch.setattr("omop_alchemy.config.load_stack_config", _raise_file_not_found) + with pytest.raises(RuntimeError) as exc_info: + get_cdm_context() + assert isinstance(exc_info.value.__cause__, FileNotFoundError) + + +def test_handle_error_runtime_error_exits_with_code_1_and_prints_hint(capsys): + """The RuntimeError from get_cdm_context is handled cleanly, same as any other.""" + with pytest.raises(typer.Exit) as exc_info: + handle_error( + RuntimeError( + "No omop-alchemy configuration found. " + "Run `omop-config configure omop_alchemy` to set it up." + ) + ) + assert exc_info.value.exit_code == 1 captured = capsys.readouterr() - assert "/home/cava/.config/omop/config.toml" not in captured.out + assert "omop-config configure omop_alchemy" in captured.out + + +def test_handle_error_does_not_special_case_file_not_found(): + """A stray FileNotFoundError from elsewhere in a command body must not be + mislabeled as a missing-configuration error — it should propagate as-is.""" + exc = FileNotFoundError("pg_dump: No such file or directory") + with pytest.raises(FileNotFoundError) as exc_info: + handle_error(exc) + assert exc_info.value is exc