diff --git a/src/pgsleuth/cli.py b/src/pgsleuth/cli.py index 348a2e0..9799c3b 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,28 @@ 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, + skipped=result.skipped, + ) 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 +293,10 @@ def baseline_write( ) # Capture everything; threshold=info means "all severities count". - threshold = Severity.INFO.rank + result = _run_engine(dsn, config, Severity.INFO.rank) + _print_skipped(result) - try: - issues = _collect_issues(dsn, config, threshold) - except Exception as exc: # noqa: BLE001 - click.echo(f"pgsleuth: {exc}", err=True) - sys.exit(2) - - 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 +400,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 +421,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 +436,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 +485,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/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/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..b80ab8d 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( @@ -830,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"] 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