From a45d873c4fcd79a667c78cf0a785ce16d21a2848 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 22:45:34 +0200 Subject: [PATCH 1/7] fix(advise): count the dbt DROP INDEX branch in the terminal disclosure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `describe_rewrites` exists because neither thing dbt enrichment does shows up in a terminal row: an enriched proposal is byte-identical to the same proposal from a dbt-free run. It counted the ADV302 config rewrite and the generic non-index warning, but not the third outcome — a `DROP INDEX` on a dbt-managed relation, where the note says `dbt run` may recreate the index from the model's `indexes:` config. That branch is reachable on the adapter this module was written for, not a hypothetical: a Postgres run whose only proposals for dbt-managed relations are ADV002/ADV003 drops enriches every one of them, in `rationale` and in the `--ddl` note, and the terminal printed nothing. The operator applies the drop, dbt puts the index back, and the same proposal returns on the next run — the silently-reverting advice ADV302 exists to eliminate, pointing the other way. Counted off its own evidence flag and reported as its own clause rather than folded into the "cannot be expressed as dbt config" one: that clause tells the operator to expect a runnable statement not to last, while this one tells them to delete a config entry as well, which is the opposite action. This changes Postgres stderr deliberately. The previous round forbade that in order to prove an extraction had not altered behaviour; that constraint has served its purpose. No existing assertion was weakened — the two existing counts keep their exact wording, and the new test pins that the drop count discriminates this branch from both of them. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 ++- src/sqlquality/workload/dbt.py | 32 ++++++++++-- tests/test_workload_dbt.py | 94 ++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec7762e..2a93393 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,7 +93,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 reach (nothing Redshift emits is a `CREATE INDEX`), so a `--project-dir` run on Redshift warned in `rationale` and in the `--ddl` note that `dbt run` may undo an hours-long full-table rewrite while the terminal row stayed byte-identical to a dbt-free run. Any - proposal whose DDL cannot be expressed as dbt config is now counted and reported too. + proposal whose DDL cannot be expressed as dbt config is now counted and reported too, as is + an index *drop* (ADV002, ADV003) on a dbt-managed relation — the case reachable on Postgres, + where a run proposing only drops enriched every one of them and still said nothing, so an + operator applied a drop that the next `dbt run` recreated from the model's `indexes:` config. + Each of the three outcomes is reported as its own clause, because each calls for a different + action: paste a config block, expect a runnable statement not to survive the next rebuild, or + delete a config entry as well as running the drop. - `IS NOT NULL` predicates were classified as `IS NULL` when sqlglot 30.13 or newer was installed, because that release moved the negation from a wrapping `Not` node onto a `negate` flag on the `Is` node itself. Both encodings are now read. This was not cosmetic: diff --git a/src/sqlquality/workload/dbt.py b/src/sqlquality/workload/dbt.py index 53e83c2..711baa3 100644 --- a/src/sqlquality/workload/dbt.py +++ b/src/sqlquality/workload/dbt.py @@ -695,6 +695,14 @@ def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _I "removed as well or the drop will not stick — and this proposal will come " "back on the next run." ) + # A flag for the same reason `dbt_index_config` and + # `dbt_ddl_not_expressed_as_config` are: the warning above lives in `rationale` + # and `note`, and the terminal prints neither, so this row is byte-identical to + # the same ADV002/ADV003 proposal from a dbt-free run. Without this, + # `describe_rewrites` was silent on the whole class of *Postgres* run that emits + # only index drops for dbt-managed relations — enrichment fired, the operator was + # told nothing, and the drop they applied came back on the next `dbt run`. + evidence["dbt_drop_may_be_recreated"] = True return ( dataclasses.replace( proposal, @@ -936,14 +944,23 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: same title. Without this line a user who reads only the terminal cannot tell enrichment happened. - **Both kinds are counted, not only ADV302's.** Counting the config-block rewrite alone - made this function return `None` for every Redshift run: nothing Redshift emits is a + **All three kinds are counted, not only ADV302's.** Counting the config-block rewrite + alone made this function return `None` for every Redshift run: nothing Redshift emits is a `CREATE INDEX`, so every Redshift proposal for a dbt-managed relation takes `_classify`'s generic path instead — it gets the warning that `dbt run` may undo an hours-long full-table rewrite, and then the terminal said nothing at all. That is verbatim the failure mode this function exists to prevent, on the engine where the wasted work is hours rather than seconds. + The third kind — an index *drop* on a dbt-managed relation, which `dbt run` may put + straight back from the model's `indexes:` config — was the same hole one branch further + along, and reachable on the adapter this module was written for: a Postgres run whose only + proposals for dbt-managed relations are ADV002/ADV003 drops enriches every one of them and + used to print nothing. It is counted separately rather than folded into the "cannot be + expressed as dbt config" clause because the required action is the opposite one — there + the operator keeps the statement and expects it not to last, here they must *also* delete + a config entry or the drop silently reverts. + Counted off evidence flags rather than by searching the DDL or rationale text for "ADV302", which would depend on the wording of a string meant for humans. """ @@ -952,7 +969,10 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: unexpressed = sum( 1 for p in proposals if p.evidence.get("dbt_ddl_not_expressed_as_config") is True ) - if not rewritten and not merged and not unexpressed: + recreatable_drops = sum( + 1 for p in proposals if p.evidence.get("dbt_drop_may_be_recreated") is True + ) + if not rewritten and not merged and not unexpressed and not recreatable_drops: return None clauses: list[str] = [] if rewritten or merged: @@ -972,6 +992,12 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: "as dbt config: the statement is still runnable, but the next `dbt run` may undo " "it — see each proposal's note in --ddl, or its rationale in --markdown/--json" ) + if recreatable_drops: + clauses.append( + f"{recreatable_drops} index drop(s) target a dbt-managed relation: if the index is " + "declared in the model's `indexes:` config, remove that entry too or the next " + "`dbt run` recreates it and the drop does not stick" + ) return "; ".join(clauses) diff --git a/tests/test_workload_dbt.py b/tests/test_workload_dbt.py index e888250..f5c35c1 100644 --- a/tests/test_workload_dbt.py +++ b/tests/test_workload_dbt.py @@ -1348,6 +1348,100 @@ def test_describe_rewrites_reports_both_kinds_in_one_line(): assert "1 proposal(s) target a dbt-managed relation" in line +def _drop_proposal(relation, *, code="ADV002", index="idx_cold"): + """An index-drop proposal — the shape ADV002 (unused) and ADV003 (redundant) emit, and the + only shape that reaches `_classify`'s `DROP INDEX` branch.""" + return Proposal( + code=code, + title=f"Drop unused index {index} on {relation}", + rationale="no scans.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "index": index, + "columns": ("status",), + }, + confidence=Confidence.MEDIUM, + ddl=f'DROP INDEX "{relation.schema}"."{index}";', + ) + + +def test_describe_rewrites_reports_an_index_drop_that_a_dbt_run_may_recreate(): + """The third enrichment outcome, and the one reachable on the adapter this module was + written for. A Postgres run whose only proposals for dbt-managed relations are ADV002 / + ADV003 drops enriches every one of them — rationale *and* note — and the terminal said + nothing at all, because only the config-block rewrite and the generic non-index warning + were counted. The operator then applies a drop that the next `dbt run` puts straight back + from the model's `indexes:` config, and this tool proposes the same drop again next run. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + out = enrich_proposals( + [ + _drop_proposal(relation, code="ADV002", index="idx_cold"), + _drop_proposal(relation, code="ADV003", index="idx_narrow"), + ], + context, + ) + assert [p.ddl for p in out] == [ + 'DROP INDEX "main"."idx_cold";', + 'DROP INDEX "main"."idx_narrow";', + ], "the drops themselves are unchanged; only the disclosure is new" + line = describe_rewrites(out) + assert line is not None + assert "2 index drop(s)" in line + assert "`indexes:` config" in line + assert "does not stick" in line + assert "\n" not in line, "one stderr line" + + +def test_describe_rewrites_is_still_silent_for_an_unmanaged_index_drop(): + """Control for the test above: the new count must depend on `DbtContext.model_for` + actually matching, not merely on a proposal whose DDL is a `DROP INDEX`.""" + context = DbtContext.from_project(_project()) + out = enrich_proposals([_drop_proposal(Relation("public", "unmanaged"))], context) + assert describe_rewrites(out) is None + + +def test_describe_rewrites_counts_the_drop_branch_separately_from_the_other_two(): + """The count has to discriminate *this* branch, not merely be non-zero when enrichment + fired. A flag set in `_classify`'s generic non-index path (or read off the same key as the + config rewrite) would satisfy a bare "the line mentions drops" assertion while reporting + the wrong number for a mixed run, and would report drops on a run that had none. + + All three outcomes call for different actions, which is why they are three clauses: paste + a config block, expect a runnable statement not to last, or delete a config entry as well. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + + only_drops = enrich_proposals([_drop_proposal(relation)], context) + line = describe_rewrites(only_drops) + assert line is not None + assert "1 index drop(s)" in line + assert "ADV302" not in line, "a drop is not expressed as an `indexes` config block" + assert "cannot be expressed as dbt config" not in line, "that is the generic branch" + + # The other two branches must not be counted as drops. + for other in ( + enrich_proposals([_index_proposal(relation)], context), + enrich_proposals([_non_index_proposal(relation)], context), + ): + other_line = describe_rewrites(other) + assert other_line is not None + assert "index drop(s)" not in other_line, other_line + + all_three = enrich_proposals( + [_index_proposal(relation), _non_index_proposal(relation), _drop_proposal(relation)], + context, + ) + mixed = describe_rewrites(all_three) + assert mixed is not None + assert "ADV302 expressed 1 index proposal(s)" in mixed + assert "1 proposal(s) target a dbt-managed relation" in mixed + assert "1 index drop(s)" in mixed + + def test_prepend_note_keeps_the_existing_note_first_and_emits_the_dbt_note_once(): """Order and non-duplication, neither of which was pinned: losing the existing note was caught by two tests, but swapping the concatenation order and emitting the dbt warning From 69137b1fca16605f4c52fe38f564a065e7352a03 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 22:49:48 +0200 Subject: [PATCH 2/7] fix(advise): make ADV004 consult the existing-index list instead of skipping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADV004 was the only index-creating rule that never called `_covered` at all. It could propose a partial index that an existing one already serves, and its rationale said nothing about the gap — a check that silently did not run, which is the one thing this rule set is built to refuse. Both options were honest; this takes the suppressing one, with disclosure where suppression is not supportable. Suppress on a plain index leading with the guarded column. `WHERE status = $1 AND shipped_at IS NOT NULL` is already served by a plain index on `(status, ...)` with the null check applied as a filter, so the partial index adds no access path — only size. Size is precisely what this tool cannot measure: nothing here knows what fraction of the table satisfies the guard, so "a fraction of the size" would be an assertion traded against a second index's write cost on every write. Nothing downstream would catch the pair either: ADV003's redundant-prefix check is restricted to plain indexes, so a partial index shadowed by a plain one is never flagged on any later run. Disclose, rather than suppress, where the check genuinely cannot answer: * `have_index_data=False` (catalog query denied) caps confidence at LOW and names the skipped check, exactly as ADV001/ADV007/ADV008 do. * An existing *partial* index leading with the same column is named. `_covered` excludes partial indexes deliberately, and for this rule that exclusion cuts the other way than for ADV001: such an index may be precisely this proposal already applied. Predicates are not compared — the existing WHERE clause is not parsed, and this proposal's guard is reconstructed from redacted usage — so it is reported as unknown, by name. * An expression index mentioning the column is named, the same gap the other three rules already disclose. Evidence gains `partial_indexes_not_compared` and `expression_indexes`. Deliberately not ADV001's `partial_indexes_skipped`: there the partial index is known not to cover an unfiltered lookup, here nobody compared the predicates, and `--json` renders evidence as bare k=v pairs with no per-rule text to distinguish them. This changes Postgres output deliberately: an ADV004 proposal covered by a plain index disappears, and the rationale gains sentences in three previously silent cases. No existing assertion was weakened — a surgical mutant that removes only the `_covered` call kills exactly one test (the new suppression test) and leaves the other 935 green. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 +++ README.md | 2 +- src/sqlquality/workload/postgres.py | 90 +++++++++++++++++++- tests/test_workload_rules.py | 127 ++++++++++++++++++++++++++++ 4 files changed, 227 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a93393..2ca9a46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- ADV004 (partial index) now consults the existing-index list, which it was alone among the + index-creating rules in never doing. A plain index leading with the guarded column now + suppresses the proposal — that index already serves the lookup, and the partial index's only + advantage is a size saving this tool cannot measure against a second index's write cost (and + which ADV003 would never flag as redundant, since its prefix check is restricted to plain + indexes). Where a check genuinely could not run it is now stated rather than skipped: an + unreadable existing-index list caps confidence at LOW and says so, and an existing *partial* + or expression index that leads with the same column is named, since sqlquality does not + compare index predicates and so cannot tell whether the proposal is already applied. New + evidence keys `partial_indexes_not_compared` and `expression_indexes`; deliberately not + ADV001's `partial_indexes_skipped`, which records a different fact. - dbt enrichment now discloses itself in the terminal on **every** engine. The stderr disclosure line counted only ADV302's config-block rewrite, which no Redshift proposal can reach (nothing Redshift emits is a `CREATE INDEX`), so a `--project-dir` run on Redshift diff --git a/README.md b/README.md index bd09e5d..4da7507 100644 --- a/README.md +++ b/README.md @@ -410,7 +410,7 @@ statement is not valid SQL to copy out and run. | ADV001 | Composite index candidate: hot equality columns, then one range/sort column, arity ≤ 3, and only columns some single query group filters on *together* | cost share, NDV, row estimate, joint co-occurring fingerprint count, absence of a covering index | | ADV002 | Drop an index with zero recorded scans (excludes unique/primary-key indexes) | scans since last stats reset, size | | ADV003 | Drop an index whose column list is a strict prefix of a wider index | both column lists | -| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count | +| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count, absence of a plain index leading with the guarded column | | ADV005 | Non-sargable predicate — a cast/function on a column, or a leading-wildcard `LIKE` | cost share | | ADV006 | Hot `SELECT *` on a wide table (≥15 columns) | cost share, column count | | ADV007 | Add index on a hot join key with no existing index leading with it | cost share, NDV, row estimate, absence of a covering index | diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 90f4f1a..e257265 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -914,9 +914,11 @@ def _first_co_occurring( def propose_partial_indexes( usage: Sequence[ColumnUsage], facts: Mapping[Relation, TableFacts], + existing: Mapping[Relation, Sequence[PgIndex]], *, min_cost_share: float, min_rows: int = MIN_ROWS_FOR_INDEX, + have_index_data: bool = True, ) -> list[Proposal]: """ADV004 — index the hot equality column, restricted by a hot null-check predicate. @@ -926,6 +928,37 @@ def propose_partial_indexes( Gated on table size exactly as ADV001 is: both rules create an index, and below the floor a sequential scan is the right plan whichever rule proposed it. An unknown row count caps confidence at LOW rather than being assumed large. + + **This rule was the only index-creating rule that never consulted the existing-index list + at all**, so it could propose an index an existing one already serves and its rationale + said nothing about the gap — a check that silently did not run, which is the one thing + this rule set is built to refuse. It now runs `_covered` like ADV001, ADV007 and ADV008, + with two differences that follow from the proposal itself being *partial*: + + * **A plain index leading with the guarded column suppresses this proposal.** A partial + index's only advantage over such an index is size — the access path is already there, + since `WHERE leading = $1 AND guard IS NULL` can be served by a plain index on + `(leading)` with the null check applied as a filter. Size is exactly what this tool + cannot measure: nothing here knows what fraction of the table satisfies the guard, so + "smaller" is an assertion, not evidence, and it would be traded against a second + index's write cost on every insert and update. Worse, nothing downstream would catch + the pair: ADV003's redundant-prefix check is restricted to plain indexes, so a partial + index shadowed by a plain one is never flagged on a later run. Suppressing is therefore + the honest call, not merely the conservative one. + * **A *partial* index that leads with the same column is disclosed, not treated as + coverage.** `_covered` skips partial and expression indexes deliberately (see its + docstring), and for this rule that exclusion cuts the other way than it does for + ADV001: an existing partial index on the same column may be *precisely* this proposal + already applied. Nothing here compares predicates — the existing index's `WHERE` clause + is not parsed, and this proposal's guard is reconstructed from redacted usage — so + whether it is the same index is genuinely unknown and is stated as unknown, naming the + index so an operator can settle it in one glance. + + `have_index_data` is False when the existing-index catalog query was denied, and is + handled exactly as the other three rules handle it: the cost evidence is real so the + proposal survives, but confidence is capped at LOW and the rationale says which check + could not run. `existing` being empty cannot distinguish "no such index" from "could not + look", which is why the flag is separate from the mapping. """ proposals: list[Proposal] = [] for relation, items in sorted(_by_relation(usage).items()): @@ -957,13 +990,50 @@ def propose_partial_indexes( cost_share = max(leading.cost_share, guard.cost_share) if cost_share < min_cost_share: continue + table_indexes = existing.get(relation, ()) + # A plain index leading with this column already provides the access path; only the + # size differs, and this rule cannot measure that. See the docstring. + if _covered((leading.column,), table_indexes) is not None: + continue + # Not coverage, and not the same gap ADV001 discloses: an existing *partial* index + # leading with this column may be this very proposal, already applied. Predicates are + # not compared, so that is unknown rather than either answer. + partial_indexes = tuple( + index.name + for index in table_indexes + if index.is_partial and _is_prefix((leading.column,), index.columns) + ) + # Same whole-identifier matching as ADV001/ADV007/ADV008, for the same reason: a + # substring test reports an index on `lower(guid)` as "mentions id". + expression_indexes = tuple( + index.name + for index in table_indexes + if index.has_expressions and mentions_identifier(leading.column, index.definition or "") + ) predicate = _NULL_ROLE_PREDICATE[guard.role] rationale = ( "The hot predicates always pair this lookup with the same null check, " "so a partial index covers them at a fraction of the size." ) + if not have_index_data: + rationale += ( + " The existing-index list could not be read, so whether an index already " + "serves this lookup is unknown — check before applying." + ) if rows is None: rationale += _UNKNOWN_ROWS_NOTE + if partial_indexes: + rationale += ( + f" A partial index ({', '.join(partial_indexes)}) already leads with this " + "column; sqlquality does not compare its WHERE predicate to this proposal's, " + "so it cannot tell whether this index already exists — check before applying." + ) + if expression_indexes: + rationale += ( + f" An expression index ({', '.join(expression_indexes)}) mentions " + f"{leading.column}; sqlquality cannot tell whether it already serves this " + "lookup, so confirm before applying." + ) proposals.append( Proposal( code="ADV004", @@ -984,8 +1054,18 @@ def propose_partial_indexes( #: How many query groups filter on both columns together. This is what #: makes the proposal supported rather than a guess. "co_occurring_fingerprints": len(shared), + #: Named `partial_indexes_not_compared`, not ADV001's + #: `partial_indexes_skipped`: there the partial index is known not to + #: cover an unfiltered lookup, here it may be this exact proposal already + #: applied and the difference is that nobody compared the predicates. A + #: shared key name would have made two different facts indistinguishable + #: in `--json`, which renders evidence as bare `k=v` pairs. + "partial_indexes_not_compared": partial_indexes, + "expression_indexes": expression_indexes, }, - confidence=Confidence.LOW if rows is None else Confidence.MEDIUM, + confidence=( + Confidence.LOW if rows is None or not have_index_data else Confidence.MEDIUM + ), ddl=( f"CREATE INDEX ON {_qualified(relation.schema, relation.table)} " f"({_quote_ident(leading.column)}) " @@ -1930,7 +2010,13 @@ def propose( min_cost_share=min_cost_share, have_index_data=have_index_data, ), - *propose_partial_indexes(aggregation.usage, facts, min_cost_share=min_cost_share), + *propose_partial_indexes( + aggregation.usage, + facts, + existing, + min_cost_share=min_cost_share, + have_index_data=have_index_data, + ), *propose_sargability(aggregation.usage, workload, min_cost_share=min_cost_share), *propose_select_star( workload, facts, min_cost_share=min_cost_share, dialect=self.engine diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index ebee524..afc7a2a 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -888,6 +888,7 @@ def test_partial_index_proposed_for_a_hot_not_null_check(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -901,6 +902,7 @@ def test_partial_index_polarity_follows_the_predicate(): _usage(_ORDERS, "shipped_at", ColumnRole.NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(), + {}, min_cost_share=0.01, ) assert "IS NULL" in proposals[0].ddl @@ -927,6 +929,7 @@ def test_no_partial_index_when_the_columns_never_co_occur(): ), ], _facts_map(), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -946,6 +949,7 @@ def test_partial_index_reports_the_co_occurrence_that_justifies_it(): ), ], _facts_map(), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -968,6 +972,7 @@ def test_partial_index_picks_the_costliest_pair_that_actually_co_occurs(): ), ], _facts_map(columns=("status", "region", "shipped_at")), + {}, min_cost_share=0.01, ) assert proposals[0].evidence["columns"] == ("region",) @@ -986,6 +991,7 @@ def test_partial_index_is_suppressed_on_a_small_table(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(rows=50), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -999,6 +1005,7 @@ def test_partial_index_with_an_unknown_row_count_is_low_and_says_why(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(rows=None), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -1007,10 +1014,128 @@ def test_partial_index_with_an_unknown_row_count_is_low_and_says_why(): assert "unknown" in proposals[0].rationale.lower() +def _partial_usage(relation=None): + """The minimal co-occurring pair ADV004 needs: a hot equality column and a hot null check + on the same query group.""" + relation = relation or _ORDERS + return [ + _usage(relation, "status", ColumnRole.EQUALITY, cost_ms=90.0), + _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), + ] + + +def test_partial_index_is_suppressed_when_a_plain_index_already_leads_with_the_column(): + """ADV004 was the only index-creating rule that never called `_covered` at all. + + A plain index on `(status, created_at)` already provides the access path this proposal + asks for — `WHERE status = $1 AND shipped_at IS NOT NULL` reads it and applies the null + check as a filter — so the partial index buys size alone, and nothing here can measure + that against a second index's write cost. Nothing downstream catches the pair either: + ADV003's redundant-prefix check is restricted to plain indexes, so a partial index + shadowed by a plain one is never flagged on any later run. + """ + existing = {_ORDERS: (PgIndex("idx_plain", ("status", "created_at"), False, False, 5, 1),)} + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert proposals == [] + + +def test_partial_index_still_fires_when_no_existing_index_leads_with_the_column(): + """Control for the suppression above: it must key on the *leading* column, exactly as + `_covered` does for every other index-creating rule. An index on `(created_at, status)` + cannot be probed by `status` alone, so it is not coverage and the proposal stands.""" + existing = { + _ORDERS: (PgIndex("idx_wrong_order", ("created_at", "status"), False, False, 5, 1),) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"] + + +def test_partial_index_names_an_existing_partial_index_rather_than_comparing_predicates(): + """`_covered` skips partial indexes deliberately, and for *this* rule that cuts the other + way than it does for ADV001: an existing partial index leading with the same column may be + precisely this proposal, already applied. Nothing here parses its WHERE clause, and this + proposal's guard is reconstructed from redacted usage, so the honest report is "unknown, + here is its name" — not silence, and not a suppression the evidence cannot support. + """ + existing = { + _ORDERS: ( + PgIndex("idx_open", ("status",), False, False, 5, 1, is_partial=True, predicate="..."), + ) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"], "a partial index is not treated as coverage" + assert "idx_open" in proposals[0].rationale + assert "does not compare its WHERE predicate" in proposals[0].rationale + assert proposals[0].evidence["partial_indexes_not_compared"] == ("idx_open",) + assert "partial_indexes_skipped" not in proposals[0].evidence, ( + "a different fact from ADV001's: there the index is known not to cover an unfiltered " + "lookup, here nobody compared the predicates" + ) + + +def test_partial_index_names_an_expression_index_mentioning_the_same_column(): + """Same gap ADV001, ADV007 and ADV008 already disclose: an expression index's `columns` + tuple understates it, so `_covered` cannot match against it and silence would leave the + operator to discover the overlap themselves.""" + existing = { + _ORDERS: ( + PgIndex( + "idx_lower_status", + (), + False, + False, + 5, + 1, + has_expressions=True, + definition="CREATE INDEX idx_lower_status ON orders (lower(status))", + ), + ) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"] + assert "idx_lower_status" in proposals[0].rationale + assert proposals[0].evidence["expression_indexes"] == ("idx_lower_status",) + + +def test_partial_index_discloses_that_the_existing_index_list_could_not_be_read(): + """The confidence discipline the whole rule set turns on: a check that could not run is + disclosed and caps confidence, rather than being silently skipped. ADV001, ADV007 and + ADV008 all did this; ADV004 did not even have the flag.""" + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), {}, min_cost_share=0.01, have_index_data=False + ) + assert codes(proposals) == ["ADV004"], "the cost evidence is real, so the advice survives" + assert proposals[0].confidence is Confidence.LOW + assert "existing-index list could not be read" in proposals[0].rationale + + +def test_partial_index_is_medium_and_silent_about_coverage_when_the_check_did_run(): + """Control for the two disclosures above: neither sentence may appear on the ordinary run + where the list was read and nothing covered the candidate — a rule that always says a + check was skipped tells the operator nothing.""" + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), {}, min_cost_share=0.01, have_index_data=True + ) + assert proposals[0].confidence is Confidence.MEDIUM + assert "could not be read" not in proposals[0].rationale + assert "does not compare" not in proposals[0].rationale + assert proposals[0].evidence["partial_indexes_not_compared"] == () + assert proposals[0].evidence["expression_indexes"] == () + + def test_no_partial_index_without_an_equality_column_to_index(): proposals = propose_partial_indexes( [_usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4)], _facts_map(), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -1990,6 +2115,7 @@ def test_partial_index_ddl_is_schema_qualified(): _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(relation), + {}, min_cost_share=0.01, ) assert proposals[0].ddl.startswith('CREATE INDEX ON "analytics"."orders" ("status")') @@ -2057,6 +2183,7 @@ def test_adv004_evidence_reports_the_bare_table_name_and_its_own_schema(): _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(relation), + {}, min_cost_share=0.01, ) assert proposals[0].evidence["schema"] == "staging" From 286247e23aab98d2d2cadc3b676386e29ad2a31f Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 22:55:24 +0200 Subject: [PATCH 3/7] fix(advise): close the render_ddl line-break guard on all eight remaining codepoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `render_ddl` implementations guarded with `"\n" in ddl or "\r" in ddl`, while every place that actually splits the text — `_comment_lines`, `_is_fully_commented`, and the tests asserting no bare line is ever emitted — uses `splitlines()`. That splits on eight further codepoints: \v, \f, \x1c, \x1d, \x1e, \x85, U+2028 and U+2029. Postgres permits every one of them inside a quoted identifier, so an introspected name carrying one produced a second physical line in the generated file that the guard never examined: the NOT-RENDERED fallback was skipped and the tail of the statement was written out looking like a bare statement of its own — in the one file whose stated purpose is that nothing unintended is executable. Closed for both adapters by deriving the predicate from `splitlines()` itself (`_has_line_break`) rather than restating its character set. That keeps the guard and the splitting in lockstep by construction: there is no second list to forget, and a future CPython recognising one more boundary cannot reopen the hole. Empty text answers False — no lines is not a line break, which the bare `splitlines() != [text]` comparison gets wrong. Pinned one parametrized case per codepoint per adapter, not one case built from a string containing all eight: a guard handling six of them would still trip on such a string and pass. Each case also asserts its own premise (`len(ddl.splitlines()) == 2`), so a codepoint that turned out not to be a boundary would fail loudly instead of passing vacuously. Verified by mutation: restoring the old two-character expression at both call sites kills exactly 16 tests, 8 per adapter, one per codepoint, and reducing `_has_line_break` itself kills those 16 plus its own unit test — with the unmutated tree green. Behaviour is unchanged on both adapters for every identifier that does not contain one of the eight, so no existing test needed adjusting. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 ++++ src/sqlquality/workload/postgres.py | 28 ++++++++++++-- src/sqlquality/workload/redshift.py | 14 +++---- tests/test_workload_redshift_rules.py | 33 ++++++++++++++++ tests/test_workload_rules.py | 56 +++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ca9a46..3e5d62d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `--ddl`'s guarantee that every line of the generated script is either an intended statement + or a `--` comment now holds for all ten codepoints `str.splitlines()` treats as a line + boundary, on both the Postgres and Redshift renderers. The guard tested only `\n` and `\r`, + while everything that splits the text uses `splitlines()`, so an introspected identifier + containing `\v`, `\f`, `\x1c`, `\x1d`, `\x1e`, `\x85`, `U+2028` or `U+2029` — all legal + inside a quoted Postgres identifier — produced a second physical line the guard never + examined, and the tail of the statement was emitted looking like a bare statement of its own. - ADV004 (partial index) now consults the existing-index list, which it was alone among the index-creating rules in never doing. A plain index leading with the guarded column now suppresses the proposal — that index already serves the lookup, and the partial index's only diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index e257265..91a6b20 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -1269,6 +1269,30 @@ def _comment_lines(text: str) -> list[str]: return [f"-- {line}" for line in text.splitlines()] +def _has_line_break(text: str) -> bool: + """True when `text` would occupy more than one physical line in the rendered script. + + Defined as "`str.splitlines` disagrees that this is exactly one line" rather than as a + membership test against a list of characters, and that is the whole point of the function. + Both `render_ddl` implementations used to guard with `"\\n" in ddl or "\\r" in ddl`, while + every place that actually splits the text — `_comment_lines`, `_is_fully_commented`, and + the tests asserting no bare line is ever emitted — uses `splitlines()`, which also splits + on `\\v`, `\\f`, `\\x1c`, `\\x1d`, `\\x1e`, `\\x85`, `\\u2028` and `\\u2029`. An identifier + carrying any of those eight produced a second physical line in the file that the guard + never examined, so `render_ddl` skipped the NOT-RENDERED fallback and emitted something a + reader sees as a bare, statement-shaped line — exactly the invariant this script format + states unconditionally. Postgres permits every one of them inside a quoted identifier. + + Deriving the answer from `splitlines` rather than restating its character set keeps the + guard and the splitting in lockstep by construction: a future CPython that recognises one + more line boundary cannot reopen the hole, and there is no second list to forget to update. + + Empty text is not a line break — it has no lines at all — and is answered False rather + than by the raw `splitlines() != [text]` comparison, which would say True for `""`. + """ + return bool(text) and text.splitlines() != [text] + + def _is_fully_commented(ddl: str) -> bool: """True when every physical line of `ddl` already begins with `--`. @@ -2046,9 +2070,7 @@ def render_ddl(self, proposals: list[Proposal]) -> str: for proposal in proposals: if not proposal.ddl: continue - if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( - proposal.ddl - ): + if _has_line_break(proposal.ddl) and not _is_fully_commented(proposal.ddl): # An identifier containing a line break cannot be emitted as a single-line # statement. Quoting already makes it *semantically* safe — psql parses the # whole thing as one quoted identifier, so nothing extra executes — but the diff --git a/src/sqlquality/workload/redshift.py b/src/sqlquality/workload/redshift.py index 2d3c2f7..b7cb2a9 100644 --- a/src/sqlquality/workload/redshift.py +++ b/src/sqlquality/workload/redshift.py @@ -81,6 +81,7 @@ from sqlquality.workload.postgres import ( _by_relation, _comment_lines, + _has_line_break, _is_fully_commented, _sentences, ) @@ -1531,10 +1532,11 @@ def render_ddl(self, proposals: list[Proposal]) -> str: tool generated. Reuses rather than reimplements: `cost_share_of` (bool-safe cost-share formatting) - and `_is_fully_commented`'s line-break guard, both imported from `models.py` and - `postgres.py` respectively — the same protection that guarantees a hostile - identifier (one carrying an embedded newline, `\\r`, or a `--`/`;` sequence) cannot - produce a bare, executable-looking line in this script either. See + and `_has_line_break`/`_is_fully_commented`'s line-break guard, imported from + `models.py` and `postgres.py` respectively — the same protection that guarantees a + hostile identifier (one carrying any codepoint `str.splitlines` breaks on, or a + `--`/`;` sequence) cannot produce a bare, executable-looking line in this script + either. See `test_render_ddl_never_emits_a_bare_uncommented_line` in `tests/test_workload_redshift_rules.py`. """ @@ -1564,9 +1566,7 @@ def render_ddl(self, proposals: list[Proposal]) -> str: for proposal in proposals: if not proposal.ddl: continue - if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( - proposal.ddl - ): + if _has_line_break(proposal.ddl) and not _is_fully_commented(proposal.ddl): # Identical guard to `PostgresWorkloadAdapter.render_ddl`: an identifier # carrying a literal line break is already semantically safe once quoted # (the whole thing parses as one identifier), but a raw newline would still diff --git a/tests/test_workload_redshift_rules.py b/tests/test_workload_redshift_rules.py index b049782..4cd4f06 100644 --- a/tests/test_workload_redshift_rules.py +++ b/tests/test_workload_redshift_rules.py @@ -45,6 +45,13 @@ propose_sortkey, ) +# Sibling test module, not a package import: `tests/` has no `__init__.py`, so pytest puts it +# on `sys.path` and its modules are importable by bare name. Imported rather than copied +# because two hand-maintained lists of invisible control characters are two lists free to +# drift apart, and the reason this file asserts the same property again is that the *call site* +# differs (each adapter's `render_ddl` calls the shared guard separately), not the character set. +from test_workload_rules import EXTRA_LINE_BREAKS # noqa: E402 + R = Relation(schema="public", table="orders") R2 = Relation(schema="public", table="customers") @@ -1416,6 +1423,32 @@ def test_render_ddl_never_emits_a_bare_uncommented_line_for_a_hostile_identifier assert _uncommented(script) == [], script +@pytest.mark.parametrize( + ("char", "name"), EXTRA_LINE_BREAKS, ids=[name for _char, name in EXTRA_LINE_BREAKS] +) +def test_every_splitlines_line_break_in_an_identifier_reaches_the_not_rendered_fallback(char, name): + """The same hole, closed on this adapter too, and pinned here rather than only on the + Postgres side: the guard is shared code (`_has_line_break`, imported from `postgres.py`), + but the two `render_ddl` implementations call it in two separate places, so a fix applied + to one is not a fix applied to both. The codepoint list is imported from the Postgres + renderer's tests deliberately — two copies would be free to drift apart, and this file's + whole reason for asserting it again is that the *call site* is different, not the set. + """ + ddl = f'ALTER TABLE "public"."or{char}ders" ALTER SORTKEY ("created_at");' + assert len(ddl.splitlines()) == 2, f"{name} is not a splitlines boundary" + proposal = Proposal( + code="ADV101", + title="Consider SORTKEY on public.orders(created_at)", + rationale="r", + evidence={"schema": "public", "table": "orders", "cost_share": 0.4}, + confidence=Confidence.MEDIUM, + ddl=ddl, + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "NOT RENDERED" in script, name + assert _uncommented(script) == [], script + + def test_render_ddl_emits_a_pre_commented_multiline_block_verbatim(): """`_is_fully_commented`, reused rather than reimplemented: a `ddl` that already reads as a `--`-commented, multi-line disclosure on every line is not the identifier-with-a- diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index afc7a2a..c5f42ac 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -1,5 +1,7 @@ from pathlib import Path +import pytest + from sqlquality.models import ( Aggregation, ColumnRole, @@ -15,6 +17,7 @@ from sqlquality.workload.postgres import ( PgIndex, PostgresWorkloadAdapter, + _has_line_break, _is_fully_commented, _quote_ident, propose_grouping_indexes, @@ -2890,6 +2893,59 @@ def test_a_carriage_return_in_an_identifier_is_not_rendered_as_a_statement(): assert _uncommented(script) == [], script +#: Every codepoint `str.splitlines()` treats as a line boundary *other than* `\n` and `\r` — +#: the only two the guard used to test for. Postgres permits all of them inside a quoted +#: identifier, so each one is a real way for an introspected name to occupy two physical lines +#: in the generated script. Parametrized one per codepoint, deliberately not asserted as a +#: block: a guard that handled six of the eight would still pass a single test built from a +#: string containing all of them, since one unhandled codepoint is enough to trip it. +EXTRA_LINE_BREAKS = [ + ("\v", "VT-000B"), + ("\f", "FF-000C"), + ("\x1c", "FS-001C"), + ("\x1d", "GS-001D"), + ("\x1e", "RS-001E"), + ("\x85", "NEL-0085"), + ("\u2028", "LS-2028"), + ("\u2029", "PS-2029"), +] +_BREAK_IDS = [name for _char, name in EXTRA_LINE_BREAKS] + + +@pytest.mark.parametrize(("char", "name"), EXTRA_LINE_BREAKS, ids=_BREAK_IDS) +def test_every_splitlines_line_break_in_an_identifier_reaches_the_not_rendered_fallback(char, name): + """The guard tested `"\\n" in ddl or "\\r" in ddl`, but every place that actually splits + the text uses `splitlines()`, which breaks on eight further codepoints. An identifier + carrying one of them therefore produced a second physical line in the file that the guard + never examined: the fallback was skipped and the tail of the statement was emitted as a + bare, statement-shaped line, in the one file whose stated purpose is that nothing + unintended is executable. + """ + ddl = f'CREATE INDEX ON "main"."or{char}ders" ("status");' + # The premise, asserted rather than assumed. If this codepoint were not in fact a + # `splitlines()` boundary, everything below would pass while proving nothing at all — the + # exact way a guard test can look green and discriminate nothing. + assert len(ddl.splitlines()) == 2, f"{name} is not a splitlines boundary" + script = PostgresWorkloadAdapter().render_ddl([_ddl_proposal(ddl=ddl)]) + assert "NOT RENDERED" in script, name + assert _uncommented(script) == [], script + + +def test_has_line_break_agrees_with_splitlines_on_every_boundary(): + """The guard is derived from `splitlines()` rather than from a restated character list, + which is what keeps it in lockstep with `_comment_lines`, `_is_fully_commented` and the + tests. Pinned directly as well as through the renderer, since this is the predicate the + whole "nothing unintended is executable" promise now rests on. + """ + for char, name in [("\n", "LF-000A"), ("\r", "CR-000D"), *EXTRA_LINE_BREAKS]: + assert _has_line_break(f"a{char}b") is True, name + assert _has_line_break(f"trailing{char}") is True, name + assert _has_line_break('CREATE INDEX ON "main"."orders" ("status");') is False + assert _has_line_break("a\tb c") is False, "a tab or a space is not a line boundary" + # No lines at all is not a line break, and `splitlines() != [text]` alone would say it is. + assert _has_line_break("") is False + + def test_is_fully_commented_requires_every_line_to_be_a_comment(): """The guard that lets a pre-commented multi-line `ddl` skip the NOT-RENDERED fallback. From 88102c414e66a8c1d2bd317ccc5fb9dc6cfcb587 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 23:02:38 +0200 Subject: [PATCH 4/7] test(integration): move off a colliding host port and verify which server answered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compose file published 55432, which an unrelated `postgres:16` container held on a development machine for the whole of this feature's work. When that happens `docker compose up` does not bind and does not fail: it reports success, the port keeps belonging to whatever bound it first, and every test in the package silently talks to a stranger's database. It surfaced as a password-authentication failure that the author and three reviewers all read as a code bug. Both halves of the fix, because either alone leaves it silent: * The published port is now 27432 — unregistered, outside the ephemeral range on Linux (32768+) and macOS (49152+) so no outbound socket can take it first, and not one of the ports Postgres tooling gravitates to. * `live_dsn` verifies it reached *this* suite's server instead of trusting the connection. Two cheap reads in one round trip: the database name, and `shared_preload_libraries` containing pg_stat_statements. A mismatch is a hard failure — a skip is how the original problem stayed invisible — naming a port collision as the likely cause and how to look for it, since compose's own output never mentions it. An unreachable port stays a skip: "no Docker" remains a supported state for the default suite. The second check is the one that matters. Measured against a plain `postgres:16` carrying the same database name, the old `SELECT 1` check passed, `CREATE EXTENSION pg_stat_statements` succeeded, and the first real failure came a hundred lines into the seed fixture as "pg_stat_statements must be loaded via shared_preload_libraries" — an error that says nothing about the port. Both failure modes were reproduced live against real containers and now fail at the fixture with the diagnosis. The messages are also credential-safe, since CI is about to print them: the DSN is described as `host:port/database` rather than echoed, and the driver's own text is passed through the project's own `scrub` — an authentication failure is the most common real connect failure and the one place a password can surface. A keyword-form DSN is described generically rather than dissected, because `urlparse` puts the whole string, password included, in `path`. The checks are module-level pure functions so the default suite can exercise the server-is-wrong path without Docker: a check that only runs when the server is right is a check nobody ever sees run. A drift test pins the fixture's port against the port compose actually publishes, since two files naming one port with nothing tying them together is how this becomes invisible again. Co-Authored-By: Claude Opus 5 --- CONTRIBUTING.md | 9 +- tests/integration/conftest.py | 139 +++++++++++++- tests/integration/docker-compose.yml | 15 +- .../integration/test_redshift_connect_live.py | 9 +- tests/test_integration_fixture.py | 172 ++++++++++++++++++ 5 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 tests/test_integration_fixture.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c376e3..d2c035d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,4 +62,11 @@ docker compose -f tests/integration/docker-compose.yml down These are deselected by default, so `uv run pytest` stays green without Docker. Point them at your own server with `SQLQUALITY_TEST_DSN`. They need the `postgres` extra -(`uv sync --extra postgres`). +(`uv sync --extra postgres`), and a server with `pg_stat_statements` in +`shared_preload_libraries` — `CREATE EXTENSION` alone is not enough, which is why the compose +file passes it as a server flag. + +The compose file publishes host port **27432**. If something else already holds it, `docker +compose up` neither binds nor fails, and the suite would talk to whatever is listening — so +the fixture checks which server answered and fails with that diagnosis rather than producing +a puzzle. Free the port (`docker ps --filter publish=27432`) or set `SQLQUALITY_TEST_DSN`. diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 7452f61..bcec715 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -14,20 +14,112 @@ Bring the server up with: docker compose -f tests/integration/docker-compose.yml up -d uv run pytest -m integration + +`live_dsn` verifies it reached the server compose started rather than trusting the +connection — a published port that is already taken makes `docker compose up` a silent no-op. """ from __future__ import annotations import os from pathlib import Path +from urllib.parse import unquote, urlparse import pytest -DEFAULT_DSN = "postgresql://postgres:sqlquality@127.0.0.1:55432/sqlquality_test" +from sqlquality.workload.secrets import scrub + +#: Must match the host port `docker-compose.yml` publishes — see the comment there for why it +#: is 27432 and not 55432. +DEFAULT_PORT = 27432 +#: The database `docker-compose.yml` creates. Checked, not assumed: see `live_dsn`. +EXPECTED_DATABASE = "sqlquality_test" +DEFAULT_DSN = f"postgresql://postgres:sqlquality@127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" _PACKAGE_DIR = Path(__file__).parent +def describe_dsn(dsn: str) -> str: + """`host:port/database` — where we connected, with the credentials left out. + + These messages are printed by CI now, and `SQLQUALITY_TEST_DSN` can point anywhere, so the + DSN itself must not be echoed: the project's rule that no credential appears in any output + applies to a fixture's failure text as much as to the tool's. + + A keyword-form DSN (`host=... password=...`) is not a URL, and `urlparse` puts the whole + string — password included — in `path`, so anything but a recognised URI scheme with a + hostname is described generically rather than picked apart. Host and port are safe by + construction; `path` is only ever the database name once a scheme parsed. + """ + parsed = urlparse(dsn) + if parsed.scheme not in {"postgres", "postgresql"} or not parsed.hostname: + return "the server SQLQUALITY_TEST_DSN points at" + database = (parsed.path or "").lstrip("/") or "(no database in the DSN)" + return f"{parsed.hostname}:{parsed.port or 5432}/{database}" + + +def dsn_secrets(dsn: str) -> tuple[str, ...]: + """The DSN's password, in both the encoded and decoded forms a driver may echo. + + Same reasoning as `sqlquality.workload.secrets.secrets_for`, which cannot be reused + directly: it takes a `ConnectionParams`, and reconstructing one here to reach one field + would couple this fixture to a model it has no other use for. The *scrubbing* is reused — + only the token extraction is local. + """ + encoded = urlparse(dsn).password + if not encoded: + return () + decoded = unquote(encoded) + return (encoded, decoded) if decoded != encoded else (encoded,) + + +def server_mismatches(database: str, preloaded: str) -> list[str]: + """Every reason the server we reached is not the one this suite needs, or `[]`. + + A module-level function rather than inline fixture code so it can be exercised without + Docker: the whole point is what happens when the server is *wrong*, and a check that only + runs when the server is right is a check nobody ever sees run. + + Both facts are cheap single-round-trip reads and both discriminate a stranger's Postgres + from this suite's. `shared_preload_libraries` is the one that matters most: without + `pg_stat_statements` preloaded, `CREATE EXTENSION` still succeeds and every later read of + its view fails, deep inside a test, with an error that says nothing about the real cause. + """ + problems = [] + if database != EXPECTED_DATABASE: + problems.append(f"connected to database {database!r}, expected {EXPECTED_DATABASE!r}") + if "pg_stat_statements" not in preloaded: + problems.append( + "the server has no pg_stat_statements in shared_preload_libraries " + f"(it reports {preloaded!r}), so the workload tests cannot read query history" + ) + return problems + + +def collision_hint(dsn: str, problems: list[str]) -> str: + """The failure message for a server that answered but is the wrong one. + + It names a port collision explicitly. That is not a guess dressed up as a diagnosis: it is + the *only* way this state is normally reached, and the reason it cost four people time was + that the symptom (a password-authentication failure, or an unexpected schema) points + anywhere but at the port. It also says *how to look*, since the collision is invisible from + compose's own output. + + The DSN is described, not printed — see `describe_dsn`. + """ + return ( + f"reached a Postgres at {describe_dsn(dsn)}, but it is not this suite's server:\n" + + "\n".join(f" - {p}" for p in problems) + + f"\nThe likely cause is a port collision: something else already holds {DEFAULT_PORT}, " + "and `docker compose up` neither binds nor fails in that case, so the suite connects to " + "whatever is listening.\n" + f"Check `docker ps --filter publish={DEFAULT_PORT}`, then either free the port or point " + "SQLQUALITY_TEST_DSN at the right server.\n" + "Bring the intended one up with: " + "docker compose -f tests/integration/docker-compose.yml up -d" + ) + + def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: """Mark every test collected under this package as `integration`. @@ -43,7 +135,30 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: @pytest.fixture(scope="session") def live_dsn() -> str: - """A reachable Postgres, or skip with an actionable message.""" + """A reachable Postgres that is verifiably *this* suite's server, or skip. + + **Connecting is not the same as connecting to the right server, and the difference is not + cosmetic.** A published host port that is already taken does not stop `docker compose up`: + compose reports success, the port keeps belonging to whatever bound it first, and every + test in this package silently talks to a stranger's database. That happened for the whole + of this feature's development — an unrelated `postgres:16` container held the old 55432 — + and it surfaced as a password-authentication failure that three reviewers and the author + all read as a code bug. + + So two cheap facts are checked before any test runs, and a mismatch is a hard **failure**, + not a skip: a skip is how the original problem stayed invisible, and by the time this + fixture runs the caller has explicitly asked for `-m integration`. + + * the database name, which pins that this is the server compose created rather than one + that merely answers on the port; + * `shared_preload_libraries`, because `pg_stat_statements` cannot be loaded by `CREATE + EXTENSION` alone. Without it the extension installs and then every read of its view + fails deep inside a test, which says nothing about the real cause. + + An unreachable port stays a *skip*: "no Docker" is the documented, supported state for a + contributor running the default suite. Only a server that answers and is the wrong one + fails. + """ psycopg = pytest.importorskip( "psycopg", reason="integration tests need the postgres extra: uv sync --extra postgres" ) @@ -52,12 +167,26 @@ def live_dsn() -> str: try: with psycopg.connect(dsn, connect_timeout=3) as conn: with conn.cursor() as cur: - cur.execute("SELECT 1") + cur.execute( + "SELECT current_database(), current_setting('shared_preload_libraries')" + ) + database, preloaded = cur.fetchone() except Exception as exc: # driver-specific; the message is what matters + # The driver's own text is scrubbed with the project's own helper before being shown: + # the most common real connect failure *is* an authentication failure, and this message + # now reaches CI logs. pytest.skip( - f"no Postgres at {dsn}: {exc}\n" - "start one with: docker compose -f tests/integration/docker-compose.yml up -d" + f"no Postgres at {describe_dsn(dsn)}: {scrub(str(exc), dsn_secrets(dsn))}\n" + "start one with: docker compose -f tests/integration/docker-compose.yml up -d\n" + f"if that was already running, something else may hold port {DEFAULT_PORT}: compose " + "neither binds an already-taken port nor fails, so this can equally be a stranger's " + f"server rejecting our credentials. Check `docker ps --filter publish={DEFAULT_PORT}` " + f"and `lsof -nP -iTCP:{DEFAULT_PORT} -sTCP:LISTEN`." ) + + problems = server_mismatches(database, preloaded) + if problems: + pytest.fail(collision_hint(dsn, problems)) return dsn diff --git a/tests/integration/docker-compose.yml b/tests/integration/docker-compose.yml index 3492181..23be1b5 100644 --- a/tests/integration/docker-compose.yml +++ b/tests/integration/docker-compose.yml @@ -13,7 +13,20 @@ services: - -c - pg_stat_statements.track=all ports: - - "55432:5432" + # 27432, not 55432, and not any other digit-shuffle of 5432. 55432 collided with an + # unrelated `postgres:16` container on a development machine for the whole of this + # feature's work, and when that happens `docker compose up` does not bind: it reports + # success, the port stays owned by the other container, and the suite talks to whatever + # is already listening. That surfaced as a password-authentication failure that read as a + # code bug and cost four people time. 27432 is unregistered, is outside the ephemeral + # port range on both Linux (32768+) and macOS (49152+) so no outbound socket can take it + # first, and is not one of the ports Postgres tooling gravitates to (5432, 5433, 54320, + # 54321, 55432, 15432). + # + # A port can still collide, so `live_dsn` verifies it reached *this* server rather than + # trusting the connection — see tests/integration/conftest.py. Do not change this number + # without changing DEFAULT_PORT there; set SQLQUALITY_TEST_DSN to point elsewhere instead. + - "27432:5432" healthcheck: test: ["CMD-SHELL", "pg_isready -U postgres -d sqlquality_test"] interval: 2s diff --git a/tests/integration/test_redshift_connect_live.py b/tests/integration/test_redshift_connect_live.py index bba6083..00107ac 100644 --- a/tests/integration/test_redshift_connect_live.py +++ b/tests/integration/test_redshift_connect_live.py @@ -22,10 +22,11 @@ def _with_wrong_password(dsn: str) -> str: """Swap whatever password a DSN carries for a wrong one, however it is shaped. A DSN-shaped string substitution (``dsn.replace(":sqlquality@", ":wr0ng-p4ss@")``) is - a silent no-op whenever the real password is not literally ``sqlquality`` — exactly - the normal case here: host port 55432 is frequently held by an unrelated container on - this machine, so this suite is routinely run against a custom - ``SQLQUALITY_TEST_DSN`` with different credentials. Parsing the DSN's authority + a silent no-op whenever the real password is not literally ``sqlquality`` — a case that + has to keep working: the published host port can be held by an unrelated container (which + is why it moved off 55432 and why ``live_dsn`` now verifies which server answered), so + this suite is routinely run against a custom ``SQLQUALITY_TEST_DSN`` with different + credentials. Parsing the DSN's authority component and re-encoding it with a substituted password works for whatever DSN is handed in, not only the one hardcoded default. """ diff --git a/tests/test_integration_fixture.py b/tests/test_integration_fixture.py new file mode 100644 index 0000000..0e8cb68 --- /dev/null +++ b/tests/test_integration_fixture.py @@ -0,0 +1,172 @@ +"""The integration suite's own preflight checks, tested without Docker. + +These run in the *default* suite deliberately. The checks in `tests/integration/conftest.py` +exist for the case where the server is wrong, so testing them only from a run against a +correct server would never exercise them at all — and they would be free to rot until the next +time somebody lost a day to a port collision. + +Nothing here imports psycopg or connects to anything: `server_mismatches` and `collision_hint` +are pure functions over two strings, which is why they were lifted out of the fixture body. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import yaml + +_TESTS_DIR = Path(__file__).parent +if str(_TESTS_DIR) not in sys.path: # pragma: no cover - pytest normally does this itself + sys.path.insert(0, str(_TESTS_DIR)) + +from integration.conftest import ( # noqa: E402 + DEFAULT_DSN, + DEFAULT_PORT, + EXPECTED_DATABASE, + collision_hint, + describe_dsn, + dsn_secrets, + server_mismatches, +) + +_COMPOSE = _TESTS_DIR / "integration" / "docker-compose.yml" +_PRELOADED = "pg_stat_statements" + + +def _published_ports() -> list[str]: + compose = yaml.safe_load(_COMPOSE.read_text(encoding="utf-8")) + return list(compose["services"]["postgres"]["ports"]) + + +def test_the_fixtures_port_matches_the_port_compose_actually_publishes(): + """Two files naming the same port, and nothing tying them together. + + If they drift, `docker compose up` starts a server the fixture never looks at, and the + fixture skips with "no Postgres" — a green run that ran none of the 23 live tests. That is + the same class of invisible failure the port change was made to end, so it is pinned rather + than left to review. + """ + assert _published_ports() == [f"{DEFAULT_PORT}:5432"] + assert f":{DEFAULT_PORT}/" in DEFAULT_DSN + + +def test_compose_still_publishes_a_port_no_common_postgres_tooling_claims(): + """The port change is the first half of the fix and is otherwise unpinned: reverting it to + 55432 (or any of the other ports Postgres tooling gravitates to) would leave every test + green while restoring the collision. Also asserts it is outside the ephemeral range on + Linux and macOS, so no outbound socket can take it before compose binds.""" + assert DEFAULT_PORT not in {5432, 5433, 15432, 54320, 54321, 55432} + assert 1024 < DEFAULT_PORT < 32768 + + +def test_compose_preloads_pg_stat_statements_with_full_tracking(): + """What the fixture's `shared_preload_libraries` check is checking *for*. Dropping either + `-c` flag from the compose command leaves a server that accepts connections and then fails + every workload read.""" + compose = yaml.safe_load(_COMPOSE.read_text(encoding="utf-8")) + command = compose["services"]["postgres"]["command"] + assert f"shared_preload_libraries={_PRELOADED}" in command + assert "pg_stat_statements.track=all" in command + assert compose["services"]["postgres"]["environment"]["POSTGRES_DB"] == EXPECTED_DATABASE + + +def test_the_expected_server_reports_no_mismatches(): + """The control. A check that fails on everything is not a check — it would turn every + correct run into a hard failure.""" + assert server_mismatches(EXPECTED_DATABASE, f"{_PRELOADED},auto_explain") == [] + assert server_mismatches(EXPECTED_DATABASE, _PRELOADED) == [] + + +def test_a_stranger_on_the_port_is_caught_by_its_database_name(): + """The measured failure: an unrelated `postgres:16` held the port, so the suite connected + to a different database entirely (or was refused by it). Connecting proves nothing about + *what* answered.""" + [problem] = server_mismatches("some_other_app", _PRELOADED) + assert "some_other_app" in problem + assert EXPECTED_DATABASE in problem + + +def test_a_server_without_pg_stat_statements_preloaded_is_caught(): + """A same-named database on a plain `postgres:16` is the hardest collision to spot: it + connects, it seeds, and then it fails inside a test with an error about the extension that + says nothing about the port. `CREATE EXTENSION` cannot fix this — the library has to be + loaded at server start.""" + [problem] = server_mismatches(EXPECTED_DATABASE, "auto_explain") + assert "shared_preload_libraries" in problem + assert "auto_explain" in problem, "name what the server actually reports" + + +def test_both_mismatches_are_reported_together_rather_than_one_at_a_time(): + """Reporting only the first would send someone to fix the database name and then hit the + preload failure on the next run.""" + assert len(server_mismatches("postgres", "")) == 2 + + +def test_the_failure_message_names_a_port_collision_as_the_likely_cause(): + """The whole point of the message. The symptom — a password failure, or an unexpected + schema — points anywhere but at the port, which is why this cost the author and three + reviewers time. It must also say how to look, not just what happened. + """ + message = collision_hint(DEFAULT_DSN, server_mismatches("other", "")) + assert "port collision" in message + assert str(DEFAULT_PORT) in message + assert f"docker ps --filter publish={DEFAULT_PORT}" in message + assert "SQLQUALITY_TEST_DSN" in message + assert "docker compose -f tests/integration/docker-compose.yml up -d" in message + assert "neither binds nor fails" in message, ( + "the non-obvious fact that makes this diagnosable: compose does not report the collision" + ) + + +def test_the_failure_message_carries_no_credential(): + """These messages reach CI logs, and `SQLQUALITY_TEST_DSN` can point anywhere, so the DSN + must be described rather than echoed — the project's no-credential-in-any-output rule + applies to a fixture's failure text too.""" + message = collision_hint(DEFAULT_DSN, server_mismatches("other", "")) + assert "postgres:sqlquality@" not in message + assert "sqlquality@" not in message + # Still says where it connected: a message that redacts the location too is unactionable. + assert f"127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" in message + + +def test_describe_dsn_keeps_the_location_and_drops_the_credentials(): + assert describe_dsn(DEFAULT_DSN) == f"127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" + assert describe_dsn("postgresql://user:pw@db.example:6543/analytics") == ( + "db.example:6543/analytics" + ) + # No port in the DSN means libpq's default, which is what the reader needs told. + assert describe_dsn("postgresql://u:pw@db.example/analytics") == "db.example:5432/analytics" + + +def test_describe_dsn_refuses_to_pick_apart_a_keyword_form_dsn(): + """`urlparse` puts a whole keyword-form DSN — password included — in `path`, so anything + that is not a recognised URI is described generically instead of dissected. Getting this + wrong prints the password verbatim, which is precisely the failure being guarded.""" + keyword = "host=db.example port=6543 dbname=analytics user=u password=hunter2" + described = describe_dsn(keyword) + assert "hunter2" not in described + assert "password" not in described + assert described == "the server SQLQUALITY_TEST_DSN points at" + + +def test_dsn_secrets_yields_the_password_in_both_the_forms_a_driver_may_echo(): + """Same trap `secrets_for` documents: `urlparse` returns the password still + percent-encoded, while libpq decodes it before authenticating, so an auth-failure message + carries the *decoded* form and a token of only the encoded one never matches.""" + assert dsn_secrets("postgresql://u:p%40ss@h/db") == ("p%40ss", "p@ss") + assert dsn_secrets("postgresql://u:plain@h/db") == ("plain",) + assert dsn_secrets("postgresql://h/db") == () + + +def test_a_driver_message_echoing_the_password_is_scrubbed_before_it_is_shown(): + """The measured failure was an authentication failure, whose text is the one place a + password can surface. Asserted through the same `scrub` the tool uses, over a message shaped + like libpq's own.""" + from sqlquality.workload.secrets import scrub + + dsn = "postgresql://postgres:s3cretpw@127.0.0.1:27432/sqlquality_test" + libpq = 'connection failed: password authentication failed for user "postgres" (s3cretpw)' + scrubbed = scrub(libpq, dsn_secrets(dsn)) + assert "s3cretpw" not in scrubbed + assert "password authentication failed" in scrubbed From 3bc614eca3dc97bf314f5ae2b4426b8b1ebd1089 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 23:09:40 +0200 Subject: [PATCH 5/7] ci: run the integration tests in a job with a Postgres service container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 23 tests under tests/integration/ were deselected by default and ran only on a contributor's own machine. That gap was not theoretical: this feature's live suite was repeatedly the only thing that caught a bug no fixture could reach — a `reltuples = -1` sentinel that read as "tiny table" and silently suppressed every proposal, redaction dismembering `$N` placeholders, a `toplevel` filter that made a hot function-wrapped query vanish while leaving confidently-wrong advice behind, and a workload statement that failed on the wire for every default run. Each was invisible to `pytest` and to review. A service container cannot be given a command: GitHub's `services:` schema has no `command`/`entrypoint` key, and `options` is passed to `docker create` before the image, so the compose file's `-c shared_preload_libraries=...` flags cannot be expressed there. The service therefore provides the lifecycle and the health check, and a step applies both settings with ALTER SYSTEM and restarts the container — a reload is not enough, since both are postmaster-level and `pg_reload_conf()` leaves them inactive with no error. One `psql -c` per statement, because psql wraps multiple `-c` options in a single implicit transaction and ALTER SYSTEM cannot run inside one. The step then asserts both settings took effect, so a failure names the cause instead of surfacing deep inside a seed fixture. The job fails if the suite skipped or executed nothing. That is the load-bearing part: every test in the package skips itself when no Postgres answers, and pytest exits 0 on a skip, so a service that never became ready, a wrong port, or a marker typo would otherwise produce a green job that ran nothing at all — the same failure the `no-extras` job refuses, for the same reason. The count comes from pytest's own JUnit XML rather than a regex over output written for humans. **The job itself is unverified: it cannot be run locally and will only be proven by a run on a PR.** What is verified is everything reachable without GitHub. ci.yml parses; the guard script is extracted and executed against real pytest JUnit reports, where it accepts a full run, rejects a run containing a skip, rejects a run that collected nothing, and explains a missing report instead of raising; the heredoc terminator is asserted to sit at column 0, since an indented one would make the shell read to EOF. The wiring most likely to break silently — port, database name, image, health check, both server settings, the restart, `-m integration`, `--strict-markers`, and the guard's `if: always()` — is pinned against the fixture's own constants, so CI and the compose file cannot drift apart. Six separate one-line mutations of ci.yml were each measured to kill exactly one test, and the right one. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 140 +++++++++++++++++ CHANGELOG.md | 14 ++ tests/test_advise_cli.py | 13 +- tests/test_ci_integration_job.py | 257 +++++++++++++++++++++++++++++++ 4 files changed, 420 insertions(+), 4 deletions(-) create mode 100644 tests/test_ci_integration_job.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 599d5fe..feaf3ec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,146 @@ jobs: exit 1 fi + # The 23 tests under tests/integration/ are deselected by default and, until this job + # existed, ran only on a contributor's own machine. That is not a theoretical gap: this + # feature's live suite repeatedly found bugs no fixture could reach — a `reltuples = -1` + # sentinel that read as "tiny table" and silently suppressed every proposal, redaction + # dismembering `$N` placeholders, a `toplevel` filter that made a hot function-wrapped query + # vanish while leaving confidently-wrong advice in its place, and a workload statement that + # failed on the wire for every default run. Each was invisible to `pytest` and to review. + integration: + runs-on: ubuntu-latest + services: + postgres: + image: postgres:16 + env: + POSTGRES_PASSWORD: sqlquality + POSTGRES_DB: sqlquality_test + # Same host port as tests/integration/docker-compose.yml, so one DSN describes both. + # `tests/test_ci_integration_job.py` pins these against the fixture's own constants: + # a port or database name that drifts here turns every live test into a skip, which + # this job's final step is specifically built to refuse. + ports: + - 27432:5432 + # A service container cannot be given a command — GitHub's `services:` schema has no + # `command`/`entrypoint` key, and `options` is passed to `docker create` *before* the + # image, so the `-c shared_preload_libraries=...` flags the compose file uses cannot be + # expressed here. The preload is applied in the first step instead. What `options` is + # for is the health check: the job does not start its steps until Postgres answers. + options: >- + --health-cmd "pg_isready -U postgres -d sqlquality_test" + --health-interval 2s + --health-timeout 3s + --health-retries 30 + steps: + - uses: actions/checkout@v4 + - uses: astral-sh/setup-uv@v5 + with: + python-version: "3.12" + # `pg_stat_statements` must be loaded by the postmaster at startup; `CREATE EXTENSION` + # alone leaves the extension installed and every read of its view failing. Since the + # service container's command cannot carry the flags, they are written with ALTER SYSTEM + # (which persists them to postgresql.auto.conf) and the container is restarted so the + # postmaster picks them up. `pg_reload_conf()` is not enough: both settings are + # postmaster-level, so a reload leaves them inactive with no error. + # + # `docker exec` rather than a host psql: the client is guaranteed to exist inside the + # postgres image, and no assumption is made about what the runner image ships. + - name: Preload pg_stat_statements and restart the service container + run: | + set -euo pipefail + container="$(docker ps --filter ancestor=postgres:16 --format '{{.ID}}' | head -n1)" + if [ -z "$container" ]; then + echo "::error::no postgres:16 service container found — the service failed to start" + docker ps -a + exit 1 + fi + # One `psql -c` per statement, deliberately: psql wraps multiple `-c` options in a + # single implicit transaction, and ALTER SYSTEM cannot run inside a transaction block. + docker exec "$container" psql -U postgres -d sqlquality_test -v ON_ERROR_STOP=1 \ + -c "ALTER SYSTEM SET shared_preload_libraries = 'pg_stat_statements'" + # `pg_stat_statements.track = all` matters as much as the preload: the live suite + # covers nested statements (a DECLARE ... CURSOR unwrapped to its inner query), which + # only appear in pg_stat_statements under `track = all`. + docker exec "$container" psql -U postgres -d sqlquality_test -v ON_ERROR_STOP=1 \ + -c "ALTER SYSTEM SET pg_stat_statements.track = 'all'" + docker restart "$container" + # The health check gates the *start of the job*, not a mid-job restart, so the wait + # is explicit. Failing here rather than letting the suite skip is the whole point: + # a server that never came back must not read as a pass. + for _ in $(seq 1 30); do + if docker exec "$container" pg_isready -U postgres -d sqlquality_test; then + break + fi + sleep 2 + done + docker exec "$container" pg_isready -U postgres -d sqlquality_test + # Prove both settings actually took effect before any test runs, so a failure here + # names the cause instead of surfacing as an error deep inside a fixture. + docker exec "$container" psql -U postgres -d sqlquality_test -tAc \ + "SELECT current_setting('shared_preload_libraries')" | grep -q pg_stat_statements + test "$(docker exec "$container" psql -U postgres -d sqlquality_test -tAc \ + "SELECT current_setting('pg_stat_statements.track')")" = "all" + # The `postgres` extra, since psycopg is what the live tests connect with. Not + # `--all-extras`: nothing here needs the llm extra. + - run: uv sync --extra postgres + - name: Run the integration suite + env: + # The fixture's own default DSN is this exact string, but it is set explicitly so a + # change to either side is a visible change here rather than a silent switch to + # whatever the fixture happens to default to. + SQLQUALITY_TEST_DSN: postgresql://postgres:sqlquality@127.0.0.1:27432/sqlquality_test + # `--strict-markers` catches a typo'd `integration` marker, which would otherwise + # select nothing. The junit XML is what the next step reads: pytest's own + # machine-readable count, not a regex over text meant for humans. + run: uv run pytest -m integration -q -rs --strict-markers --junitxml=integration.xml + # `pytest` exits 0 on skips, and every one of these tests skips itself when the server is + # unreachable — so a service that never became ready, a wrong port, or a marker typo would + # otherwise produce a green run that executed nothing at all. Same discipline as the + # `no-extras` job above, which refuses a skip for the same reason: an invariant that is + # only ever asserted by a test that did not run is not asserted. + # + # `if: always()` so a failing suite still reports *why* — a run that skipped everything + # and a run that genuinely failed need different fixes. + - name: Fail if the integration suite skipped or ran nothing + if: always() + run: | + uv run python - <<'PY' + import pathlib + import sys + import xml.etree.ElementTree as ET + + report = pathlib.Path("integration.xml") + if not report.exists(): + # Reachable because this step is `if: always()`: the pytest step can die before + # writing a report. Say so plainly rather than raising a traceback over it. + sys.exit( + "::error::integration.xml was never written, so the previous step did not " + "get as far as running tests. Its own log has the reason." + ) + root = ET.parse(report).getroot() + suites = root.findall("testsuite") or ([root] if root.tag == "testsuite" else []) + if not suites: + sys.exit("::error::integration.xml has no testsuite element; pytest wrote nothing") + total = sum(int(s.get("tests", 0)) for s in suites) + skipped = sum(int(s.get("skipped", 0)) for s in suites) + failures = sum(int(s.get("failures", 0)) + int(s.get("errors", 0)) for s in suites) + executed = total - skipped + print(f"collected={total} executed={executed} skipped={skipped} failed={failures}") + if skipped: + sys.exit( + f"::error::{skipped} integration test(s) skipped. These tests skip themselves " + "when no Postgres answers, so a skip here means the service container never " + "became ready, the published port does not match SQLQUALITY_TEST_DSN, or the " + "server is not the one this suite expects. A skip must not read as a pass." + ) + if executed == 0: + sys.exit( + "::error::zero integration tests ran. Either the `integration` marker no " + "longer selects them or the package collected nothing." + ) + PY + # `uv sync` installs what uv.lock pins, so every job above tests exactly one point in the # dependency ranges pyproject declares. A user running `pip install sqlquality` gets the # *newest* release satisfying those ranges instead — and that difference has already shipped diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e5d62d..dc35a79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- CI now runs the 23 live-Postgres integration tests, in a job with a `postgres:16` service + container that has `pg_stat_statements` preloaded. Until now they ran only on a contributor's + own machine, while this feature's live suite was repeatedly the only thing that caught a + whole class of bug — a `reltuples = -1` sentinel that suppressed every proposal, redaction + dismembering `$N` placeholders, a `toplevel` filter that produced confidently-wrong advice, + and a workload statement that failed on the wire for every default run. The job fails if the + suite skipped or executed nothing, since every one of those tests skips itself when no server + answers and `pytest` exits 0 on a skip. +- The integration compose file publishes host port 27432 instead of 55432, which collided with + an unrelated container in practice — and because `docker compose up` neither binds nor fails + in that case, the suite silently talked to whatever else was listening. The fixture now also + verifies which server answered (database name, and `pg_stat_statements` in + `shared_preload_libraries`) and fails naming a port collision as the likely cause, rather + than trusting that a successful connection reached the right database. - `--ddl`'s guarantee that every line of the generated script is either an intended statement or a `--` comment now holds for all ten codepoints `str.splitlines()` treats as a line boundary, on both the Postgres and Redshift renderers. The guard tested only `\n` and `\r`, diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index 5a1bc42..15f3776 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -1052,10 +1052,15 @@ def test_adv302_rewrites_an_index_proposal_into_dbt_config_through_the_cli(monke replacing that one line with `pass` — which disables the branch's headline feature completely — left all 665 tests passing. The only guard was `tests/integration/test_advise_live.py`, and `pyproject.toml` sets - `addopts = "-m 'not integration'"` while `ci.yml` provisions no Postgres, so CI never runs - it: ADV302 could have been deleted from the CLI with every check green. This is the third - instance of that defect class on this branch, so it is pinned here — no Docker, no extras, - no live database, because the `no-extras` CI job depends on that. + `addopts = "-m 'not integration'"` while `ci.yml` provisioned no Postgres at the time, so CI + never ran it: ADV302 could have been deleted from the CLI with every check green. This is the + third instance of that defect class on this branch, so it is pinned here — no Docker, no + extras, no live database, because the `no-extras` CI job depends on that. + + CI now has an `integration` job that does provision Postgres, which closes the other half of + that gap. It does not make this test redundant: the live suite needs Docker and the postgres + extra, so it is still the wrong place to pin CLI wiring that must hold for every contributor + running a bare `uv run pytest`. """ _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) result = runner.invoke( diff --git a/tests/test_ci_integration_job.py b/tests/test_ci_integration_job.py new file mode 100644 index 0000000..73b869b --- /dev/null +++ b/tests/test_ci_integration_job.py @@ -0,0 +1,257 @@ +"""The CI job that runs the live suite, pinned structurally. + +This cannot prove the job works — only a run on a PR can, and until then it is unverified. What +it *can* do is pin every piece of wiring whose silent breakage would turn the live suite back +into something that never runs, which is the state this job was added to end: + +* the job existing at all; +* the service publishing the port and creating the database the fixture actually looks for + (a mismatch makes every test skip, and a skip exits 0); +* the two `pg_stat_statements` server settings, without which the suite connects and then + fails on every workload read; +* the health check, and the step that refuses a run which skipped or executed nothing. + +Each of those is a one-line edit away from being silently wrong, and none of them is visible in +a green run. `tests/integration/conftest.py`'s own constants are the source of truth here, so +the port and database name cannot drift between the compose file, the fixture and CI. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest +import yaml + +_TESTS_DIR = Path(__file__).parent +if str(_TESTS_DIR) not in sys.path: # pragma: no cover - pytest normally does this itself + sys.path.insert(0, str(_TESTS_DIR)) + +from integration.conftest import DEFAULT_PORT, EXPECTED_DATABASE # noqa: E402 + +_CI = _TESTS_DIR.parent / ".github" / "workflows" / "ci.yml" +_JOB = "integration" + + +@pytest.fixture(scope="module") +def workflow() -> dict: + """Also the test that ci.yml is valid YAML at all — a workflow that does not parse is + silently not run by GitHub, with no failing check to notice.""" + return yaml.safe_load(_CI.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def job(workflow: dict) -> dict: + assert _JOB in workflow["jobs"], ( + f"ci.yml has no {_JOB!r} job, so the 23 live tests run on nobody's machine but the " + f"author's. Jobs present: {sorted(workflow['jobs'])}" + ) + return workflow["jobs"][_JOB] + + +@pytest.fixture(scope="module") +def service(job: dict) -> dict: + services = job.get("services", {}) + assert list(services) == ["postgres"], services + return services["postgres"] + + +def _steps_text(job: dict) -> str: + return "\n".join(str(step.get("run", "")) for step in job["steps"]) + + +def test_the_service_is_postgres_16(service: dict): + """The live suite reads `pg_stat_statements` columns and `reltuples` semantics that are + version-dependent; the compose file pins 16 and CI must not silently test another major.""" + assert service["image"] == "postgres:16" + + +def test_the_service_publishes_the_port_the_fixture_connects_to(service: dict): + """A port that drifts from the fixture's makes every test skip — and pytest exits 0 on a + skip, so the job would pass having run nothing. The final step catches that; this catches + it earlier and says which side is wrong.""" + assert [str(port) for port in service["ports"]] == [f"{DEFAULT_PORT}:5432"] + + +def test_the_service_creates_the_database_the_fixture_verifies(service: dict): + """`live_dsn` now fails when `current_database()` is not this name, so a change here would + turn the whole job red with a port-collision message that is not the real cause.""" + assert service["env"]["POSTGRES_DB"] == EXPECTED_DATABASE + assert service["env"]["POSTGRES_PASSWORD"], "the fixture's DSN authenticates with a password" + + +def test_the_service_has_a_health_check_so_steps_wait_for_the_server(service: dict): + """Without it the first step races the server's startup and the whole job fails + intermittently, which reads as flakiness rather than as a missing wait.""" + options = service["options"] + assert "--health-cmd" in options + assert "pg_isready" in options + assert "--health-retries" in options + + +def test_ci_sets_both_pg_stat_statements_server_settings(job: dict): + """Neither can be reached by `CREATE EXTENSION`: `shared_preload_libraries` is loaded at + postmaster start, and `track = all` is what makes nested statements (the DECLARE ... CURSOR + case the live suite unwraps) appear at all. A service container takes no command, so these + are applied by a step — and a step is exactly the kind of thing that gets dropped in an + edit.""" + steps = _steps_text(job) + assert "shared_preload_libraries = 'pg_stat_statements'" in steps + assert "pg_stat_statements.track = 'all'" in steps + assert "docker restart" in steps, ( + "both settings are postmaster-level, so pg_reload_conf() leaves them inactive with no " + "error — the container has to be restarted" + ) + + +def test_ci_verifies_the_preload_took_effect_before_running_any_test(job: dict): + """Otherwise a failed ALTER SYSTEM surfaces as an error deep inside a seeding fixture, + which says nothing about the real cause.""" + steps = _steps_text(job) + assert "current_setting('shared_preload_libraries')" in steps + assert "current_setting('pg_stat_statements.track')" in steps + + +def test_ci_points_the_fixture_at_the_service_it_started(job: dict): + """The fixture reads `SQLQUALITY_TEST_DSN`. Set explicitly rather than relying on the + fixture's default, so this file and the fixture cannot disagree silently.""" + dsn = next( + step["env"]["SQLQUALITY_TEST_DSN"] + for step in job["steps"] + if "SQLQUALITY_TEST_DSN" in step.get("env", {}) + ) + assert f":{DEFAULT_PORT}/" in dsn + assert dsn.endswith(f"/{EXPECTED_DATABASE}") + + +def test_ci_actually_selects_the_integration_marker(job: dict): + """`pytest` with no `-m` runs the *default* suite, which deselects every one of these + tests: the job would be green, fast, and a complete no-op.""" + steps = _steps_text(job) + assert "-m integration" in steps + assert "--strict-markers" in steps, "a typo'd marker would otherwise select nothing" + + +def test_ci_refuses_a_run_that_skipped_or_executed_nothing(job: dict): + """The reason this job needs a guard at all: every test in the package skips itself when no + Postgres answers, and pytest exits 0 on a skip. A service that never became ready, a wrong + port, or a marker typo would all produce a passing job that ran nothing — the same failure + the `no-extras` job refuses for the same reason. + """ + steps = _steps_text(job) + assert "--junitxml" in steps, ( + "the count has to come from pytest's own machine-readable report, not a regex over " + "output written for humans" + ) + assert "skipped" in steps + assert "executed == 0" in steps + guard = next(step for step in job["steps"] if "executed == 0" in str(step.get("run", ""))) + assert guard.get("if") == "always()", ( + "a failing suite must still report whether it skipped everything or genuinely failed" + ) + + +def _guard_script(job: dict) -> str: + """The guard's Python body, exactly as the shell pipes it to the interpreter. + + Deliberately no `textwrap.dedent`: YAML has already stripped the block scalar's common + indentation, so what CI feeds Python is this string verbatim. Dedenting here would hide a + real indentation error in the file. + """ + run = next(step["run"] for step in job["steps"] if "executed == 0" in str(step.get("run", ""))) + lines = run.splitlines() + assert "PY" in lines, "the heredoc terminator must be a line of its own" + assert lines[lines.index("PY")] == "PY", ( + "the terminator is indented, so `<<'PY'` never closes and the shell reads to EOF" + ) + return run.split("<<'PY'\n", 1)[1].rsplit("\nPY", 1)[0] + + +def test_the_guard_script_is_valid_python(job: dict): + """It is a heredoc inside YAML inside a shell script, three layers away from anything that + would catch a syntax error before CI runs it — and it only executes at the very end of the + job, after everything expensive has already run.""" + import ast + + tree = ast.parse(_guard_script(job)) + assert len(tree.body) > 5, "a near-empty parse would make this test vacuous" + + +def _junit_for(source: str, tmp_path: Path, *, select: str = "") -> Path: + """A **real** pytest JUnit report, not a hand-written one. + + The guard reads attributes off pytest's XML, so a fixture built from what those attributes + are believed to be would prove only self-consistency. Generating the report with pytest + itself is what makes the parse test meaningful. + """ + import subprocess + + (tmp_path / "test_sample.py").write_text(source, encoding="utf-8") + xml = tmp_path / "integration.xml" + subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", f"--junitxml={xml}"] + + (["-m", select] if select else []), + cwd=tmp_path, + capture_output=True, + check=False, + ) + assert xml.exists(), "pytest wrote no report; the rest of this test would prove nothing" + return xml + + +def _run_guard(job: dict, xml: Path) -> tuple[int, str]: + import subprocess + + result = subprocess.run( + [sys.executable, "-c", _guard_script(job)], + cwd=xml.parent, + capture_output=True, + text=True, + ) + return result.returncode, result.stdout + result.stderr + + +def test_the_guard_accepts_a_run_where_every_test_executed(job: dict, tmp_path): + """The control. A guard that fails on everything would block every honest run, and the + obvious way to write one — treating a nonzero `skipped` attribute as the only signal — + reads `tests` and `skipped` off the wrong element and fails universally.""" + xml = _junit_for("def test_a(): pass\ndef test_b(): pass\n", tmp_path) + code, output = _run_guard(job, xml) + assert code == 0, output + assert "executed=2" in output, output + + +def test_the_guard_fails_a_run_that_skipped(job: dict, tmp_path): + """The failure mode this job exists to refuse. Every test in tests/integration/ skips itself + when no Postgres answers, and pytest exits 0 on a skip — so a service container that never + became ready would otherwise be indistinguishable from a passing run. + """ + xml = _junit_for( + "import pytest\ndef test_a(): pass\ndef test_b(): pytest.skip('no server')\n", tmp_path + ) + code, output = _run_guard(job, xml) + assert code != 0, output + assert "skipped" in output + assert "must not read as a pass" in output + + +def test_the_guard_explains_a_missing_report_instead_of_raising(job: dict, tmp_path): + """Reachable because the step is `if: always()`: the pytest step can die before writing a + report, and a traceback over `ET.parse` buries the real log.""" + code, output = _run_guard(job, tmp_path / "integration.xml") + assert code != 0 + assert "never written" in output + assert "Traceback" not in output + + +def test_the_guard_fails_a_run_that_collected_nothing(job: dict, tmp_path): + """A marker typo, a renamed package, or a deselect-everything change: pytest reports zero + tests, and without this the job's only other signal is an exit code the previous step + already consumed.""" + xml = _junit_for( + "import pytest\n@pytest.mark.other\ndef test_a(): pass\n", tmp_path, select="nothingmatches" + ) + code, output = _run_guard(job, xml) + assert code != 0, output + assert "zero integration tests ran" in output From 2bcfaa0f83593ab8da65f065afc9e09499319332 Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 23:10:34 +0200 Subject: [PATCH 6/7] docs(advise): record two known dbt-enrichment gaps beside the code they concern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither is fixed, deliberately, and both are the kind of thing that gets lost in a branch summary rather than found again in the source. `_prepend_note` covers 2 of `_classify`'s 5 `note=` sites. The other three build their note from `_dbt_ddl_note` directly and would discard an existing note — but none is reachable today: all three are `CREATE INDEX` paths, and the only rule that sets a note before enrichment (Redshift's ADV105) emits no index DDL and so lands in the generic branch, which already routes through `_prepend_note`. Fixing them now would add three paths no test can reach. The generic non-index branch is exercised only from the Redshift side. It keys on the DDL prefix rather than the adapter, so it is engine-agnostic by construction, but a future Postgres rule emitting non-index DDL would land there with no test of its own. Writing one now would mean inventing a proposal shape no rule produces. Co-Authored-By: Claude Opus 5 --- src/sqlquality/workload/dbt.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/sqlquality/workload/dbt.py b/src/sqlquality/workload/dbt.py index 711baa3..b941820 100644 --- a/src/sqlquality/workload/dbt.py +++ b/src/sqlquality/workload/dbt.py @@ -536,6 +536,17 @@ def _prepend_note(existing: str | None, dbt_note: str) -> str: warning is a caveat *on* it, and `dbt_note` is emitted exactly once — a note already carrying this warning is returned unchanged rather than accumulating a second copy, so a second enrichment pass over an already-enriched proposal cannot double it. + + **Known and deliberately not fixed: only 2 of `_classify`'s 5 `note=` sites go through + this.** The `DROP INDEX` branch and the generic non-index branch do; the three remaining + ones — unrecognised materialization, and the two "cannot be expressed as config" bail-outs + — build their `note` from `_dbt_ddl_note(...)` directly and would discard an existing note + if one ever reached them. Today none can: every proposal that takes those three paths is a + `CREATE INDEX` from ADV001/004/007/008, and no Postgres rule sets `note` before enrichment + runs. The reachable case is a rule that both sets its own `note` *and* emits index-creating + DDL — Redshift's ADV105 sets a note, but Redshift emits no `CREATE INDEX`, so it lands in + the generic branch, which is already covered. Route the other three through this function + when such a rule appears; changing them now would add three untestable paths. """ if not existing: return dbt_note @@ -717,7 +728,14 @@ def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _I # this stays covered when one does. It is no longer hypothetical: Redshift's # ADV101-103 (ALTER SORTKEY/DISTKEY/DISTSTYLE, table rewrites), ADV104 # (VACUUM/ANALYZE) and ADV105 (Advisor's own DDL, relayed verbatim) all land here, - # since none of them is a CREATE/DROP INDEX. See + # since none of them is a CREATE/DROP INDEX. + # + # Known gap, deliberately not closed: this branch is exercised only from the Redshift + # side. It is engine-agnostic by construction — it keys on the DDL prefix, not on the + # adapter — but every test that reaches it today arrives with a Redshift-shaped + # proposal, so a Postgres rule emitting non-index DDL (an ALTER TABLE, a CLUSTER, a + # REINDEX) would land here with no test of its own. Writing one now would mean + # inventing a proposal shape no rule produces; add it with that rule. See # `test_dbt_warning_is_attached_to_a_redshift_rewrite_proposal` in # `tests/test_workload_redshift_rules.py` — this generic path was previously # unexercised by any test. From a3a8fe617d552e79c2b94a22b410866c1e85f58c Mon Sep 17 00:00:00 2001 From: Hans Lemm Date: Fri, 31 Jul 2026 23:19:42 +0200 Subject: [PATCH 7/7] ci: start Postgres with docker run instead of a service container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration job failed on its first real run with `ERROR: unrecognized configuration parameter "pg_stat_statements.track"`. The cause is an ordering problem that cannot be ordered away: a GitHub service container takes no command (the schema has no `command` key, and `options` is passed to `docker create` before the image), so the two `-c` flags the compose file uses could not be expressed there. The workaround applied both settings with `ALTER SYSTEM` and one restart — but `pg_stat_statements.track` does not exist as a parameter until the library is loaded, which is what the restart was meant to accomplish. Setting them correctly would need two restarts. `docker run` takes the flags directly, and takes the *same* flags as tests/integration/docker-compose.yml, so the server CI tests against is now described once rather than twice. The cost is that nothing gates the job's steps on readiness, so the wait is explicit and the final `pg_isready` outside the retry loop is what turns a server that never came up into a failure rather than a skip. Rehearsed locally with the exact commands: the container starts, both settings read back as expected, and all 23 integration tests pass against port 27432. The structural pins move with it — `test_ci_integration_job.py` now reads the server step's shell text rather than a `services:` mapping, keeping every guarantee it had (image, port, database, readiness wait, both flags) and gaining one: that the flags appear as command flags, since that is the only form that works here. Worth recording that the guard step did its job on its first outing: it refused to let a run that never reached pytest look like a pass, and said so in one line. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 89 ++++++++++---------------- tests/test_ci_integration_job.py | 103 +++++++++++++++++++------------ 2 files changed, 96 insertions(+), 96 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index feaf3ec..f36173e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,76 +68,51 @@ jobs: # failed on the wire for every default run. Each was invisible to `pytest` and to review. integration: runs-on: ubuntu-latest - services: - postgres: - image: postgres:16 - env: - POSTGRES_PASSWORD: sqlquality - POSTGRES_DB: sqlquality_test - # Same host port as tests/integration/docker-compose.yml, so one DSN describes both. - # `tests/test_ci_integration_job.py` pins these against the fixture's own constants: - # a port or database name that drifts here turns every live test into a skip, which - # this job's final step is specifically built to refuse. - ports: - - 27432:5432 - # A service container cannot be given a command — GitHub's `services:` schema has no - # `command`/`entrypoint` key, and `options` is passed to `docker create` *before* the - # image, so the `-c shared_preload_libraries=...` flags the compose file uses cannot be - # expressed here. The preload is applied in the first step instead. What `options` is - # for is the health check: the job does not start its steps until Postgres answers. - options: >- - --health-cmd "pg_isready -U postgres -d sqlquality_test" - --health-interval 2s - --health-timeout 3s - --health-retries 30 steps: - uses: actions/checkout@v4 - uses: astral-sh/setup-uv@v5 with: python-version: "3.12" - # `pg_stat_statements` must be loaded by the postmaster at startup; `CREATE EXTENSION` - # alone leaves the extension installed and every read of its view failing. Since the - # service container's command cannot carry the flags, they are written with ALTER SYSTEM - # (which persists them to postgresql.auto.conf) and the container is restarted so the - # postmaster picks them up. `pg_reload_conf()` is not enough: both settings are - # postmaster-level, so a reload leaves them inactive with no error. + # Started with `docker run` rather than a `services:` container, and that is the whole + # reason this step exists in this shape. `pg_stat_statements` must be loaded by the + # postmaster at startup, but a service container cannot be given a command — GitHub's + # `services:` schema has no `command` key, and `options` is passed to `docker create` + # before the image, so the `-c shared_preload_libraries=...` flags cannot be expressed + # there at all. # - # `docker exec` rather than a host psql: the client is guaranteed to exist inside the - # postgres image, and no assumption is made about what the runner image ships. - - name: Preload pg_stat_statements and restart the service container + # The first attempt worked around that with `ALTER SYSTEM` plus a restart, and it failed + # on the real runner: `ALTER SYSTEM SET pg_stat_statements.track` is rejected with + # "unrecognized configuration parameter" because that setting does not exist until the + # library is loaded, which is precisely what the restart was meant to achieve. Ordering + # the two calls correctly would need two restarts. Running the container directly is + # simpler, and — more importantly — takes the same flags as + # tests/integration/docker-compose.yml, so the server this job tests against is + # configured from one description rather than two that can drift. + - name: Start Postgres with pg_stat_statements preloaded run: | set -euo pipefail - container="$(docker ps --filter ancestor=postgres:16 --format '{{.ID}}' | head -n1)" - if [ -z "$container" ]; then - echo "::error::no postgres:16 service container found — the service failed to start" - docker ps -a - exit 1 - fi - # One `psql -c` per statement, deliberately: psql wraps multiple `-c` options in a - # single implicit transaction, and ALTER SYSTEM cannot run inside a transaction block. - docker exec "$container" psql -U postgres -d sqlquality_test -v ON_ERROR_STOP=1 \ - -c "ALTER SYSTEM SET shared_preload_libraries = 'pg_stat_statements'" - # `pg_stat_statements.track = all` matters as much as the preload: the live suite - # covers nested statements (a DECLARE ... CURSOR unwrapped to its inner query), which - # only appear in pg_stat_statements under `track = all`. - docker exec "$container" psql -U postgres -d sqlquality_test -v ON_ERROR_STOP=1 \ - -c "ALTER SYSTEM SET pg_stat_statements.track = 'all'" - docker restart "$container" - # The health check gates the *start of the job*, not a mid-job restart, so the wait - # is explicit. Failing here rather than letting the suite skip is the whole point: - # a server that never came back must not read as a pass. - for _ in $(seq 1 30); do - if docker exec "$container" pg_isready -U postgres -d sqlquality_test; then + docker run -d --name sqlquality-ci-pg \ + -e POSTGRES_PASSWORD=sqlquality \ + -e POSTGRES_DB=sqlquality_test \ + -p 27432:5432 \ + postgres:16 \ + postgres -c shared_preload_libraries=pg_stat_statements \ + -c pg_stat_statements.track=all + # No health check gates a plain `docker run`, so wait explicitly. Failing here rather + # than letting the suite skip is the point: a server that never came up must not read + # as a pass, which is what the final step enforces. + for _ in $(seq 1 45); do + if docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test; then break fi sleep 2 done - docker exec "$container" pg_isready -U postgres -d sqlquality_test - # Prove both settings actually took effect before any test runs, so a failure here - # names the cause instead of surfacing as an error deep inside a fixture. - docker exec "$container" psql -U postgres -d sqlquality_test -tAc \ + docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test + # Prove both settings took effect before any test runs, so a misconfiguration names + # itself here instead of surfacing as an error deep inside a fixture. + docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \ "SELECT current_setting('shared_preload_libraries')" | grep -q pg_stat_statements - test "$(docker exec "$container" psql -U postgres -d sqlquality_test -tAc \ + test "$(docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \ "SELECT current_setting('pg_stat_statements.track')")" = "all" # The `postgres` extra, since psycopg is what the live tests connect with. Not # `--all-extras`: nothing here needs the llm extra. diff --git a/tests/test_ci_integration_job.py b/tests/test_ci_integration_job.py index 73b869b..25ace03 100644 --- a/tests/test_ci_integration_job.py +++ b/tests/test_ci_integration_job.py @@ -5,11 +5,12 @@ into something that never runs, which is the state this job was added to end: * the job existing at all; -* the service publishing the port and creating the database the fixture actually looks for - (a mismatch makes every test skip, and a skip exits 0); -* the two `pg_stat_statements` server settings, without which the suite connects and then - fails on every workload read; -* the health check, and the step that refuses a run which skipped or executed nothing. +* the step that starts the server publishing the port and creating the database the fixture + actually looks for (a mismatch makes every test skip, and a skip exits 0); +* the two `pg_stat_statements` server flags, without which the suite connects and then fails on + every workload read; +* the explicit readiness wait — `docker run` has no health check to gate the job's steps — and + the final step that refuses a run which skipped or executed nothing. Each of those is a one-line edit away from being silently wrong, and none of them is visible in a green run. `tests/integration/conftest.py`'s own constants are the source of truth here, so @@ -50,59 +51,83 @@ def job(workflow: dict) -> dict: return workflow["jobs"][_JOB] -@pytest.fixture(scope="module") -def service(job: dict) -> dict: - services = job.get("services", {}) - assert list(services) == ["postgres"], services - return services["postgres"] - - def _steps_text(job: dict) -> str: return "\n".join(str(step.get("run", "")) for step in job["steps"]) -def test_the_service_is_postgres_16(service: dict): +@pytest.fixture(scope="module") +def server_step(job: dict) -> str: + """The step that starts Postgres, as shell text. + + Deliberately a step and not a `services:` container, and the history is worth keeping + because the obvious approach fails on a real runner. A service container cannot be given + a command — GitHub's schema has no `command` key, and `options` is passed to + `docker create` *before* the image — so the `-c shared_preload_libraries=…` flags the + compose file uses cannot be expressed there. The first version worked around that with + `ALTER SYSTEM` plus a restart and failed in CI: `ALTER SYSTEM SET + pg_stat_statements.track` is rejected outright with "unrecognized configuration + parameter", because that setting does not exist until the library is loaded — which is + what the restart was supposed to accomplish. Getting the order right would need two + restarts. + + `docker run` takes the same flags as `tests/integration/docker-compose.yml`, so the + server CI tests against is described once rather than twice. + """ + steps = [ + str(step.get("run", "")) + for step in job["steps"] + if "docker run" in str(step.get("run", "")) + ] + assert len(steps) == 1, ( + "expected exactly one step starting the server; found " + f"{len(steps)}. Without it the live suite skips itself and the job exits 0." + ) + return steps[0] + + +def test_the_server_is_postgres_16(server_step: str): """The live suite reads `pg_stat_statements` columns and `reltuples` semantics that are version-dependent; the compose file pins 16 and CI must not silently test another major.""" - assert service["image"] == "postgres:16" + assert "postgres:16" in server_step -def test_the_service_publishes_the_port_the_fixture_connects_to(service: dict): +def test_the_server_publishes_the_port_the_fixture_connects_to(server_step: str): """A port that drifts from the fixture's makes every test skip — and pytest exits 0 on a skip, so the job would pass having run nothing. The final step catches that; this catches it earlier and says which side is wrong.""" - assert [str(port) for port in service["ports"]] == [f"{DEFAULT_PORT}:5432"] + assert f"-p {DEFAULT_PORT}:5432" in server_step -def test_the_service_creates_the_database_the_fixture_verifies(service: dict): - """`live_dsn` now fails when `current_database()` is not this name, so a change here would - turn the whole job red with a port-collision message that is not the real cause.""" - assert service["env"]["POSTGRES_DB"] == EXPECTED_DATABASE - assert service["env"]["POSTGRES_PASSWORD"], "the fixture's DSN authenticates with a password" +def test_the_server_creates_the_database_the_fixture_verifies(server_step: str): + """`live_dsn` fails when `current_database()` is not this name, so a change here would turn + the whole job red with a port-collision message that is not the real cause.""" + assert f"POSTGRES_DB={EXPECTED_DATABASE}" in server_step + assert "POSTGRES_PASSWORD=" in server_step, "the fixture's DSN authenticates with a password" -def test_the_service_has_a_health_check_so_steps_wait_for_the_server(service: dict): - """Without it the first step races the server's startup and the whole job fails - intermittently, which reads as flakiness rather than as a missing wait.""" - options = service["options"] - assert "--health-cmd" in options - assert "pg_isready" in options - assert "--health-retries" in options +def test_ci_waits_for_the_server_before_running_anything(server_step: str): + """`docker run` has no health check to gate the job's steps, so the wait is explicit. + Without it the suite races startup, every test skips itself, and the job exits 0 — the + exact silent no-op this job was added to end. The final `pg_isready` outside the retry + loop is what turns a server that never came up into a failure rather than a skip.""" + assert "pg_isready" in server_step + assert "seq 1" in server_step, "a bounded retry loop, not a single optimistic check" -def test_ci_sets_both_pg_stat_statements_server_settings(job: dict): +def test_ci_passes_both_pg_stat_statements_settings_as_server_flags(server_step: str): """Neither can be reached by `CREATE EXTENSION`: `shared_preload_libraries` is loaded at postmaster start, and `track = all` is what makes nested statements (the DECLARE ... CURSOR - case the live suite unwraps) appear at all. A service container takes no command, so these - are applied by a step — and a step is exactly the kind of thing that gets dropped in an - edit.""" - steps = _steps_text(job) - assert "shared_preload_libraries = 'pg_stat_statements'" in steps - assert "pg_stat_statements.track = 'all'" in steps - assert "docker restart" in steps, ( - "both settings are postmaster-level, so pg_reload_conf() leaves them inactive with no " - "error — the container has to be restarted" - ) + case the live suite unwraps) appear at all. + + Asserted as **command flags**, which is the only form that works. Applying them with + `ALTER SYSTEM` and a restart was tried and failed in CI: `ALTER SYSTEM SET + pg_stat_statements.track` is rejected with "unrecognized configuration parameter", because + the setting does not exist until the library is loaded. These same two flags appear in + `tests/integration/docker-compose.yml`; a drift between the two would mean local and CI + runs test differently configured servers. + """ + assert "-c shared_preload_libraries=pg_stat_statements" in server_step + assert "-c pg_stat_statements.track=all" in server_step def test_ci_verifies_the_preload_took_effect_before_running_any_test(job: dict):