From 1b15e22710027c4a470d27d984dd7cd33a16020a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 18 Jul 2026 08:37:49 -0400 Subject: [PATCH] sp_IndexCleanup rule_coverage: create the Crap scratch database rule_coverage_test.py assumed a pre-existing Crap database. On a long-lived instance it persists between runs (nothing drops it), so the gap was invisible; on a freshly-started instance -- every CI container -- Crap has never existed. run_sql_script connects with -d Crap, so setup failed with Msg 4060 ("Cannot open database"), which is Level 11 and slipped past the Level-16 error check: the fixture silently did not build and every Crap-scoped assertion failed with "found 0". This masqueraded as a procedure bug (missing PAGE compression recommendations); it was not -- the procedure emits them correctly on a fresh instance, confirmed directly. Create Crap if absent (CrapA/CrapB were already created this way) and treat Msg 4060/911 as fatal in sql_errors so a missing scratch database fails loudly instead of leaving the suite green. rule_coverage now runs on a fresh instance, so wire it back into CI as the fourth IndexCleanup runner. Verified by dropping Crap on a local instance: fails identically to CI without the fix, passes 40/40 with it. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/sql-tests.yml | 17 +++--------- sp_IndexCleanup/tests/README.md | 29 ++++++++++----------- sp_IndexCleanup/tests/rule_coverage_test.py | 22 ++++++++++++++-- 3 files changed, 38 insertions(+), 30 deletions(-) diff --git a/.github/workflows/sql-tests.yml b/.github/workflows/sql-tests.yml index 4111ad7a..1d729198 100644 --- a/.github/workflows/sql-tests.yml +++ b/.github/workflows/sql-tests.yml @@ -176,19 +176,9 @@ jobs: run: | /opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P "$SA_PASSWORD" -C -No -b -i "sp_IndexCleanup/tests/fixture_stackoverflow2013.sql" - # Three runners: adversarial synthetic tables, the Users dedupe cases - # (which also EXECUTE every generated MERGE/DISABLE script), and the - # no-database-access preflight guard. - # - # rule_coverage_test.py is deliberately NOT run here. Several of its - # positive controls assert the procedure still recommends PAGE compression - # for small tables, and on a freshly-started instance (which the CI - # containers always are) those COMPRESSION SCRIPT rows are absent in the - # single-database (@database_name) path -- though present via - # @get_all_databases and present on any established instance. Whether that - # is a real gap in the procedure or a stats-not-yet-settled artifact needs - # a fresh-instance repro to root-cause; until then this runner stays a - # local-only test (see sp_IndexCleanup/tests/README.md). + # Four runners: adversarial synthetic tables, the Users dedupe cases + # (which also EXECUTE every generated MERGE/DISABLE script), rule coverage + # in its own scratch databases, and the no-database-access preflight guard. - name: sp_IndexCleanup assertion harness env: SA_PASSWORD: CI_Test#2026! @@ -197,6 +187,7 @@ jobs: run: | python3 sp_IndexCleanup/tests/run_tests.py --server localhost --password "$SA_PASSWORD" python3 sp_IndexCleanup/tests/fixture_cases_test.py --server localhost --password "$SA_PASSWORD" + python3 sp_IndexCleanup/tests/rule_coverage_test.py --server localhost --password "$SA_PASSWORD" python3 sp_IndexCleanup/tests/no_access_test.py --server localhost --password "$SA_PASSWORD" # ReproBuilder is fully portable: every plan is an embedded string constant diff --git a/sp_IndexCleanup/tests/README.md b/sp_IndexCleanup/tests/README.md index 5e78b401..25aee97d 100644 --- a/sp_IndexCleanup/tests/README.md +++ b/sp_IndexCleanup/tests/README.md @@ -65,21 +65,20 @@ the index was analyzed and genuinely has zero reads first, so the absence of an ### CI -`run_tests.py`, `fixture_cases_test.py`, and `no_access_test.py` run in CI on -every 2017/2019/2022/2025 container. They need a `StackOverflow2013` with a -`Users` table, which the containers do not have, so CI first builds a -faithful-schema synthetic one from `fixture_stackoverflow2013.sql` (exact Users -columns, clustered PK, the `DropIndexes` helper, ~500k varied rows). Real values -are not needed: the harness forces its index reads and every rule is structural. - -`rule_coverage_test.py` is **local only**, not in CI. Several of its positive -controls assert the procedure still recommends PAGE compression for small tables, -and on a freshly-started instance -- which the CI containers always are -- those -`COMPRESSION SCRIPT` rows are absent in the single-database (`@database_name`) -path, though present via `@get_all_databases` (groups F/G) and present on any -established instance. Whether that is a real gap or a stats-not-yet-settled -artifact is unresolved; until it is, run this one by hand on a long-lived -instance. Green CI does **not** mean `rule_coverage_test.py` passed. +All four runners run in CI on every 2017/2019/2022/2025 container. Three of them +(`run_tests.py`, `fixture_cases_test.py`, `no_access_test.py`) need a +`StackOverflow2013` with a `Users` table, which the containers do not have, so CI +first builds a faithful-schema synthetic one from `fixture_stackoverflow2013.sql` +(exact Users columns, clustered PK, the `DropIndexes` helper, ~500k varied rows). +Real values are not needed: the harness forces its index reads and every rule is +structural. + +`rule_coverage_test.py` creates its own scratch `Crap`/`CrapA`/`CrapB` +databases. It used to assume `Crap` already existed -- true on a long-lived box +where it persists between runs, false on a fresh CI container -- and the missing +database failed silently (the `-d Crap` connection error is Msg 4060, Level 11, +below the Level-16 error check). It now creates `Crap` if absent and treats Msg +4060/911 as fatal, so it runs cleanly on a fresh instance. ## Fixtures (manual) diff --git a/sp_IndexCleanup/tests/rule_coverage_test.py b/sp_IndexCleanup/tests/rule_coverage_test.py index 021faf39..fa88674c 100644 --- a/sp_IndexCleanup/tests/rule_coverage_test.py +++ b/sp_IndexCleanup/tests/rule_coverage_test.py @@ -901,8 +901,14 @@ def sql_errors(stdout, stderr): stderr only, which is how adversarial_test.sql managed to fail on every run for months while the suite stayed green. """ - return (re.findall(r"Msg \d+, Level 1[6-9][^\n]*", stdout or "") + - re.findall(r"Msg \d+, Level 1[6-9][^\n]*", stderr or "")) + # Level 16-19 are the usual "this statement failed" errors. Msg 4060 and + # Msg 911 ("Cannot open database") are only Level 11, but they mean the + # script never ran at all -- a missing scratch database, exactly the failure + # that once left this suite silently green on a fresh instance. Catch those + # explicitly so they fail loudly too. + pattern = r"Msg (?:\d+, Level 1[6-9]|4060|911)[^\n]*" + return (re.findall(pattern, stdout or "") + + re.findall(pattern, stderr or "")) def parse_output(stdout): @@ -1541,6 +1547,18 @@ def main(): print("verification, not a skip.") print() + # Create the scratch database if it is not already there. On an established + # instance it persists between runs (nothing drops it), but a fresh instance + # -- every CI container -- has never had it. run_sql_script below connects + # with -d Crap, so without this the setup connection fails with Msg 4060 + # ("Cannot open database"), which is Level 11 and slips past the Level-16 + # error check, silently leaving the fixture unbuilt and every Crap-scoped + # assertion to fail with "found 0". CrapA/CrapB are already created this way. + print("Ensuring the %s database exists..." % TEST_DATABASE) + run_sqlcmd(server, password, database="master", + query="IF DB_ID('%s') IS NULL CREATE DATABASE %s;" + % (TEST_DATABASE, TEST_DATABASE)) + print("Building synthetic fixture in %s..." % TEST_DATABASE) stdout, stderr = run_sql_script(server, password, SETUP_SQL)