From 3b1d44a675c697ef744a9f38ff2df662f6a8f76c Mon Sep 17 00:00:00 2001 From: Aleksey Smaga Date: Tue, 5 May 2026 17:47:55 +0200 Subject: [PATCH 1/2] refactor(engine): extract checker dispatch from cli.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cli.py was a 590-line mix of Click wiring, the checker-dispatch engine, and CLI-side error-to-exit-code translation. Move dispatch to a pure pgsleuth/engine.py: open_context, run, RunResult, SkippedChecker, UnsupportedServerVersionError. The CLI is now a shell that builds Config, calls engine.run, and renders. Side effects that the engine used to perform are now structured data on the result: per-checker version gating and statement_timeout cancellations become SkippedChecker records (no stderr from inside dispatch), and an unsupported server version raises UnsupportedServerVersionError instead of sys.exit(2). The CLI translates both back to its existing stderr output and exit codes. Behavior change: baseline staleness is now keyed off RunResult.ran (the precise set of checkers that actually executed) rather than the config-only running set. Entries for version-gated or timed-out checkers are now classified as unknown_baseline_entries — their findings cannot be confirmed gone, so prune preserves them under the existing warn-before-remove policy. Closes the latent bug flagged by the comment that previously sat on _running_checkers. Tests: existing CLI tests migrate to patch pgsleuth.engine.run / pgsleuth.engine.connect via two helpers in conftest.py (make_run_result, fake_engine_run). New tests/test_engine.py covers threshold filtering, version-gating skips, statement_timeout skip via a real conn, baseline filtering with stale vs unknown classification, and open_context. --- src/pgsleuth/cli.py | 206 ++++------------ src/pgsleuth/engine.py | 214 +++++++++++++++++ tests/conftest.py | 50 ++++ tests/test_cli_baseline.py | 227 ++++++++++-------- tests/test_cli_unsupported_version.py | 13 +- tests/test_engine.py | 333 ++++++++++++++++++++++++++ tests/test_statement_timeout.py | 38 +-- 7 files changed, 799 insertions(+), 282 deletions(-) create mode 100644 src/pgsleuth/engine.py create mode 100644 tests/test_engine.py diff --git a/src/pgsleuth/cli.py b/src/pgsleuth/cli.py index 348a2e0..4f27ee3 100644 --- a/src/pgsleuth/cli.py +++ b/src/pgsleuth/cli.py @@ -4,26 +4,17 @@ import re import sys -from contextlib import nullcontext from pathlib import Path -from typing import Iterable import click -import psycopg from rich.console import Console import pgsleuth.checkers # noqa: F401 -- registers built-in checkers from pgsleuth import baseline as baseline_module -from pgsleuth.checkers.base import Issue, Severity, registry +from pgsleuth import engine +from pgsleuth.checkers.base import Severity, registry from pgsleuth.config import DEFAULT_EXCLUDED_SCHEMAS, Config -from pgsleuth.context import CheckerContext -from pgsleuth.db.connection import ( - SUPPORTED_VERSION_MIN, - SUPPORTED_VERSION_NAMES, - connect, - server_version_num, - statement_timeout, -) +from pgsleuth.engine import RunResult from pgsleuth.reporters import json as json_reporter from pgsleuth.reporters import text as text_reporter @@ -100,18 +91,6 @@ def _common_options(f): return f -def _running_checkers(config: Config) -> frozenset[str]: - """Names of checkers that this invocation will actually run. - - Excludes checkers filtered out by --checkers, disabled in TOML, or - not-yet-imported. Does *not* exclude checkers that will be skipped - at runtime due to version gating or `statement_timeout` cancellation — - those are best-effort knowledge we'd need to track inside `_run_all` - to be precise, and the practical impact is small. - """ - return frozenset(name for name in registry.names() if config.is_checker_enabled(name)) - - def _build_config_from_options( *, config_path: Path | None, @@ -152,6 +131,30 @@ def _build_config_from_options( return config +def _print_skipped(result: RunResult) -> None: + """Emit one stderr line per skipped checker (version-gated or timed out).""" + for sk in result.skipped: + click.echo(f"[skipped] {sk.checker} — {sk.detail}", err=True) + + +def _run_engine(dsn: str, config: Config, threshold: int, baseline=None) -> RunResult: + """Open a context and dispatch the engine, translating engine errors to exit-2. + + Centralizes the try/except so the three subcommands stay focused on their + own argument-handling. `engine.UnsupportedServerVersionError` and any other + DB exception both exit 2 with the message echoed to stderr. + """ + try: + with engine.open_context(dsn, config) as ctx: + return engine.run(ctx, threshold=threshold, baseline=baseline) + except engine.UnsupportedServerVersionError as exc: + click.echo(f"pgsleuth: {exc}", err=True) + sys.exit(2) + except Exception as exc: # noqa: BLE001 + click.echo(f"pgsleuth: {exc}", err=True) + sys.exit(2) + + @main.command("check") @_common_options @click.option( @@ -222,43 +225,24 @@ def check( click.echo(f"pgsleuth: {exc}", err=True) sys.exit(2) - try: - issues = _collect_issues(dsn, config, threshold) - except Exception as exc: # noqa: BLE001 - click.echo(f"pgsleuth: {exc}", err=True) - sys.exit(2) + result = _run_engine(dsn, config, threshold, baseline=baseline) + _print_skipped(result) - suppressed_count = 0 - if baseline is not None: - result = baseline_module.filter_issues(issues, baseline) - issues = result.kept - suppressed_count = result.suppressed_count - - # An entry is only meaningfully "stale" if its checker was actually - # in scope this run. Entries whose checker was filtered out - # (--checkers, [pgsleuth.checkers.X].enabled=false, version-gated) - # can't be matched against findings that were never produced — - # warning about them would be misleading. - running = _running_checkers(config) - stale = [ - e - for e in baseline_module.stale_entries(baseline, result.matched_fps) - if e.checker in running - ] - if stale: - click.echo( - f"pgsleuth: {len(stale)} baseline " - f"{'entry' if len(stale) == 1 else 'entries'} did not reproduce. " - f"Run 'pgsleuth baseline prune' to clean up.", - err=True, - ) + if result.stale_baseline_entries: + n = len(result.stale_baseline_entries) + click.echo( + f"pgsleuth: {n} baseline " + f"{'entry' if n == 1 else 'entries'} did not reproduce. " + f"Run 'pgsleuth baseline prune' to clean up.", + err=True, + ) if output_format == "json": - json_reporter.render(issues, suppressed=suppressed_count) + json_reporter.render(result.issues, suppressed=result.suppressed_count) else: - text_reporter.render(issues, suppressed=suppressed_count) + text_reporter.render(result.issues, suppressed=result.suppressed_count) - sys.exit(1 if issues else 0) + sys.exit(1 if result.issues else 0) @main.group("baseline") @@ -305,15 +289,10 @@ def baseline_write( ) # Capture everything; threshold=info means "all severities count". - threshold = Severity.INFO.rank - - try: - issues = _collect_issues(dsn, config, threshold) - except Exception as exc: # noqa: BLE001 - click.echo(f"pgsleuth: {exc}", err=True) - sys.exit(2) + result = _run_engine(dsn, config, Severity.INFO.rank) + _print_skipped(result) - baseline = baseline_module.from_issues(issues) + baseline = baseline_module.from_issues(result.issues) baseline_module.dump(baseline, output_path) n = len(baseline.fingerprints) click.echo( @@ -417,9 +396,9 @@ def baseline_prune( Loads the baseline, runs every enabled checker at info+ severity (a user's --min-severity is intentionally ignored here so we don't drop entries the current run merely wouldn't surface), and rewrites the - file with stale entries removed. Entries whose checker is no longer - registered (e.g. removed in a pgsleuth upgrade) are kept and warned - about — pass --ignore-unknown-checkers to silence. + file with stale entries removed. Entries whose checker did not run + in this invocation (filtered out, version-gated, or timed out) are + kept and warned about — pass --ignore-unknown-checkers to silence. """ config = _build_config_from_options( config_path=config_path, @@ -438,23 +417,10 @@ def baseline_prune( # Capture every finding regardless of user's --min-severity, so we don't # mistake "did not surface in this run" for "no longer present." - threshold = Severity.INFO.rank - try: - issues = _collect_issues(dsn, config, threshold) - except Exception as exc: # noqa: BLE001 - click.echo(f"pgsleuth: {exc}", err=True) - sys.exit(2) - - result = baseline_module.filter_issues(issues, baseline) - - # A checker is "known" for prune purposes only if it actually ran in - # this invocation. Treating registered-but-unrun checkers (--checkers - # filter, disabled in TOML, version-gated) as "known" would cause - # prune to drop their entries as stale — but we have no information - # about whether those findings still exist. - running = _running_checkers(config) - unknowns = baseline_module.unknown_checker_entries(baseline, running) + result = _run_engine(dsn, config, Severity.INFO.rank, baseline=baseline) + _print_skipped(result) + unknowns = result.unknown_baseline_entries if unknowns and not ignore_unknown_checkers: unknown_names = ", ".join(sorted({e.checker for e in unknowns})) click.echo( @@ -466,7 +432,7 @@ def baseline_prune( err=True, ) - pruned = baseline_module.prune(baseline, result.matched_fps, known_checkers=running) + pruned = baseline_module.prune(baseline, result.matched_baseline_fps, known_checkers=result.ran) pruned_set = set(pruned.fingerprints) removed = [e for e in baseline.fingerprints if e not in pruned_set] @@ -515,77 +481,5 @@ def _resolve_baseline_path(explicit_path: Path | None, no_baseline: bool) -> Pat return None -def _collect_issues(dsn: str, config: Config, threshold: int) -> list[Issue]: - """Connect, version-check, run enabled checkers, return filtered issues. - - Shared by the `check` command and the upcoming `baseline write` / - `baseline prune` subcommands so the connect-and-run pipeline lives - in one place. On unsupported server version, prints a stderr message - and `sys.exit(2)` — propagates as `SystemExit`, which the caller's - `except Exception` does not catch (intentional). Other DB errors - propagate as ordinary exceptions for the caller to log. - """ - with connect(dsn) as conn: - version = server_version_num(conn) - if version < SUPPORTED_VERSION_MIN: - click.echo( - f"pgsleuth: PostgreSQL {_pg_version_str(version)} is not supported. " - f"Supported versions: {SUPPORTED_VERSION_NAMES}.", - err=True, - ) - sys.exit(2) - ctx = CheckerContext(conn=conn, config=config, server_version=version) - return list(_run_all(ctx, threshold)) - - -def _run_all(ctx: CheckerContext, threshold: int) -> Iterable[Issue]: - for cls in registry.all(): - if not ctx.config.is_checker_enabled(cls.name): - continue - if not cls.supports(ctx.server_version): - click.echo( - f"[skipped] {cls.name} — requires PostgreSQL " - f"{_pg_version_label(cls.min_version, cls.max_version)} " - f"(connected: {_pg_version_str(ctx.server_version)})", - err=True, - ) - continue - - timeout_ms = ctx.config.statement_timeout_for(cls.name) - cm = statement_timeout(ctx.conn, timeout_ms) if timeout_ms is not None else nullcontext() - # Materialize per-checker so a QueryCanceled mid-yield discards partial - # findings rather than surfacing a half-result. - try: - with cm: - checker_issues = list(cls().run(ctx)) - except psycopg.errors.QueryCanceled: - click.echo( - f"[skipped] {cls.name} — exceeded statement_timeout of {timeout_ms}ms", - err=True, - ) - continue - - for issue in checker_issues: - if issue.severity.rank >= threshold: - yield issue - - -def _pg_version_str(num: int) -> str: - # PG10 changed the encoding: pre-10 is M_mm_pp (e.g. 90603 = 9.6.3), - # post-10 is M0_mmmm (e.g. 150004 = 15.4). - if num >= 100000: - return f"{num // 10000}.{num % 10000}" - return f"{num // 10000}.{(num // 100) % 100}" - - -def _pg_version_label(min_version: int | None, max_version: int | None) -> str: - parts = [] - if min_version is not None: - parts.append(f"{min_version // 10000}+") - if max_version is not None: - parts.append(f"<{max_version // 10000}") - return " and ".join(parts) if parts else "any" - - if __name__ == "__main__": main() diff --git a/src/pgsleuth/engine.py b/src/pgsleuth/engine.py new file mode 100644 index 0000000..1c32821 --- /dev/null +++ b/src/pgsleuth/engine.py @@ -0,0 +1,214 @@ +"""Checker-dispatch engine. + +The CLI is a shell — this module is what makes pgsleuth pgsleuth. Build a +`Config`, open a context with `open_context(dsn, config)`, call `run(ctx, ...)`, +hand the resulting `RunResult` to a reporter (or to whatever your library code +needs). + +Pure: no Click imports, no `sys.exit`, no stderr writes. Per-checker timeouts +and version-gating are returned as `SkippedChecker` records instead of being +printed; an unsupported server version raises `UnsupportedServerVersionError` +instead of exiting. Callers (the CLI, library users) decide how to surface +those. +""" + +from __future__ import annotations + +from contextlib import contextmanager, nullcontext +from dataclasses import dataclass +from typing import Iterator, Literal + +import psycopg + +from pgsleuth import baseline as baseline_module +from pgsleuth.baseline import Baseline, BaselineEntry +from pgsleuth.checkers.base import Issue, registry +from pgsleuth.config import Config +from pgsleuth.context import CheckerContext +from pgsleuth.db.connection import ( + SUPPORTED_VERSION_MIN, + SUPPORTED_VERSION_NAMES, + connect, + server_version_num, + statement_timeout, +) + +SkipReason = Literal["version_gated", "statement_timeout"] + + +@dataclass(frozen=True) +class SkippedChecker: + """A checker that didn't run to completion this invocation. + + `detail` is a ready-to-display human string (the CLI prints it after + `[skipped] — `; library callers can log it as-is). + """ + + checker: str + reason: SkipReason + detail: str + + +@dataclass(frozen=True) +class RunResult: + """Outcome of a single engine.run() invocation. + + `issues` is post-threshold and (if a baseline was supplied) post-baseline. + `ran` is the precise set of checker names that actually executed — a + checker filtered out by config, version-gated, or aborted by + `statement_timeout` is *not* in `ran`. Use this set (rather than the + config-only "enabled" set) to reason about which baseline entries are + stale: a checker that didn't run can't tell us whether its findings are + gone. + """ + + issues: list[Issue] + skipped: tuple[SkippedChecker, ...] + ran: frozenset[str] + suppressed_count: int = 0 + matched_baseline_fps: frozenset[str] = frozenset() + stale_baseline_entries: tuple[BaselineEntry, ...] = () + unknown_baseline_entries: tuple[BaselineEntry, ...] = () + + +class UnsupportedServerVersionError(Exception): + """Raised by `open_context` when the connected server is below the floor.""" + + def __init__(self, server_version: int) -> None: + super().__init__( + f"PostgreSQL {pg_version_str(server_version)} is not supported. " + f"Supported versions: {SUPPORTED_VERSION_NAMES}." + ) + self.server_version = server_version + + +@contextmanager +def open_context(dsn: str, config: Config) -> Iterator[CheckerContext]: + """Connect, verify the server version, yield a CheckerContext. + + Raises `UnsupportedServerVersionError` for servers below + `SUPPORTED_VERSION_MIN`. Other connection errors propagate as ordinary + `psycopg` exceptions for the caller to handle. + """ + with connect(dsn) as conn: + version = server_version_num(conn) + if version < SUPPORTED_VERSION_MIN: + raise UnsupportedServerVersionError(version) + yield CheckerContext(conn=conn, config=config, server_version=version) + + +def run( + ctx: CheckerContext, + *, + threshold: int, + baseline: Baseline | None = None, +) -> RunResult: + """Run every enabled checker; return findings, skips, and optional baseline summary. + + For each registered checker: + + - Skip silently if the config disables it (filter not surfaced — the user + asked for it). + - Record a `SkippedChecker(reason="version_gated")` if the connected + server is outside the checker's `[min_version, max_version)` range. + - Apply the per-checker `statement_timeout` and materialize results to a + list inside the timeout block (so a mid-iteration `QueryCanceled` + drops partial findings rather than leaking them); on cancel, record a + `SkippedChecker(reason="statement_timeout")` and continue with the + next checker. + - Otherwise add the checker to `ran` and append findings whose severity + meets `threshold`. + + With `baseline` supplied, fingerprints in the baseline are filtered out + of `issues` (counted in `suppressed_count`); `stale_baseline_entries` + enumerates baseline entries whose checker `ran` but whose fingerprint + didn't reproduce, and `unknown_baseline_entries` enumerates entries whose + checker did *not* run (filtered, gated, or timed out — we have no + information about whether their findings still exist). + """ + issues: list[Issue] = [] + skipped: list[SkippedChecker] = [] + ran: list[str] = [] + + for cls in registry.all(): + if not ctx.config.is_checker_enabled(cls.name): + continue + if not cls.supports(ctx.server_version): + skipped.append( + SkippedChecker( + checker=cls.name, + reason="version_gated", + detail=( + f"requires PostgreSQL " + f"{pg_version_label(cls.min_version, cls.max_version)} " + f"(connected: {pg_version_str(ctx.server_version)})" + ), + ) + ) + continue + + timeout_ms = ctx.config.statement_timeout_for(cls.name) + cm = statement_timeout(ctx.conn, timeout_ms) if timeout_ms is not None else nullcontext() + try: + with cm: + checker_issues = list(cls().run(ctx)) + except psycopg.errors.QueryCanceled: + skipped.append( + SkippedChecker( + checker=cls.name, + reason="statement_timeout", + detail=f"exceeded statement_timeout of {timeout_ms}ms", + ) + ) + continue + + ran.append(cls.name) + for issue in checker_issues: + if issue.severity.rank >= threshold: + issues.append(issue) + + ran_set = frozenset(ran) + + if baseline is None: + return RunResult( + issues=issues, + skipped=tuple(skipped), + ran=ran_set, + ) + + filtered = baseline_module.filter_issues(issues, baseline) + stale = tuple( + e + for e in baseline_module.stale_entries(baseline, filtered.matched_fps) + if e.checker in ran_set + ) + unknown = tuple(baseline_module.unknown_checker_entries(baseline, ran_set)) + + return RunResult( + issues=filtered.kept, + skipped=tuple(skipped), + ran=ran_set, + suppressed_count=filtered.suppressed_count, + matched_baseline_fps=filtered.matched_fps, + stale_baseline_entries=stale, + unknown_baseline_entries=unknown, + ) + + +def pg_version_str(num: int) -> str: + """Format a PG-encoded version int as a human string ("15.4", "9.6.3").""" + # PG10 changed the encoding: pre-10 is M_mm_pp (e.g. 90603 = 9.6.3), + # post-10 is M0_mmmm (e.g. 150004 = 15.4). + if num >= 100000: + return f"{num // 10000}.{num % 10000}" + return f"{num // 10000}.{(num // 100) % 100}" + + +def pg_version_label(min_version: int | None, max_version: int | None) -> str: + """Render a checker's version gate as "10+", "<14", "10+ and <14", or "any".""" + parts = [] + if min_version is not None: + parts.append(f"{min_version // 10000}+") + if max_version is not None: + parts.append(f"<{max_version // 10000}") + return " and ".join(parts) if parts else "any" diff --git a/tests/conftest.py b/tests/conftest.py index 26bcd34..ced143a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,13 +17,63 @@ import pytest from testcontainers.postgres import PostgresContainer +from pgsleuth import baseline as baseline_module +from pgsleuth.checkers.base import Issue from pgsleuth.config import Config from pgsleuth.context import CheckerContext from pgsleuth.db.connection import server_version_num +from pgsleuth.engine import RunResult PG_VERSIONS = ["10-alpine", "13-alpine", "15-alpine", "17-alpine"] +def make_run_result( + issues: list[Issue], + *, + baseline: baseline_module.Baseline | None = None, + ran: frozenset[str] | None = None, +) -> RunResult: + """Build a RunResult that mirrors what `engine.run` would produce. + + `ran` defaults to the set of checkers that produced an issue. Pass it + explicitly when a test needs to distinguish "checker ran with no findings" + (in `ran`, makes its baseline entries eligible for stale-warning) from + "checker didn't run" (not in `ran`, baseline entries treated as unknown). + """ + if ran is None: + ran = frozenset(i.checker for i in issues) + if baseline is None: + return RunResult(issues=list(issues), skipped=(), ran=ran) + filtered = baseline_module.filter_issues(issues, baseline) + stale = tuple( + e for e in baseline_module.stale_entries(baseline, filtered.matched_fps) if e.checker in ran + ) + unknown = tuple(baseline_module.unknown_checker_entries(baseline, ran)) + return RunResult( + issues=filtered.kept, + skipped=(), + ran=ran, + suppressed_count=filtered.suppressed_count, + matched_baseline_fps=filtered.matched_fps, + stale_baseline_entries=stale, + unknown_baseline_entries=unknown, + ) + + +def fake_engine_run(issues=(), ran: frozenset[str] | None = None): + """side_effect for `patch("pgsleuth.engine.run", ...)`. + + Replaces the old pattern of patching `cli._run_all` with a flat issue list. + The returned function honors the engine's `(ctx, *, threshold, baseline)` + signature and runs baseline filtering through `make_run_result`. + """ + + def _side_effect(ctx, *, threshold, baseline=None): + return make_run_result(list(issues), baseline=baseline, ran=ran) + + return _side_effect + + @pytest.fixture(scope="session", params=PG_VERSIONS, ids=lambda p: f"pg{p.split('-')[0]}") def postgres_container(request: pytest.FixtureRequest) -> Iterator[PostgresContainer]: with PostgresContainer(f"postgres:{request.param}") as pg: diff --git a/tests/test_cli_baseline.py b/tests/test_cli_baseline.py index 673320d..240321e 100644 --- a/tests/test_cli_baseline.py +++ b/tests/test_cli_baseline.py @@ -13,6 +13,8 @@ from pgsleuth.checkers.base import Issue, Severity from pgsleuth.cli import main +from .conftest import fake_engine_run + @contextmanager def _fake_connect(_dsn: str): @@ -54,14 +56,16 @@ def test_check_with_baseline_suppresses_known() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[ - _issue("missing_fk_index", "public.orders(user_id)"), - _issue("missing_fk_index", "public.invoices(customer_id)"), - ], + "pgsleuth.engine.run", + side_effect=fake_engine_run( + [ + _issue("missing_fk_index", "public.orders(user_id)"), + _issue("missing_fk_index", "public.invoices(customer_id)"), + ] + ), ), ): result = runner.invoke( @@ -83,14 +87,16 @@ def test_check_with_baseline_reports_new_findings() -> None: baseline = Path("baseline.json") _write_baseline(baseline, [("missing_fk_index", "public.orders(user_id)")]) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[ - _issue("missing_fk_index", "public.orders(user_id)"), # baselined - _issue("missing_fk_index", "public.audit_log(user_id)"), # NEW - ], + "pgsleuth.engine.run", + side_effect=fake_engine_run( + [ + _issue("missing_fk_index", "public.orders(user_id)"), # baselined + _issue("missing_fk_index", "public.audit_log(user_id)"), # NEW + ] + ), ), ): result = runner.invoke( @@ -112,11 +118,11 @@ def test_check_autodiscovers_default_baseline_in_cwd() -> None: [("missing_fk_index", "public.orders(user_id)")], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.orders(user_id)")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.orders(user_id)")]), ), ): result = runner.invoke(main, ["check", "--dsn", "postgresql://x/y"]) @@ -134,11 +140,11 @@ def test_check_no_baseline_disables_autodiscovery() -> None: [("missing_fk_index", "public.orders(user_id)")], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.orders(user_id)")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.orders(user_id)")]), ), ): result = runner.invoke(main, ["check", "--dsn", "postgresql://x/y", "--no-baseline"]) @@ -154,11 +160,11 @@ def test_check_no_baseline_with_no_file_runs_normally() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.orders(user_id)")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.orders(user_id)")]), ), ): result = runner.invoke(main, ["check", "--dsn", "postgresql://x/y"]) @@ -174,8 +180,8 @@ def test_check_baseline_corrupt_json_exits_2() -> None: baseline = Path("baseline.json") baseline.write_text("{ not valid json") with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke( main, @@ -190,8 +196,8 @@ def test_check_baseline_path_does_not_exist_exits_2() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke( main, @@ -208,8 +214,8 @@ def test_check_explicit_and_no_baseline_conflict() -> None: baseline = Path("baseline.json") _write_baseline(baseline, []) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke( main, @@ -250,11 +256,11 @@ def test_check_baseline_no_stale_warning_for_checkers_filtered_out() -> None: # Run only one checker. Mock _run_all to behave like that filter: # it returns only findings from the enabled checker. with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.orders(user_id)")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.orders(user_id)")]), ), ): result = runner.invoke( @@ -283,9 +289,14 @@ def test_check_baseline_warns_on_stale_entries() -> None: baseline = Path("baseline.json") _write_baseline(baseline, [("missing_fk_index", "public.fixed_long_ago")]) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", return_value=[]), # nothing reproduces + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + # missing_fk_index ran but produced nothing — its baseline entry is stale, + # not unknown. Pass `ran` explicitly so the helper knows the checker ran. + patch( + "pgsleuth.engine.run", + side_effect=fake_engine_run(ran=frozenset({"missing_fk_index"})), + ), ): result = runner.invoke( main, @@ -305,14 +316,16 @@ def test_baseline_write_creates_file_with_default_path() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[ - _issue("missing_fk_index", "public.orders(user_id)"), - _issue("missing_primary_key", "public.events"), - ], + "pgsleuth.engine.run", + side_effect=fake_engine_run( + [ + _issue("missing_fk_index", "public.orders(user_id)"), + _issue("missing_primary_key", "public.events"), + ] + ), ), ): result = runner.invoke(main, ["baseline", "write", "--dsn", "postgresql://x/y"]) @@ -332,11 +345,11 @@ def test_baseline_write_to_custom_output_path() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.t")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.t")]), ), ): result = runner.invoke( @@ -367,11 +380,11 @@ def test_baseline_write_overwrites_existing_file() -> None: ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.new")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.new")]), ), ): result = runner.invoke(main, ["baseline", "write", "--dsn", "postgresql://x/y"]) @@ -398,9 +411,9 @@ def test_baseline_write_captures_all_severities() -> None: warning_issue = _issue("missing_fk_index", "public.t") with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", return_value=[info_issue, warning_issue]), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=fake_engine_run([info_issue, warning_issue])), ): result = runner.invoke(main, ["baseline", "write", "--dsn", "postgresql://x/y"]) @@ -414,9 +427,9 @@ def test_baseline_write_empty_when_no_findings() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", return_value=[]), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=fake_engine_run()), ): result = runner.invoke(main, ["baseline", "write", "--dsn", "postgresql://x/y"]) @@ -430,11 +443,11 @@ def test_baseline_write_uses_pgsleuth_dsn_envvar() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.t")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.t")]), ), ): result = runner.invoke( @@ -532,11 +545,11 @@ def test_baseline_prune_removes_stale_entries() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.still_here")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.still_here")]), ), ): result = runner.invoke(main, ["baseline", "prune", "--dsn", "postgresql://x/y"]) @@ -562,11 +575,11 @@ def test_baseline_prune_keeps_unknown_checker_entries_with_warning() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.match")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.match")]), ), ): result = runner.invoke(main, ["baseline", "prune", "--dsn", "postgresql://x/y"]) @@ -589,9 +602,9 @@ def test_baseline_prune_ignore_unknown_checkers_silences_warning() -> None: [("removed_checker_v1", "public.unknown_obj")], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", return_value=[]), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=fake_engine_run()), ): result = runner.invoke( main, @@ -628,11 +641,11 @@ def test_baseline_prune_dry_run_does_not_write() -> None: before_mtime = path.stat().st_mtime_ns with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.match")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.match")]), ), ): result = runner.invoke( @@ -662,11 +675,11 @@ def test_baseline_prune_uses_explicit_baseline_path() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.match")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.match")]), ), ): result = runner.invoke( @@ -684,8 +697,8 @@ def test_baseline_prune_missing_baseline_file_exits_2() -> None: runner = CliRunner() with runner.isolated_filesystem(): with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke( main, @@ -708,8 +721,8 @@ def test_baseline_prune_corrupt_baseline_exits_2() -> None: with runner.isolated_filesystem(): Path("pgsleuth.baseline.json").write_text("{ not valid json") with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke(main, ["baseline", "prune", "--dsn", "postgresql://x/y"]) @@ -734,11 +747,11 @@ def test_baseline_prune_does_not_drop_entries_for_unrun_checkers() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[_issue("missing_fk_index", "public.orders(user_id)")], + "pgsleuth.engine.run", + side_effect=fake_engine_run([_issue("missing_fk_index", "public.orders(user_id)")]), ), ): result = runner.invoke( @@ -774,14 +787,16 @@ def test_baseline_prune_no_changes_when_all_match() -> None: ], ) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[ - _issue("missing_fk_index", "public.a"), - _issue("missing_fk_index", "public.b"), - ], + "pgsleuth.engine.run", + side_effect=fake_engine_run( + [ + _issue("missing_fk_index", "public.a"), + _issue("missing_fk_index", "public.b"), + ] + ), ), ): result = runner.invoke(main, ["baseline", "prune", "--dsn", "postgresql://x/y"]) @@ -799,14 +814,16 @@ def test_check_baseline_json_output_includes_suppressed() -> None: baseline = Path("baseline.json") _write_baseline(baseline, [("missing_fk_index", "public.orders(user_id)")]) with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), patch( - "pgsleuth.cli._run_all", - return_value=[ - _issue("missing_fk_index", "public.orders(user_id)"), - _issue("missing_fk_index", "public.audit_log(user_id)"), - ], + "pgsleuth.engine.run", + side_effect=fake_engine_run( + [ + _issue("missing_fk_index", "public.orders(user_id)"), + _issue("missing_fk_index", "public.audit_log(user_id)"), + ] + ), ), ): result = runner.invoke( diff --git a/tests/test_cli_unsupported_version.py b/tests/test_cli_unsupported_version.py index 3b09b0f..fe37991 100644 --- a/tests/test_cli_unsupported_version.py +++ b/tests/test_cli_unsupported_version.py @@ -9,6 +9,8 @@ from pgsleuth.cli import main +from .conftest import fake_engine_run + @contextmanager def _fake_connect(_dsn: str): @@ -18,8 +20,8 @@ def _fake_connect(_dsn: str): def test_unsupported_version_exits_with_message() -> None: runner = CliRunner() with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=90603), # PG 9.6.3 + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=90603), # PG 9.6.3 ): result = runner.invoke(main, ["check", "--dsn", "postgresql://x/y"]) @@ -31,11 +33,10 @@ def test_unsupported_version_exits_with_message() -> None: def test_supported_version_does_not_refuse() -> None: # When the server is supported but no checkers find anything, exit 0. runner = CliRunner() - fake_issues: list = [] with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", return_value=iter(fake_issues)), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=fake_engine_run()), ): result = runner.invoke(main, ["check", "--dsn", "postgresql://x/y"]) diff --git a/tests/test_engine.py b/tests/test_engine.py new file mode 100644 index 0000000..6c48e25 --- /dev/null +++ b/tests/test_engine.py @@ -0,0 +1,333 @@ +"""Unit tests for the engine module — exercising what was previously only +reachable through the Click runner. + +Most tests don't touch a real DB: synthetic checkers yield prebaked Issues, +and `Config(statement_timeout_ms=None)` skips the `SET statement_timeout` +side trip. The timeout-skip test needs a real `conn` to trigger +`psycopg.errors.QueryCanceled`. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from typing import ClassVar, Iterable +from unittest.mock import patch + +import psycopg +import pytest + +from pgsleuth import baseline as baseline_module +from pgsleuth import engine +from pgsleuth.baseline import fingerprint_for +from pgsleuth.checkers.base import Checker, Issue, Severity, _Registry +from pgsleuth.config import Config +from pgsleuth.context import CheckerContext + + +def _issue(checker: str, obj: str, severity: Severity = Severity.WARNING) -> Issue: + return Issue( + checker=checker, + severity=severity, + object_type="table", + object_name=obj, + message=f"finding on {obj}", + ) + + +def _ctx( + *, conn=None, config: Config | None = None, server_version: int = 150004 +) -> CheckerContext: + """Build a CheckerContext for tests that don't need a real DB connection. + + The default Config has `statement_timeout_ms=None` so the engine's + `statement_timeout(ctx.conn, ...)` branch is bypassed and `conn` can be a + placeholder. + """ + return CheckerContext( + conn=conn, + config=config or Config(statement_timeout_ms=None), + server_version=server_version, + ) + + +# ---------- Fixed-output checkers (no DB required) ---------- + + +def _make_checker( + name_: str, + issues: list[Issue], + *, + severity: Severity = Severity.WARNING, + min_version: int | None = None, + max_version: int | None = None, +) -> type[Checker]: + class _Synthetic(Checker): + name: ClassVar[str] = name_ + description: ClassVar[str] = "test-only" + default_severity: ClassVar[Severity] = severity + + def run(self, ctx: CheckerContext) -> Iterable[Issue]: + yield from issues + + _Synthetic.min_version = min_version + _Synthetic.max_version = max_version + return _Synthetic + + +@pytest.fixture() +def isolated_registry(monkeypatch: pytest.MonkeyPatch) -> _Registry: + """Replace `pgsleuth.engine.registry` with a fresh, empty registry.""" + reg = _Registry() + monkeypatch.setattr("pgsleuth.engine.registry", reg) + return reg + + +# ---------- threshold filtering ---------- + + +def test_run_keeps_issues_at_or_above_threshold(isolated_registry: _Registry) -> None: + isolated_registry.register( + _make_checker( + "c_warning", + [ + _issue("c_warning", "public.t_info", severity=Severity.INFO), + _issue("c_warning", "public.t_warn", severity=Severity.WARNING), + _issue("c_warning", "public.t_err", severity=Severity.ERROR), + ], + ) + ) + result = engine.run(_ctx(), threshold=Severity.WARNING.rank) + objects = sorted(i.object_name for i in result.issues) + assert objects == ["public.t_err", "public.t_warn"] + + +def test_run_keeps_all_at_info_threshold(isolated_registry: _Registry) -> None: + isolated_registry.register( + _make_checker( + "c", + [ + _issue("c", "public.a", severity=Severity.INFO), + _issue("c", "public.b", severity=Severity.WARNING), + ], + ) + ) + result = engine.run(_ctx(), threshold=Severity.INFO.rank) + assert len(result.issues) == 2 + + +# ---------- version gating ---------- + + +def test_run_records_version_gated_skip(isolated_registry: _Registry) -> None: + # Checker requires PG 16+; ctx is on PG 15. + isolated_registry.register( + _make_checker("needs_pg16", [_issue("needs_pg16", "public.t")], min_version=160000) + ) + result = engine.run(_ctx(server_version=150004), threshold=0) + + assert result.issues == [] + assert "needs_pg16" not in result.ran + assert len(result.skipped) == 1 + assert result.skipped[0].checker == "needs_pg16" + assert result.skipped[0].reason == "version_gated" + assert "16+" in result.skipped[0].detail + assert "15.4" in result.skipped[0].detail + + +def test_run_records_max_version_gated_skip(isolated_registry: _Registry) -> None: + isolated_registry.register( + _make_checker("obsolete", [_issue("obsolete", "public.t")], max_version=140000) + ) + result = engine.run(_ctx(server_version=150004), threshold=0) + assert result.issues == [] + assert "obsolete" not in result.ran + assert result.skipped[0].reason == "version_gated" + + +# ---------- statement_timeout skip (needs real conn) ---------- + + +class _SlowChecker(Checker): + name: ClassVar[str] = "_test_slow_engine" + description: ClassVar[str] = "test-only" + default_severity: ClassVar[Severity] = Severity.WARNING + + def run(self, ctx: CheckerContext) -> Iterable[Issue]: + with ctx.conn.cursor() as cur: + cur.execute("SELECT pg_sleep(2)") + yield _issue(self.name, "never.reached") + + +class _FastChecker(Checker): + name: ClassVar[str] = "_test_fast_engine" + description: ClassVar[str] = "test-only" + default_severity: ClassVar[Severity] = Severity.WARNING + + def run(self, ctx: CheckerContext) -> Iterable[Issue]: + yield _issue(self.name, "public.fast_finding") + + +def test_run_records_statement_timeout_skip_and_continues( + conn: psycopg.Connection, monkeypatch: pytest.MonkeyPatch +) -> None: + reg = _Registry() + reg.register(_SlowChecker) + reg.register(_FastChecker) + monkeypatch.setattr("pgsleuth.engine.registry", reg) + + ctx = CheckerContext( + conn=conn, + config=Config(statement_timeout_ms=200), + server_version=150004, + ) + result = engine.run(ctx, threshold=Severity.INFO.rank) + + objects = {i.object_name for i in result.issues} + assert "public.fast_finding" in objects + assert "never.reached" not in objects + + assert "_test_fast_engine" in result.ran + assert "_test_slow_engine" not in result.ran + + timeouts = [s for s in result.skipped if s.reason == "statement_timeout"] + assert len(timeouts) == 1 + assert timeouts[0].checker == "_test_slow_engine" + assert "200ms" in timeouts[0].detail + + +# ---------- baseline filtering ---------- + + +def test_run_without_baseline_returns_zero_suppressed(isolated_registry: _Registry) -> None: + isolated_registry.register(_make_checker("c", [_issue("c", "public.t")])) + result = engine.run(_ctx(), threshold=0) + assert result.suppressed_count == 0 + assert result.matched_baseline_fps == frozenset() + assert result.stale_baseline_entries == () + assert result.unknown_baseline_entries == () + + +def test_run_with_baseline_suppresses_matched(isolated_registry: _Registry) -> None: + isolated_registry.register( + _make_checker( + "c", + [ + _issue("c", "public.matched"), + _issue("c", "public.new"), + ], + ) + ) + baseline = baseline_module.from_issues([_issue("c", "public.matched")]) + result = engine.run(_ctx(), threshold=0, baseline=baseline) + + assert result.suppressed_count == 1 + assert [i.object_name for i in result.issues] == ["public.new"] + assert result.matched_baseline_fps == frozenset({fingerprint_for("c", "public.matched")}) + + +def test_run_with_baseline_filters_stale_to_ran_checkers(isolated_registry: _Registry) -> None: + """An entry whose checker actually ran but didn't reproduce → stale. + An entry whose checker did *not* run → unknown (not stale). + """ + isolated_registry.register(_make_checker("ran_checker", [])) # ran, no findings + # Note: gone_checker is not registered → not in ran. + + baseline = baseline_module.from_issues( + [ + _issue("ran_checker", "public.fixed"), # checker ran, didn't produce → stale + _issue("gone_checker", "public.unknown"), # checker didn't run → unknown + ] + ) + result = engine.run(_ctx(), threshold=0, baseline=baseline) + + assert "ran_checker" in result.ran + assert "gone_checker" not in result.ran + + stale_objects = {e.object for e in result.stale_baseline_entries} + unknown_objects = {e.object for e in result.unknown_baseline_entries} + assert stale_objects == {"public.fixed"} + assert unknown_objects == {"public.unknown"} + + +def test_run_with_baseline_version_gated_checker_marked_unknown( + isolated_registry: _Registry, +) -> None: + """A version-gated checker isn't in `ran`, so its baseline entry is + classified as unknown (we can't tell whether the finding still exists). + Fixes the latent bug where a version-gated checker's baseline could be + silently pruned. + """ + isolated_registry.register(_make_checker("gated_checker", [], min_version=160000)) + + baseline = baseline_module.from_issues([_issue("gated_checker", "public.t")]) + result = engine.run(_ctx(server_version=150004), threshold=0, baseline=baseline) + + assert "gated_checker" not in result.ran + assert result.stale_baseline_entries == () # not stale — we couldn't run it + assert {e.object for e in result.unknown_baseline_entries} == {"public.t"} + + +# ---------- open_context / UnsupportedServerVersionError ---------- + + +def test_open_context_raises_on_unsupported_version() -> None: + @contextmanager + def fake_connect(_dsn): + yield object() + + with ( + patch("pgsleuth.engine.connect", fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=90603), + ): + with pytest.raises(engine.UnsupportedServerVersionError) as exc_info: + with engine.open_context("postgresql://x/y", Config()): + pass + + assert exc_info.value.server_version == 90603 + msg = str(exc_info.value) + assert "PostgreSQL 9.6 is not supported" in msg + assert "Supported versions:" in msg + + +def test_open_context_yields_ctx_on_supported_version() -> None: + sentinel = object() + + @contextmanager + def fake_connect(_dsn): + yield sentinel + + with ( + patch("pgsleuth.engine.connect", fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + ): + with engine.open_context("postgresql://x/y", Config()) as ctx: + assert ctx.conn is sentinel + assert ctx.server_version == 150004 + + +# ---------- pure helpers ---------- + + +def test_pg_version_str_post_pg10() -> None: + assert engine.pg_version_str(150004) == "15.4" + assert engine.pg_version_str(170000) == "17.0" + + +def test_pg_version_str_pre_pg10() -> None: + assert engine.pg_version_str(90603) == "9.6" + + +def test_pg_version_label_min_only() -> None: + assert engine.pg_version_label(140000, None) == "14+" + + +def test_pg_version_label_max_only() -> None: + assert engine.pg_version_label(None, 160000) == "<16" + + +def test_pg_version_label_both() -> None: + assert engine.pg_version_label(140000, 160000) == "14+ and <16" + + +def test_pg_version_label_neither() -> None: + assert engine.pg_version_label(None, None) == "any" diff --git a/tests/test_statement_timeout.py b/tests/test_statement_timeout.py index a9bb7d8..48cdff9 100644 --- a/tests/test_statement_timeout.py +++ b/tests/test_statement_timeout.py @@ -81,11 +81,13 @@ def _fake_connect(_dsn: str): def _capture_config(captured: list[Config]): - """Return a side_effect that captures the CheckerContext.config and yields no findings.""" + """Return a side_effect that captures the CheckerContext.config and yields an empty RunResult.""" - def wrapper(ctx: CheckerContext, threshold: int): + def wrapper(ctx: CheckerContext, *, threshold: int, baseline=None): captured.append(ctx.config) - return iter(()) + from pgsleuth.engine import RunResult + + return RunResult(issues=[], skipped=(), ran=frozenset()) return wrapper @@ -94,9 +96,9 @@ def test_cli_flag_overrides_config() -> None: runner = CliRunner() captured: list[Config] = [] with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", side_effect=_capture_config(captured)), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=_capture_config(captured)), ): result = runner.invoke( main, @@ -110,9 +112,9 @@ def test_cli_no_statement_timeout_flag() -> None: runner = CliRunner() captured: list[Config] = [] with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), - patch("pgsleuth.cli._run_all", side_effect=_capture_config(captured)), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=_capture_config(captured)), ): result = runner.invoke( main, @@ -125,8 +127,8 @@ def test_cli_no_statement_timeout_flag() -> None: def test_cli_conflicting_flags_raise_usage_error() -> None: runner = CliRunner() with ( - patch("pgsleuth.cli.connect", _fake_connect), - patch("pgsleuth.cli.server_version_num", return_value=150004), + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), ): result = runner.invoke( main, @@ -217,18 +219,24 @@ def test_run_all_skips_slow_checker_continues_with_others( ) -> None: """A slow checker is skipped; the other checker's findings still surface.""" from pgsleuth.checkers.base import _Registry - from pgsleuth.cli import _run_all + from pgsleuth.engine import run as engine_run test_registry = _Registry() test_registry.register(_SlowChecker) test_registry.register(_FastChecker) - monkeypatch.setattr("pgsleuth.cli.registry", test_registry) + monkeypatch.setattr("pgsleuth.engine.registry", test_registry) cfg = Config(statement_timeout_ms=200) ctx = CheckerContext(conn=conn, config=cfg, server_version=150004) - issues = list(_run_all(ctx, threshold=Severity.INFO.rank)) + result = engine_run(ctx, threshold=Severity.INFO.rank) - object_names = {i.object_name for i in issues} + object_names = {i.object_name for i in result.issues} assert "public.fast_finding" in object_names assert "never.reached" not in object_names + # The structured RunResult records both the skip and the successful run: + assert any( + s.checker == "_test_slow" and s.reason == "statement_timeout" for s in result.skipped + ) + assert "_test_fast" in result.ran + assert "_test_slow" not in result.ran From 2d6dbbae44d153e444eab9e44b936aac52c7f7ef Mon Sep 17 00:00:00 2001 From: Aleksey Smaga Date: Tue, 5 May 2026 17:50:48 +0200 Subject: [PATCH 2/2] feat(reporters): surface skipped checkers in JSON output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JSON reporter now emits a "skipped" array alongside "issues" and "suppressed". Each entry carries the checker name, reason (version_gated | statement_timeout), and the human-readable detail string the engine already produces for the [skipped] stderr line. A CI consumer parsing JSON previously saw "no issues, clean run" while half the checks may have silently not run. With this field, dashboards and gating logic can decide what skips mean for them — e.g. fail the build if statement_timeout cancellations exceed N, or require explicit acknowledgement when version_gated entries appear. The text reporter is unchanged; terminal users continue to see the [skipped] lines on stderr exactly as before. --- src/pgsleuth/cli.py | 6 +++- src/pgsleuth/reporters/json.py | 13 +++++++++ tests/test_cli_baseline.py | 52 ++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/pgsleuth/cli.py b/src/pgsleuth/cli.py index 4f27ee3..9799c3b 100644 --- a/src/pgsleuth/cli.py +++ b/src/pgsleuth/cli.py @@ -238,7 +238,11 @@ def check( ) if output_format == "json": - json_reporter.render(result.issues, suppressed=result.suppressed_count) + json_reporter.render( + result.issues, + suppressed=result.suppressed_count, + skipped=result.skipped, + ) else: text_reporter.render(result.issues, suppressed=result.suppressed_count) diff --git a/src/pgsleuth/reporters/json.py b/src/pgsleuth/reporters/json.py index 640999b..3541a49 100644 --- a/src/pgsleuth/reporters/json.py +++ b/src/pgsleuth/reporters/json.py @@ -7,6 +7,7 @@ from typing import Iterable, TextIO from pgsleuth.checkers.base import Issue +from pgsleuth.engine import SkippedChecker def render( @@ -14,11 +15,23 @@ def render( *, stream: TextIO | None = None, suppressed: int = 0, + skipped: Iterable[SkippedChecker] = (), ) -> None: + """Emit a stable JSON payload for CI consumers. + + `skipped` carries any checkers that the engine did not run to completion + — version-gated or aborted by `statement_timeout`. Surfacing it in the + structured output prevents a silent "clean run" on a database where + half the checks never executed; CI can fail (or escalate) based on the + list rather than scraping stderr. + """ stream = stream or sys.stdout payload = { "issues": [issue.to_dict() for issue in issues], "suppressed": suppressed, + "skipped": [ + {"checker": s.checker, "reason": s.reason, "detail": s.detail} for s in skipped + ], } json.dump(payload, stream, indent=2, sort_keys=True) stream.write("\n") diff --git a/tests/test_cli_baseline.py b/tests/test_cli_baseline.py index 240321e..b80ab8d 100644 --- a/tests/test_cli_baseline.py +++ b/tests/test_cli_baseline.py @@ -847,3 +847,55 @@ def test_check_baseline_json_output_includes_suppressed() -> None: assert payload["suppressed"] == 1 assert len(payload["issues"]) == 1 assert payload["issues"][0]["object_name"] == "public.audit_log(user_id)" + # No checkers were skipped in this run. + assert payload["skipped"] == [] + + +def test_check_json_output_includes_skipped_checkers() -> None: + """Checkers the engine couldn't run to completion (version-gated, timed out) + must be surfaced in the JSON payload — a CI consumer parsing JSON should + not see "no issues" while half the checks silently didn't run. + """ + from pgsleuth.engine import RunResult, SkippedChecker + + def _engine_run_with_skips(ctx, *, threshold, baseline=None): + return RunResult( + issues=[], + skipped=( + SkippedChecker( + checker="needs_pg17", + reason="version_gated", + detail="requires PostgreSQL 17+ (connected: 15.4)", + ), + SkippedChecker( + checker="slow_index_scan", + reason="statement_timeout", + detail="exceeded statement_timeout of 5000ms", + ), + ), + ran=frozenset({"missing_fk_index"}), + ) + + runner = CliRunner() + with runner.isolated_filesystem(): + with ( + patch("pgsleuth.engine.connect", _fake_connect), + patch("pgsleuth.engine.server_version_num", return_value=150004), + patch("pgsleuth.engine.run", side_effect=_engine_run_with_skips), + ): + result = runner.invoke( + main, + ["check", "--dsn", "postgresql://x/y", "--format", "json", "--no-baseline"], + ) + + assert result.exit_code == 0 + start = result.output.index("{") + payload = json.loads(result.output[start:]) + assert payload["issues"] == [] + skipped = payload["skipped"] + assert len(skipped) == 2 + by_checker = {s["checker"]: s for s in skipped} + assert by_checker["needs_pg17"]["reason"] == "version_gated" + assert "17+" in by_checker["needs_pg17"]["detail"] + assert by_checker["slow_index_scan"]["reason"] == "statement_timeout" + assert "5000ms" in by_checker["slow_index_scan"]["detail"]