You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Is your feature request related to a problem? Please describe.
The PR smoke test from #4026 installs each changed sp_*.sql onto a disposable SQL Server 2022 container, then runs it once — with @VersionCheckMode = 1. That flag is an unconditional early return (sp_Blitz.sql#L75-L78):
IF(@VersionCheckMode =1)
BEGIN
RETURN;
END;
So the pipeline proves each script parses and installs, and nothing more. No check body, no query, no output path ever executes. It's closer to a spell-check than a test, and the green checkmark reads as far stronger evidence than it is.
Not hypothetical — in #4045 two breakages passed this pipeline green and had to be caught by human review:
CheckID 93 referenced a caller-local variable inside a sp_executesql batch without passing it. The outer script compiles fine (the variable is just text in a string literal); Msg 137 fires only when the batch binds at runtime.
CheckID 212 ended a dynamic string mid-IF. The outer T-SQL was still validly balanced, so it compiled clean; the syntax error surfaces only when sp_executesql parses the string at runtime.
Both live behind that RETURN. No amount of install-time compilation could catch either.
Describe the solution you'd like
Execute what we install, across every non-deprecated script instead of only the changed one.
Run each proc and fail on error. Highest value by far — catches the entire class above. sqlcmd -b already fails on error severity.
Reuse Documentation/Development/Test in Azure.sql. It's already most of the parameter sweep — @SortOrder = 'all', sp_BlitzIndex@Mode 0-4, @ExpertMode, the @OutputTableName logging paths, sp_kill @ExecuteKills. The @Output* table paths especially deserve coverage: widely used, they write schema, nothing exercises them today.
Seed the server. A fresh container has no backup history, no plan cache, no maintenance plans. Statements still bind against empty tables (which is what catches runtime errors), but a small user database, a full and log backup, and some query activity make sp_BlitzCache/sp_BlitzFirst/sp_BlitzBackups meaningfully testable.
Compare findings before and after the change. Install the current dev version of the script, run it, save the list of findings it returns. Install the pull request's version, run the same thing, save that list. Compare the two lists. Most pull requests shouldn't change what the scripts report on a stock server, so any difference is worth showing the reviewer. This is what would give reviewers confidence on large mechanical rewrites like sp_Blitz: make it install and run on Azure SQL Database (#4040) #4045, where roughly 50 rewritten sections got no line-by-line review.
Extend it for sp_Blitz only — cheaper, but every script gets the same install-only treatment today.
Test against a real server instead of a container — richer data, but manual, and it can't block a bad pull request from merging.
Known limitations worth stating up front
The container is Linux SQL Server, so @IsWindowsOperatingSystem = 0 and Windows-only paths (xp_regread in CheckID 212, xp_cmdshell, cluster DMVs) either do nothing or behave differently. Binding still happens, so both #4045 bugs would still have been caught — but only a Windows host truly exercises those paths.
Azure SQL DB is not covered at all and can't be by a container. Given #4040/#4045 were entirely about Azure, and sp_ineachdb has Azure-specific paths keyed on EngineEdition = 5, that's a real gap. Covering it needs a live Azure SQL DB reachable from CI, which means storing credentials in GitHub Actions secrets.
Decisions needed
Decision 1 — When the new version reports different findings than the old version, what should the build do?
A. Fail the build on SQL errors only. When the findings lists differ, print the differences as a note but let the build pass. — Catches every sp_Blitz: make it install and run on Azure SQL Database (#4040) #4045-class bug without blocking pull requests that change findings on purpose. Downside: the printed notes are easy to ignore.
B. Fail the build any time the two lists differ at all. — Strictest, and correct for mechanical rewrites like sp_Blitz: make it install and run on Azure SQL Database (#4040) #4045 that shouldn't change behavior. Downside: it will fail on pull requests that improve a check on purpose, and people will start ignoring or disabling it.
C. Print everything, fail nothing. — No friction, and almost no enforcement.
Recommendation: A. A SQL error is objectively wrong. A changed finding might be the whole point of the pull request, so a human should look rather than the build guessing.
Decision 2 — Where does the list of test commands live?
A. CI runs Documentation/Development/Test in Azure.sql directly. — One file, so it can never fall out of sync with what we actually test. Downside: that file now has to stay CI-safe.
B. A separate CI script, initially copied from it. — Free to add CI-only setup and cleanup. Downside: two files that will drift apart over time.
Recommendation: A, falling back to B only if CI needs setup steps that file shouldn't carry.
Decision 3 — Azure SQL DB coverage.
A. Separate issue; get regular SQL Server covered first. — Ships value sooner. Downside: Azure stays uncovered in the meantime.
B. Include it here. — Complete, but nothing ships until the credentials question is settled.
Recommendation: A. It's a different problem with a security question attached.
Decision 4 — Procs that change things when they run (sp_kill @ExecuteKills = 'Y', sp_DatabaseRestore, sp_BlitzUpdate).
A. Test all of them, with sp_BlitzUpdate last since it overwrites the procs. — Full coverage. Downside: more setup work.
B. Skip those three. — Simpler. Downside: sp_kill's actual kill path and the restore logic stay untested.
Recommendation: A. The test server is thrown away afterward, which is exactly what makes these safe to test.
Do you want to build this feature yourself?
Happy to build it once the four above are settled.
Is your feature request related to a problem? Please describe.
The PR smoke test from #4026 installs each changed
sp_*.sqlonto a disposable SQL Server 2022 container, then runs it once — with@VersionCheckMode = 1. That flag is an unconditional early return (sp_Blitz.sql#L75-L78):So the pipeline proves each script parses and installs, and nothing more. No check body, no query, no output path ever executes. It's closer to a spell-check than a test, and the green checkmark reads as far stronger evidence than it is.
Not hypothetical — in #4045 two breakages passed this pipeline green and had to be caught by human review:
sp_executesqlbatch without passing it. The outer script compiles fine (the variable is just text in a string literal);Msg 137fires only when the batch binds at runtime.IF. The outer T-SQL was still validly balanced, so it compiled clean; the syntax error surfaces only whensp_executesqlparses the string at runtime.Both live behind that
RETURN. No amount of install-time compilation could catch either.Describe the solution you'd like
Execute what we install, across every non-deprecated script instead of only the changed one.
sqlcmd -balready fails on error severity.sp_Blitz,sp_BlitzAnalysis,sp_BlitzBackups,sp_BlitzCache,sp_BlitzFirst,sp_BlitzIndex,sp_BlitzLock,sp_BlitzWho,sp_DatabaseRestore,sp_ineachdb,sp_kill. OptionalScripts:sp_BlitzPlanCompare,sp_BlitzUpdate. Everything inDeprecated/stays out.Documentation/Development/Test in Azure.sql. It's already most of the parameter sweep —@SortOrder = 'all',sp_BlitzIndex@Mode0-4,@ExpertMode, the@OutputTableNamelogging paths,sp_kill @ExecuteKills. The@Output*table paths especially deserve coverage: widely used, they write schema, nothing exercises them today.sp_BlitzCache/sp_BlitzFirst/sp_BlitzBackupsmeaningfully testable.devversion of the script, run it, save the list of findings it returns. Install the pull request's version, run the same thing, save that list. Compare the two lists. Most pull requests shouldn't change what the scripts report on a stock server, so any difference is worth showing the reviewer. This is what would give reviewers confidence on large mechanical rewrites like sp_Blitz: make it install and run on Azure SQL Database (#4040) #4045, where roughly 50 rewritten sections got no line-by-line review.Describe alternatives you've considered
sp_Blitzonly — cheaper, but every script gets the same install-only treatment today.Known limitations worth stating up front
The container is Linux SQL Server, so
@IsWindowsOperatingSystem = 0and Windows-only paths (xp_regreadin CheckID 212,xp_cmdshell, cluster DMVs) either do nothing or behave differently. Binding still happens, so both #4045 bugs would still have been caught — but only a Windows host truly exercises those paths.Azure SQL DB is not covered at all and can't be by a container. Given #4040/#4045 were entirely about Azure, and
sp_ineachdbhas Azure-specific paths keyed onEngineEdition = 5, that's a real gap. Covering it needs a live Azure SQL DB reachable from CI, which means storing credentials in GitHub Actions secrets.Decisions needed
Decision 1 — When the new version reports different findings than the old version, what should the build do?
Recommendation: A. A SQL error is objectively wrong. A changed finding might be the whole point of the pull request, so a human should look rather than the build guessing.
Decision 2 — Where does the list of test commands live?
Documentation/Development/Test in Azure.sqldirectly. — One file, so it can never fall out of sync with what we actually test. Downside: that file now has to stay CI-safe.Recommendation: A, falling back to B only if CI needs setup steps that file shouldn't carry.
Decision 3 — Azure SQL DB coverage.
Recommendation: A. It's a different problem with a security question attached.
Decision 4 — Procs that change things when they run (
sp_kill @ExecuteKills = 'Y',sp_DatabaseRestore,sp_BlitzUpdate).sp_BlitzUpdatelast since it overwrites the procs. — Full coverage. Downside: more setup work.sp_kill's actual kill path and the restore logic stay untested.Recommendation: A. The test server is thrown away afterward, which is exactly what makes these safe to test.
Do you want to build this feature yourself?
Happy to build it once the four above are settled.