New test util stuff for testing schema migrations (#13258)

[ci postgres] [ci mysql]

backend/junit/test/metabase/junit.clj

@@ -3,6 +3,7 @@
   (:require [clojure
              [pprint :as pp]
              [string :as str]]
+            [metabase.util :as u]
             [pjstadig.print :as p]
             [test-report-junit-xml.core :as junit-xml]))
 
@@ -15,30 +16,31 @@
      (str "\n" message))))
 
 (defn- result-output [{:keys [expected actual diffs message], :as event}]
-  (with-out-str
-    (println (event-description event))
-    ;; this code is adapted from `pjstadig.util`
-    (p/with-pretty-writer
-      (fn []
-        (let [print-expected (fn [actual]
-                               (p/rprint "expected: ")
-                               (pp/pprint expected)
-                               (p/rprint "  actual: ")
-                               (pp/pprint actual)
-                               (p/clear))]
-          (if (seq diffs)
-            (doseq [[actual [a b]] diffs]
-              (print-expected actual)
-              (p/rprint "    diff:")
-              (if a
-                (do (p/rprint " - ")
-                    (pp/pprint a)
-                    (p/rprint "          + "))
-                (p/rprint " + "))
-              (when b
-                (pp/pprint b))
-              (p/clear))
-            (print-expected actual)))))))
+  (with-redefs [u/colorize? (constantly false)]
+    (with-out-str
+      (println (event-description event))
+      ;; this code is adapted from `pjstadig.util`
+      (p/with-pretty-writer
+        (fn []
+          (let [print-expected (fn [actual]
+                                 (p/rprint "expected: ")
+                                 (pp/pprint expected)
+                                 (p/rprint "  actual: ")
+                                 (pp/pprint actual)
+                                 (p/clear))]
+            (if (seq diffs)
+              (doseq [[actual [a b]] diffs]
+                (print-expected actual)
+                (p/rprint "    diff:")
+                (if a
+                  (do (p/rprint " - ")
+                      (pp/pprint a)
+                      (p/rprint "          + "))
+                  (p/rprint " + "))
+                (when b
+                  (pp/pprint b))
+                (p/clear))
+              (print-expected actual))))))))
 
 (defmulti format-result
   {:arglists '([event])}

modules/drivers/mongo/test/metabase/test/data/mongo.clj

@@ -18,12 +18,12 @@
 
 (defmethod tx/dbdef->connection-details :mongo
   [_ _ dbdef]
-  {:dbname (tx/escaped-name dbdef)
+  {:dbname (tx/escaped-database-name dbdef)
    :host   "localhost"})
 
 (defn- destroy-db! [driver dbdef]
   (with-open [mongo-connection (mg/connect (tx/dbdef->connection-details driver :server dbdef))]
-    (mg/drop-db mongo-connection (tx/escaped-name dbdef))))
+    (mg/drop-db mongo-connection (tx/escaped-database-name dbdef))))
 
 (defmethod tx/create-db! :mongo
   [driver {:keys [table-definitions], :as dbdef} & {:keys [skip-drop-db?], :or {skip-drop-db? false}}]

modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj

@@ -50,8 +50,8 @@
                 FOREIGN KEY (\"user_id\") REFERENCES \"v2_test-data\".\"PUBLIC\".\"users\" (\"id\");"
                "ALTER TABLE \"v2_test-data\".\"PUBLIC\".\"checkins\" ADD CONSTRAINT \"kins_venue_id_venues_621212269\"
                 FOREIGN KEY (\"venue_id\") REFERENCES \"v2_test-data\".\"PUBLIC\".\"venues\" (\"id\");"])
-             (ddl/create-db-ddl-statements :snowflake (-> (mt/get-dataset-definition dataset-defs/test-data)
-                                                          (update :database-name #(str "v2_" %)))))))))
+             (ddl/create-db-tables-ddl-statements :snowflake (-> (mt/get-dataset-definition dataset-defs/test-data)
+                                                                 (update :database-name #(str "v2_" %)))))))))
 
 ;; TODO -- disabled because these are randomly failing, will figure out when I'm back from vacation. I think it's a
 ;; bug in the JDBC driver -- Cam

modules/drivers/sqlite/test/metabase/test/data/sqlite.clj

@@ -8,7 +8,7 @@
 (sql-jdbc.tx/add-test-extensions! :sqlite)
 
 (defmethod tx/dbdef->connection-details :sqlite [_ context dbdef]
-  {:db (str (tx/escaped-name dbdef) ".sqlite")})
+  {:db (str (tx/escaped-database-name dbdef) ".sqlite")})
 
 (doseq [[base-type sql-type] {:type/BigInteger "BIGINT"
                               :type/Boolean    "BOOLEAN"

src/metabase/db.clj

@@ -248,14 +248,21 @@
    (when-let [max-pool-size (config/config-int :mb-application-db-max-connection-pool-size)]
      {"maxPoolSize" max-pool-size})))
 
+(defn quoting-style
+  "HoneySQL quoting style to use for application DBs of the given type. Note for H2 application DBs we automatically
+  uppercase all identifiers (since this is H2's default behavior) whereas in the SQL QP we stick with the case we got
+  when we synced the DB."
+  [db-type]
+  (case db-type
+    :postgres :ansi
+    :h2       :h2
+    :mysql    :mysql))
+
 (defn- create-connection-pool!
   "Create a connection pool for the application DB and set it as the default Toucan connection. This is normally called
   once during start up; calling it a second time (e.g. from the REPL) will "
   [jdbc-spec]
-  (db/set-default-quoting-style! (case (db-type)
-                                   :postgres :ansi
-                                   :h2       :h2
-                                   :mysql    :mysql))
+  (db/set-default-quoting-style! (quoting-style (db-type)))
   ;; REPL usage only: kill the old pool if one exists
   (u/ignore-exceptions
     (when-let [^PoolBackedDataSource pool (:datasource (db/connection))]

src/metabase/db/liquibase.clj

@@ -32,19 +32,18 @@
   ;; closing the `LiquibaseConnection`/`Database` closes the parent JDBC `Connection`, so only use it in combination
   ;; with `with-open` *if* we are opening a new JDBC `Connection` from a JDBC spec. If we're passed in a `Connection`,
   ;; it's safe to assume the caller is managing its lifecycle.
-  (letfn []
-    (cond
-      (instance? java.sql.Connection jdbc-spec-or-conn)
-      (f (-> jdbc-spec-or-conn liquibase-connection database liquibase))
+  (cond
+    (instance? java.sql.Connection jdbc-spec-or-conn)
+    (f (-> jdbc-spec-or-conn liquibase-connection database liquibase))
 
-      (:connection jdbc-spec-or-conn)
-      (recur (:connection jdbc-spec-or-conn) f)
+    (:connection jdbc-spec-or-conn)
+    (recur (:connection jdbc-spec-or-conn) f)
 
-      :else
-      (with-open [jdbc-conn      (jdbc/get-connection jdbc-spec-or-conn)
-                  liquibase-conn (liquibase-connection jdbc-conn)
-                  database       (database liquibase-conn)]
-        (f (liquibase database))))))
+    :else
+    (with-open [jdbc-conn      (jdbc/get-connection jdbc-spec-or-conn)
+                liquibase-conn (liquibase-connection jdbc-conn)
+                database       (database liquibase-conn)]
+      (f (liquibase database)))))
 
 (defmacro with-liquibase
   "Execute body with an instance of a `Liquibase` bound to `liquibase-binding`.
@@ -73,7 +72,7 @@
   filter out blank / comment lines. Even though this is not necessary for H2 or Postgres go ahead and do it anyway
   because it keeps the code simple and doesn't make a significant performance difference.
 
-  As of 0.31.1 this is only used for printing the migrations without running or using force migrating."
+  As of 0.31.1 this is only used for printing the migrations without running or when force migrating."
   [^Liquibase liquibase]
   (for [line  (str/split-lines (migrations-sql liquibase))
         :when (not (or (str/blank? line)

test/metabase/db/schema_migrations_test.clj

+(ns metabase.db.schema-migrations-test
+  "Tests for the schema migrations defined in the Liquibase YAML files. The basic idea is:
+
+  1. Create a temporary H2/Postgres/MySQL/MariaDB database
+  2. Run all migrations up to a certain point
+  3. Load some arbitrary data
+  4. run migration(s) after that point (verify that they actually run)
+  5. verify that data looks like what we'd expect after running migration(s)
+
+  See `metabase.db.schema-migrations-test.impl` for the implementation of this functionality."
+  (:require [clojure.test :refer :all]
+            [metabase.db.schema-migrations-test.impl :as impl]
+            [metabase.models :refer [Database Field Table]]
+            [toucan.db :as db]))
+
+(deftest database-position-test
+  (testing "Migration 165: add `database_position` to Field"
+    (impl/test-migrations 165 [migrate!]
+      ;; create a Database with a Table with 2 Fields
+      (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
+      (db/simple-insert! Table {:name "Table", :db_id 1, :created_at :%now, :updated_at :%now, :active true})
+      (let [mock-field {:table_id 1, :created_at :%now, :updated_at :%now, :base_type "type/Text", :database_type "VARCHAR"}]
+        (db/simple-insert! Field (assoc mock-field :name "Field 1"))
+        (db/simple-insert! Field (assoc mock-field :name "Field 2")))
+      (testing "sanity check: Fields should not have a `:database_position` column yet"
+        (is (not (contains? (Field 1) :database_position))))
+      ;; now run migration 165
+      (migrate!)
+      (testing "Fields should get `:database_position` equal to their IDs"
+        (doseq [id [1 2]]
+          (testing (format "Field %d" id)
+            (is (= id
+                   (db/select-one-field :database_position Field :id id)))))))))

test/metabase/db/schema_migrations_test/impl.clj

+(ns metabase.db.schema-migrations-test.impl
+  "Tests for the schema migrations defined in the Liquibase YAML files. The basic idea is:
+
+  1. Create a temporary H2/Postgres/MySQL/MariaDB database
+  2. Run all migrations up to a certain point
+  3. Load some arbitrary data
+  4. run migration(s) after that point (verify that they actually run)
+  5. verify that data looks like what we'd expect after running migration(s)
+
+  Actual tests using this code live in `metabase.db.schema-migrations-test`."
+  (:require [clojure.java.jdbc :as jdbc]
+            [clojure.test :refer :all]
+            [clojure.tools.logging :as log]
+            [metabase
+             [db :as mdb]
+             [driver :as driver]
+             [test :as mt]
+             [util :as u]]
+            [metabase.db.liquibase :as liquibase]
+            [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+            [metabase.test.data.interface :as tx]
+            [metabase.test.initialize :as initialize]
+            [toucan.db :as db])
+  (:import [liquibase Contexts Liquibase]
+           [liquibase.changelog ChangeSet DatabaseChangeLog]))
+
+(defmulti ^:private do-with-temp-empty-app-db*
+  "Create a new completely empty app DB for `driver`, then call `(f jdbc-spec)` with a spec for that DB. Should clean up
+  before and after running `f` as needed."
+  {:arglists '([driver f])}
+  driver/dispatch-on-initialized-driver
+  :hierarchy #'driver/hierarchy)
+
+(defmethod do-with-temp-empty-app-db* :default
+  [driver f]
+  (log/debugf "Creating empty %s app db..." driver)
+  (let [dbdef {:database-name     "schema-migrations-test-db"
+               :table-definitions []}]
+    (try
+      (tx/create-db! driver dbdef)
+      (let [connection-details (tx/dbdef->connection-details driver :db dbdef)
+            jdbc-spec          (sql-jdbc.conn/connection-details->spec driver connection-details)]
+        (f jdbc-spec))
+      (finally
+        (log/debugf "Destroying empty %s app db..." driver)
+        (tx/destroy-db! driver dbdef)))))
+
+(defmethod do-with-temp-empty-app-db* :h2
+  [driver f]
+  (log/debug "Creating empty H2 app db...")
+  ;; we don't need to destroy this DB manually because it will just get shutdown immediately when the Connection
+  ;; closes because we're not setting a `DB_CLOSE_DELAY`
+  ;;
+  ;; don't use the usual implementation of `tx/dbdef->connection-details` because it creates a spec that only connects
+  ;; to with `USER=GUEST` which doesn't let us run DDL statements
+  (let [connection-details {:db "mem:schema-migrations-test-db"}
+        jdbc-spec          (binding [mdb/*allow-potentailly-unsafe-connections* true]
+                             (sql-jdbc.conn/connection-details->spec driver connection-details))]
+    (f jdbc-spec)))
+
+(defn- do-with-temp-empty-app-db [driver f]
+  (do-with-temp-empty-app-db*
+   driver
+   (fn [jdbc-spec]
+     (with-open [conn (jdbc/get-connection jdbc-spec)]
+       (binding [toucan.db/*db-connection* {:connection conn}
+                 toucan.db/*quoting-style* (mdb/quoting-style driver)]
+         (f conn))))))
+
+(defmacro ^:private with-temp-empty-app-db
+  "Create a new temporary application DB of `db-type` and execute `body` with `conn-binding` bound to a
+  `java.sql.Connection` to the database. Toucan `*db-connection*` is also bound, which means Toucan functions like
+  `select` or `update!` will operate against this database."
+  [[conn-binding db-type] & body]
+  `(do-with-temp-empty-app-db ~db-type (fn [~(vary-meta conn-binding assoc :tag 'java.sql.Connection)] ~@body)))
+
+(defn- run-migrations-in-range!
+  "Run Liquibase migrations from our migrations YAML file in the range of `start-id` -> `end-id` (inclusive) against a
+  DB with `jdbc-spec`."
+  [^java.sql.Connection conn [start-id end-id]]
+  (liquibase/with-liquibase [liquibase conn]
+    (let [change-log        (.getDatabaseChangeLog liquibase)
+          ;; create a new change log that only has the subset of migrations we want to run.
+          subset-change-log (doto (DatabaseChangeLog.)
+                              ;; we don't actually use this for anything but if we don't set it then Liquibase barfs
+                              (.setPhysicalFilePath (.getPhysicalFilePath change-log)))]
+      ;; add the relevant migrations (change sets) to our subset change log
+      (doseq [^ChangeSet change-set (.getChangeSets change-log)
+              :let                  [id (Integer/parseUnsignedInt (.getId change-set))]
+              :when                 (<= start-id id end-id)]
+        (.addChangeSet subset-change-log change-set))
+      ;; now create a new instance of Liquibase that will run just the subset change log
+      (let [subset-liquibase (Liquibase. subset-change-log (.getResourceAccessor liquibase) (.getDatabase liquibase))]
+        (when-let [unrun (not-empty (.listUnrunChangeSets subset-liquibase nil))]
+          (log/debugf "Running migrations %s...%s (inclusive)"
+                      (.getId ^ChangeSet (first unrun)) (.getId ^ChangeSet (last unrun))))
+        ;; run the migrations
+        (.update subset-liquibase (Contexts.))))))
+
+(defn- test-migrations-for-driver [driver [start-id end-id] f]
+  (log/debug (u/format-color 'yellow "Testing migrations for driver %s..." driver))
+  (with-temp-empty-app-db [conn driver]
+    ;; sanity check: make sure the DB is actually empty
+    (let [metadata (.getMetaData conn)]
+      (with-open [rs (.getTables metadata nil nil "%" (into-array String ["TABLE"]))]
+        (let [tables (jdbc/result-set-seq rs)]
+          (assert (zero? (count tables))
+                  (str "'Empty' application DB is not actually empty. Found tables:\n"
+                       (u/pprint-to-str tables))))))
+    (run-migrations-in-range! conn [1 (dec start-id)])
+    (f #(run-migrations-in-range! conn [start-id end-id])))
+  (log/debug (u/format-color 'green "Done testing migrations for driver %s." driver)))
+
+(defn test-migrations*
+  [migration-range f]
+  ;; make sure the normal Metabase application DB is set up before running the tests so things don't get confused and
+  ;; try to initialize it while the mock DB is bound
+  (initialize/initialize-if-needed! :db)
+  (let [[start-id end-id] (if (sequential? migration-range)
+                            migration-range
+                            [migration-range migration-range])]
+    (testing (format "Migrations %d-%d" start-id end-id)
+      (mt/test-drivers #{:h2 :mysql :postgres}
+        (test-migrations-for-driver driver/*driver* [start-id end-id] f)))))
+
+(defmacro test-migrations
+  "Util macro for running tests for a set of Liquibase schema migration(s).
+
+  Before invoking body, migrations up to `start-id` are automatically ran. In body, you should do the following in
+  this order:
+
+  1. Load data and check any preconditions before running migrations you're testing. Prefer `toucan.db/simple-insert!`
+     or plain SQL for loading data to avoid dependencies on the current state of the schema that may be present in
+     Toucan `pre-insert` functions and the like.
+
+  2. Call `(migrate!)` to run migrations in range of `start-id` -> `end-id` (inclusive)
+
+  3. Check any postconditions after running the migrations.
+
+  e.g.
+
+    ;; example test for migrations 100-105
+    (test-migrations [100 105] [migrate!]
+      ;; (Migrations 1-99 are ran automatically before body is invoked)
+      ;; 1. Load data
+      (create-some-users!)
+      ;; 2. Run migrations 100-105
+      (migrate!)
+      ;; 3. Do some test assertions
+      (is (= ...)))
+
+  For convenience `migration-range` can be either a range of migrations IDs to test (e.g. `[100 105]`) or just a
+  single migration ID (e.g. `100`).
+
+  These run against the current set of test `DRIVERS` (by default H2), so if you want to run against more than H2
+  either set the `DRIVERS` env var or use `mt/set-test-drivers!` from the REPL."
+  {:style/indent 2}
+  [migration-range [migrate!-binding] & body]
+  `(test-migrations*
+    ~migration-range
+    (fn [~migrate!-binding]
+      ~@body)))

test/metabase/test/data/h2.clj

@@ -48,9 +48,10 @@
 
 (defmethod tx/dbdef->connection-details :h2
   [_ context dbdef]
-  {:db (str "mem:" (tx/escaped-name dbdef) (when (= context :db)
-                                             ;; Return details with the GUEST user added so SQL queries are allowed.
-                                             ";USER=GUEST;PASSWORD=guest"))})
+  {:db (str "mem:" (tx/escaped-database-name dbdef) (when (= context :db)
+                                                      ;; Return details with the GUEST user added so SQL queries are
+                                                      ;; allowed.
+                                                      ";USER=GUEST;PASSWORD=guest"))})
 
 (defmethod sql.tx/pk-sql-type :h2 [_] "BIGINT AUTO_INCREMENT")
 
@@ -58,7 +59,8 @@
 
 (defmethod sql.tx/drop-db-if-exists-sql :h2 [& _] nil)
 
-(defmethod sql.tx/create-db-sql :h2 [& _]
+(defmethod sql.tx/create-db-sql :h2
+  [& _]
   (str
    ;; We don't need to actually do anything to create a database here. Just disable the undo
    ;; log (i.e., transactions) for this DB session because the bulk operations to load data don't need to be atomic

test/metabase/test/data/impl.clj

@@ -93,10 +93,11 @@
       (tu.tz/with-system-timezone-id "UTC"
         (tx/create-db! driver database-definition)))
     ;; Add DB object to Metabase DB
-    (let [db (db/insert! Database
-               :name    database-name
-               :engine  (name driver)
-               :details (tx/dbdef->connection-details driver :db database-definition))]
+    (let [connection-details (tx/dbdef->connection-details driver :db database-definition)
+          db                 (db/insert! Database
+                               :name    database-name
+                               :engine  (name driver)
+                               :details connection-details)]
       (try
         ;; sync newly added DB
         (u/with-timeout sync-timeout-ms
@@ -112,16 +113,23 @@
         (Database (u/get-id db))
         (catch Throwable e
           (db/delete! Database :id (u/get-id db))
-          (throw e))))
+          (throw (ex-info "Failed to create test database"
+                          {:driver             driver
+                           :database-name      database-name
+                           :connection-details connection-details}
+                          e)))))
     (catch Throwable e
-      (printf "Failed to create %s '%s' test database:\n" driver database-name)
-      (println e)
-      (if config/is-test?
-        (System/exit -1)
-        (do
-          (println (u/format-color 'red "create-database! failed; destroying %s database %s" driver (pr-str database-name)))
-          (tx/destroy-db! driver database-definition)
-          (throw e))))))
+      (let [message (format "Failed to create %s '%s' test database" driver database-name)]
+        (println message "\n" e)
+        (if config/is-test?
+          (System/exit -1)
+          (do
+            (println (u/format-color 'red "create-database! failed; destroying %s database %s" driver (pr-str database-name)))
+            (tx/destroy-db! driver database-definition)
+            (throw (ex-info message
+                            {:driver        driver
+                             :database-name database-name}
+                            e))))))))
 
 (defmethod get-or-create-database! :default
   [driver dbdef]

test/metabase/test/data/interface.clj

@@ -170,7 +170,7 @@
   []
   (the-driver-with-test-extensions (or driver/*driver* :h2)))
 
-(defn escaped-name
+(defn escaped-database-name
   "Return escaped version of database name suitable for use as a filename / database name / etc."
   ^String [^DatabaseDefinition {:keys [database-name]}]
   {:pre [(string? database-name)]}
@@ -261,13 +261,18 @@
 
 (defmulti create-db!
   "Create a new database from `database-definition`, including adding tables, fields, and foreign key constraints,
-  and add the appropriate data. This method should drop existing databases with the same name if applicable, unless
-  the skip-drop-db? arg is true. This is to workaround a scenario where the postgres driver terminates the connection
-  before dropping the DB and causes some tests to fail. (This refers to creating the actual *DBMS* database itself,
-  *not* a Metabase `Database` object.)
+  and load the appropriate data. (This refers to creating the actual *DBMS* database itself, *not* a Metabase
+  `Database` object.)
 
   Optional `options` as third param. Currently supported options include `skip-drop-db?`. If unspecified,
-  `skip-drop-db?` should default to `false`."
+  `skip-drop-db?` should default to `false`.
+
+  This method should drop existing databases with the same name if applicable, unless the `skip-drop-db?` arg is
+  truthy. This is to work around a scenario where the Postgres driver terminates the connection before dropping the DB
+  and causes some tests to fail.
+
+  This method is not expected to return anything; use `dbdef->connection-details` to get connection details for this
+  database after you create it."
   {:arglists '([driver database-definition & {:keys [skip-drop-db?]}])}
   dispatch-on-driver-with-test-extensions
   :hierarchy #'driver/hierarchy)

test/metabase/test/data/postgres.clj

@@ -66,9 +66,10 @@
 
 (defmethod ddl/drop-db-ddl-statements :postgres
   [driver {:keys [database-name], :as dbdef} & options]
-  (assert (string? database-name)
-          (format "Expected String database name; got ^%s %s"
-                  (some-> database-name class .getCanonicalName) (pr-str database-name)))
+  (when-not (string? database-name)
+    (throw (ex-info (format "Expected String database name; got ^%s %s"
+                            (some-> database-name class .getCanonicalName) (pr-str database-name))
+                    {:driver driver, :dbdef dbdef})))
   ;; add an additional statement to the front to kill open connections to the DB before dropping
   (cons
    (kill-connections-to-db-sql database-name)

test/metabase/test/data/sql.clj

@@ -173,7 +173,6 @@
   tx/dispatch-on-driver-with-test-extensions
   :hierarchy #'driver/hierarchy)
 
-
 (defmulti create-db-sql
   "Return a `CREATE DATABASE` statement."
   {:arglists '([driver dbdef])}
@@ -183,7 +182,6 @@
 (defmethod create-db-sql :sql/test-extensions [driver {:keys [database-name]}]
   (format "CREATE DATABASE %s;" (qualify-and-quote driver database-name)))
 
-
 (defmulti drop-db-if-exists-sql
   "Return a `DROP DATABASE` statement."
   {:arglists '([driver dbdef])}
@@ -193,7 +191,6 @@
 (defmethod drop-db-if-exists-sql :sql/test-extensions [driver {:keys [database-name]}]
   (format "DROP DATABASE IF EXISTS %s;" (qualify-and-quote driver database-name)))
 
-
 (defmulti create-table-sql
   "Return a `CREATE TABLE` statement."
   {:arglists '([driver dbdef tabledef])}

test/metabase/test/data/sql/ddl.clj

@@ -23,18 +23,26 @@
 (defmethod drop-db-ddl-statements :sql/test-extensions
   [driver dbdef & {:keys [skip-drop-db?]}]
   (when-not skip-drop-db?
-    ;; Exec SQL for creating the DB
-    [(sql.tx/drop-db-if-exists-sql driver dbdef)
-     (sql.tx/create-db-sql driver dbdef)]))
+    (try
+      [(sql.tx/drop-db-if-exists-sql driver dbdef)]
+      (catch Throwable e
+        (throw (ex-info "Error generating DDL statements for dropping database"
+                        {:driver driver}
+                        e))))))
 
+(defn create-db-ddl-statements
+  "DDL statements to create the DB itself (does not include statements to drop the DB if it already exists)."
+  [driver dbdef]
+  [(sql.tx/create-db-sql driver dbdef)])
 
-(defmulti create-db-ddl-statements
-  "Return a default sequence of DDL statements for creating a DB (not including dropping it)."
+(defmulti create-db-tables-ddl-statements
+  "Return a default sequence of DDL statements for creating the tables/columns/etc. inside a Database. DOES NOT INCLUDE
+  STATEMENTS FOR CREATING (OR DROPPING) THE DATABASE ITSELF."
   {:arglists '([driver dbdef & {:as options}])}
   tx/dispatch-on-driver-with-test-extensions
   :hierarchy #'driver/hierarchy)
 
-(defmethod create-db-ddl-statements :sql/test-extensions
+(defmethod create-db-tables-ddl-statements :sql/test-extensions
   [driver {:keys [table-definitions], :as dbdef} & _]
   ;; Build combined statement for creating tables + FKs + comments
   (let [statements (atom [])
@@ -61,7 +69,6 @@
           (add! (sql.tx/standalone-column-comment-sql driver dbdef tabledef fielddef)))))
     @statements))
 
-
 ;; The methods below are currently only used by `:sql-jdbc` drivers, but you can use them to help implement your
 ;; `:sql` driver test extensions as well because there's nothing JDBC specific about them
 

test/metabase/test/data/sql_jdbc/execute.clj

@@ -12,7 +12,7 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- jdbc-execute! [db-spec sql]
-  (log/tracef "[execute] %s" (pr-str sql))
+  (log/tracef "[execute %s] %s" driver/*driver* (pr-str sql))
   (jdbc/execute! db-spec [sql] {:transaction? false, :multi? true}))
 
 (defn default-execute-sql! [driver context dbdef sql & {:keys [execute!]

test/metabase/test/data/sql_jdbc/load_data.clj

@@ -198,12 +198,16 @@
   "Default implementation of `create-db!` for SQL drivers."
   {:arglists '([driver dbdef & {:keys [skip-drop-db?]}])}
   [driver {:keys [table-definitions], :as dbdef} & options]
-  ;; first execute statements to drop/create the DB if needed (this will return nothing is `skip-drop-db?` is true)
+  ;; first execute statements to drop the DB if needed (this will do nothing if `skip-drop-db?` is true)
   (doseq [statement (apply ddl/drop-db-ddl-statements driver dbdef options)]
     (execute/execute-sql! driver :server dbdef statement))
+  ;; now execute statements to create the DB
+  (doseq [statement (ddl/create-db-ddl-statements driver dbdef)]
+    (execute/execute-sql! driver :server dbdef statement))
   ;; next, get a set of statements for creating the DB & Tables
-  (let [statements (apply ddl/create-db-ddl-statements driver dbdef options)]
-    ;; exec the combined statement
+  (let [statements (apply ddl/create-db-tables-ddl-statements driver dbdef options)]
+    ;; exec the combined statement. Notice we're now executing in the `:db` context e.g. executing them for a specific
+    ;; DB rather than on `:server` (no DB in particular)
     (execute/execute-sql! driver :db dbdef (str/join ";\n" statements)))
   ;; Now load the data for each Table
   (doseq [tabledef table-definitions]
@@ -213,5 +217,10 @@
 (defn destroy-db!
   "Default impl of `destroy-db!` for SQL drivers."
   [driver dbdef]
-  (doseq [statement (apply ddl/drop-db-ddl-statements driver dbdef)]
-    (execute/execute-sql! driver :server dbdef statement)))
+  (try
+    (doseq [statement (ddl/drop-db-ddl-statements driver dbdef)]
+      (execute/execute-sql! driver :server dbdef statement))
+    (catch Throwable e
+      (throw (ex-info "Error destroying database"
+                      {:driver driver, :dbdef dbdef}
+                      e)))))