Merge pull request #5229 from metabase/fix-h2-test-fns-not-working-in-repl

Fix h2 test fns not working in repl

test/metabase/test/data.clj

@@ -32,11 +32,14 @@
 ;; These functions offer a generic way to get bits of info like Table + Field IDs from any of our many driver/dataset combos.
 
 (defn get-or-create-test-data-db!
-  "Get or create the Test Data database for DATA-LOADER, which defaults to `*driver*`."
-  ([]            (get-or-create-test-data-db! *driver*))
-  ([data-loader] (get-or-create-database! data-loader defs/test-data)))
+  "Get or create the Test Data database for DRIVER, which defaults to `*driver*`."
+  ([]       (get-or-create-test-data-db! *driver*))
+  ([driver] (get-or-create-database! driver defs/test-data)))
 
-(def ^:dynamic ^:private *get-db* get-or-create-test-data-db!)
+(def ^:dynamic ^:private *get-db*
+  "Implementation of `db` function that should return the current working test database when called, always with no arguments.
+   By default, this is `get-or-create-test-data-db!` for the current `*driver*`, which does exactly what it suggests."
+  get-or-create-test-data-db!)
 
 (defn db
   "Return the current database.
@@ -54,8 +57,6 @@
   [db & body]
   `(do-with-db ~db (fn [] ~@body)))
 
-(defn- parts->id [table-name ])
-
 (defn- $->id
   "Convert symbols like `$field` to `id` fn calls. Input is split into separate args by splitting the token on `.`.
    With no `.` delimiters, it is assumed we're referring to a Field belonging to TABLE-NAME, which is passed implicitly as the first arg.
@@ -207,7 +208,7 @@
 
 (defn get-or-create-database!
   "Create DBMS database associated with DATABASE-DEFINITION, create corresponding Metabase `Databases`/`Tables`/`Fields`, and sync the `Database`.
-   DRIVER should be an object that implements `IDatasetLoader`; it defaults to the value returned by the method `driver` for the
+   DRIVER should be an object that implements `IDriverTestExtensions`; it defaults to the value returned by the method `driver` for the
    current dataset (`*driver*`), which is H2 by default."
   ([database-definition]
    (get-or-create-database! *driver* database-definition))

test/metabase/test/data/bigquery.clj

@@ -198,11 +198,11 @@
           (throw e))))))
 
 
-;;; # ------------------------------------------------------------ IDatasetLoader ------------------------------------------------------------
+;;; # ------------------------------------------------------------ IDriverTestExtensions ------------------------------------------------------------
 
 (u/strict-extend BigQueryDriver
-  i/IDatasetLoader
-  (merge i/IDatasetLoaderDefaultsMixin
+  i/IDriverTestExtensions
+  (merge i/IDriverTestExtensionsDefaultsMixin
          {:engine                       (constantly :bigquery)
           :database->connection-details (u/drop-first-arg database->connection-details)
           :create-db!                   (u/drop-first-arg create-db!)}))

test/metabase/test/data/crate.clj

@@ -64,8 +64,8 @@
           :drop-db-if-exists-sql     (constantly nil)
           :load-data!                (make-load-data-fn generic/load-data-add-ids)
           :qualified-name-components (partial i/single-db-qualified-name-components "doc")})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details database->connection-details
           :engine                       (constantly :crate)
           :default-schema               (constantly "doc")}))

test/metabase/test/data/datasets.clj

@@ -56,38 +56,59 @@
   "Keyword name of the engine that we're currently testing against. Defaults to `:h2`."
   default-engine)
 
+(defn- engine->test-extensions-ns-symbol
+  "Return the namespace where we'd expect to find test extensions for the driver with ENGINE keyword.
+
+     (engine->test-extensions-ns-symbol :h2) ; -> 'metabase.test.data.h2"
+  [engine]
+  (symbol (str "metabase.test.data." (name engine))))
+
 (defn- engine->driver
   "Like `driver/engine->driver`, but reloads the relevant test data namespace as well if needed."
   [engine]
   (try (i/engine (driver/engine->driver engine))
        (catch IllegalArgumentException _
-         (require (symbol (str "metabase.test.data." (name engine))) :reload)))
+         (require (engine->test-extensions-ns-symbol engine) :reload)))
   (driver/engine->driver engine))
 
 (def ^:dynamic *driver*
   "The driver we're currently testing against, bound by `with-engine`.
    This is just a regular driver, e.g. `MySQLDriver`, with an extra promise keyed by `:dbpromise`
    that is used to store the `test-data` dataset when you call `load-data!`."
-  (driver/engine->driver default-engine))
+  (engine->driver default-engine))
 
-(defn do-with-engine [engine f]
+(defn do-with-engine
+  "Bind `*engine*` and `*driver*` as appropriate for ENGINE and execute F, a function that takes no args."
+  {:style/indent 1}
+  [engine f]
   (binding [*engine* engine
             *driver* (engine->driver engine)]
     (f)))
 
 (defmacro with-engine
   "Bind `*driver*` to the dataset with ENGINE and execute BODY."
+  {:style/indent 1}
   [engine & body]
   `(do-with-engine ~engine (fn [] ~@body)))
 
+(defn do-when-testing-engine
+  "Call function F (always with no arguments) *only* if we are currently testing against ENGINE.
+   (This does NOT bind `*driver*`; use `do-with-engine` if you want to do that.)"
+  {:style/indent 1}
+  [engine f]
+  (when (contains? test-engines engine)
+    (f)))
+
 (defmacro when-testing-engine
-  "Execute BODY only if we're currently testing against ENGINE."
+  "Execute BODY only if we're currently testing against ENGINE.
+   (This does NOT bind `*driver*`; use `with-engine-when-testing` if you want to do that.)"
+  {:style/indent 1}
   [engine & body]
-  `(when (contains? test-engines ~engine)
-     ~@body))
+  `(do-when-testing-engine ~engine (fn [] ~@body)))
 
 (defmacro with-engine-when-testing
   "When testing ENGINE, binding `*driver*` and executes BODY."
+  {:style/indent 1}
   [engine & body]
   `(when-testing-engine ~engine
      (with-engine ~engine
@@ -95,6 +116,7 @@
 
 (defmacro expect-with-engine
   "Generate a unit test that only runs if we're currently testing against ENGINE, and that binds `*driver*` to the driver for ENGINE."
+  {:style/indent 1}
   [engine expected actual]
   `(when-testing-engine ~engine
      (expect
@@ -104,6 +126,7 @@
 (defmacro expect-with-engines
   "Generate unit tests for all datasets in ENGINES; each test will only run if we're currently testing the corresponding dataset.
    `*driver*` is bound to the current dataset inside each test."
+  {:style/indent 1}
   [engines expected actual]
   ;; Make functions to get expected/actual so the code is only compiled one time instead of for every single driver
   ;; speeds up loading of metabase.driver.query-processor-test significantly
@@ -125,7 +148,7 @@
 
 
 ;;; Load metabase.test.data.* namespaces for all available drivers
-(doseq [[engine _] (driver/available-drivers)]
-  (let [driver-test-namespace (symbol (str "metabase.test.data." (name engine)))]
+(doseq [engine all-valid-engines]
+  (let [driver-test-namespace (engine->test-extensions-ns-symbol engine)]
     (when (find-ns driver-test-namespace)
       (require driver-test-namespace))))

test/metabase/test/data/druid.clj

@@ -16,8 +16,8 @@
                                (throw (Exception. "In order to test Druid, you must specify `MB_DRUID_PORT`."))))})
 
 (u/strict-extend DruidDriver
-  i/IDatasetLoader
-  (merge i/IDatasetLoaderDefaultsMixin
+  i/IDriverTestExtensions
+  (merge i/IDriverTestExtensionsDefaultsMixin
          {:engine                       (constantly :druid)
           :database->connection-details database->connection-details
           :create-db!                   (constantly nil)}))

test/metabase/test/data/generic_sql.clj

@@ -8,9 +8,7 @@
              [helpers :as h]]
             [medley.core :as m]
             [metabase.driver.generic-sql :as sql]
-            [metabase.test.data
-             [datasets :as datasets]
-             [interface :as i]]
+            [metabase.test.data.interface :as i]
             [metabase.util :as u]
             [metabase.util.honeysql-extensions :as hx])
   (:import clojure.lang.Keyword
@@ -21,8 +19,8 @@
 
 (defprotocol IGenericSQLDatasetLoader
   "Methods for loading `DatabaseDefinition` in a SQL database.
-   A type that implements `IGenericSQLDatasetLoader` can be made to implement most of `IDatasetLoader`
-   by using the `IDatasetLoaderMixin`.
+   A type that implements `IGenericSQLDatasetLoader` can be made to implement most of `IDriverTestExtensions`
+   by using the `IDriverTestExtensionsMixin`.
 
    Methods marked *Optional* below have a default implementation specified in `DefaultsMixin`."
   (field-base-type->sql-type [this, ^Keyword base-type]
@@ -277,7 +275,7 @@
    :quote-name                default-quote-name})
 
 
-;; ## ------------------------------------------------------------ IDatasetLoader impl ------------------------------------------------------------
+;; ## ------------------------------------------------------------ IDriverTestExtensions impl ------------------------------------------------------------
 
 (defn sequentially-execute-sql!
   "Alternative implementation of `execute-sql!` that executes statements one at a time for drivers
@@ -317,9 +315,9 @@
   (doseq [tabledef table-definitions]
     (load-data! driver dbdef tabledef)))
 
-(def IDatasetLoaderMixin
-  "Mixin for `IGenericSQLDatasetLoader` types to implement `create-db!` from `IDatasetLoader`."
-  (merge i/IDatasetLoaderDefaultsMixin
+(def IDriverTestExtensionsMixin
+  "Mixin for `IGenericSQLDatasetLoader` types to implement `create-db!` from `IDriverTestExtensions`."
+  (merge i/IDriverTestExtensionsDefaultsMixin
          {:create-db! create-db!}))
 
 
@@ -330,17 +328,21 @@
    Useful for doing engine-specific setup or teardown."
   {:style/indent 2}
   [engine get-connection-spec & sql-and-args]
-  (datasets/when-testing-engine engine
-    (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
-    (jdbc/execute! (get-connection-spec) sql-and-args)
-    (println (u/format-color 'blue "[OK]"))))
+  ((resolve 'metabase.test.data.datasets/do-when-testing-engine)
+   engine
+   (fn []
+     (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
+     (jdbc/execute! (get-connection-spec) sql-and-args)
+     (println (u/format-color 'blue "[OK]")))))
 
 (defn query-when-testing!
   "Execute a prepared SQL-AND-ARGS **query** against Database with spec returned by GET-CONNECTION-SPEC only when running tests against ENGINE.
    Useful for doing engine-specific setup or teardown where `execute-when-testing!` won't work because the query returns results."
   {:style/indent 2}
   [engine get-connection-spec & sql-and-args]
-  (datasets/when-testing-engine engine
-    (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
-    (u/prog1 (jdbc/query (get-connection-spec) sql-and-args)
-      (println (u/format-color 'blue "[OK] -> %s" (vec <>))))))
+  ((resolve 'metabase.test.data.datasets/do-when-testing-engine)
+   engine
+   (fn []
+     (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
+     (u/prog1 (jdbc/query (get-connection-spec) sql-and-args)
+       (println (u/format-color 'blue "[OK] -> %s" (vec <>)))))))

test/metabase/test/data/h2.clj

@@ -69,8 +69,8 @@
             :prepare-identifier        (u/drop-first-arg s/upper-case)
             :quote-name                (u/drop-first-arg quote-name)}))
 
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details       (u/drop-first-arg database->connection-details)
           :default-schema                     (constantly "PUBLIC")
           :engine                             (constantly :h2)

test/metabase/test/data/interface.clj

 (ns metabase.test.data.interface
   "`Definition` types for databases, tables, fields; related protocols, helper functions.
 
-   Objects that implement `IDatasetLoader` know how to load a `DatabaseDefinition` into an
+   Objects that implement `IDriverTestExtensions` know how to load a `DatabaseDefinition` into an
    actual physical RDMS database. This functionality allows us to easily test with multiple datasets."
   (:require [clojure.string :as str]
             [metabase
@@ -85,11 +85,11 @@
     (Database :name database-name, :engine (name engine-kw))))
 
 
-;; ## IDatasetLoader
+;; ## IDriverTestExtensions
 
-(defprotocol IDatasetLoader
+(defprotocol IDriverTestExtensions
   "Methods for creating, deleting, and populating *pyhsical* DBMS databases, tables, and fields.
-   Methods marked *OPTIONAL* have default implementations in `IDatasetLoaderDefaultsMixin`."
+   Methods marked *OPTIONAL* have default implementations in `IDriverTestExtensionsDefaultsMixin`."
   (engine ^clojure.lang.Keyword [this]
     "Return the engine keyword associated with this database, e.g. `:h2` or `:mongo`.")
 
@@ -126,7 +126,8 @@
   (id-field-type ^clojure.lang.Keyword [this]
     "*OPTIONAL* Return the `base_type` of the `id` `Field` (e.g. `:type/Integer` or `:type/BigInteger`). Defaults to `:type/Integer`."))
 
-(def IDatasetLoaderDefaultsMixin
+(def IDriverTestExtensionsDefaultsMixin
+  "Default implementations for the `IDriverTestExtensions` methods marked *OPTIONAL*."
   {:expected-base-type->actual         (u/drop-first-arg identity)
    :default-schema                     (constantly nil)
    :format-name                        (u/drop-first-arg identity)

test/metabase/test/data/mongo.clj

@@ -41,8 +41,8 @@
 
 
 (u/strict-extend MongoDriver
-  i/IDatasetLoader
-  (merge i/IDatasetLoaderDefaultsMixin
+  i/IDriverTestExtensions
+  (merge i/IDriverTestExtensionsDefaultsMixin
          {:create-db!                   (u/drop-first-arg create-db!)
           :database->connection-details database->connection-details
           :engine                       (constantly :mongo)

test/metabase/test/data/mysql.clj

@@ -43,7 +43,7 @@
           :load-data!                generic/load-data-all-at-once!
           :pk-sql-type               (constantly "INTEGER NOT NULL AUTO_INCREMENT")
           :quote-name                (u/drop-first-arg quote-name)})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details (u/drop-first-arg database->connection-details)
           :engine                       (constantly :mysql)}))

test/metabase/test/data/oracle.clj

@@ -83,8 +83,8 @@
           :pk-sql-type               (constantly "INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 1 INCREMENT BY 1) NOT NULL") ; LOL
           :qualified-name-components (partial i/single-db-qualified-name-components session-schema)})
 
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details       (fn [& _] @db-connection-details)
           :default-schema                     (constantly session-schema)
           :engine                             (constantly :oracle)

test/metabase/test/data/postgres.clj

@@ -53,8 +53,8 @@
           :field-base-type->sql-type (u/drop-first-arg field-base-type->sql-type)
           :load-data!                generic/load-data-all-at-once!
           :pk-sql-type               (constantly "SERIAL")})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details       (u/drop-first-arg database->connection-details)
           :default-schema                     (constantly "public")
           :engine                             (constantly :postgres)

test/metabase/test/data/presto.clj

@@ -21,7 +21,7 @@
                                  (s/upper-case (s/replace (name env-var) #"-" "_")))))))
 
 
-;;; IDatasetLoader implementation
+;;; IDriverTestExtensions implementation
 
 (defn- database->connection-details [context {:keys [database-name]}]
   (merge {:host (get-env-var :host)
@@ -92,11 +92,11 @@
         (execute-presto-query! details (insert-sql dbdef tabledef batch))))))
 
 
-;;; IDatasetLoader implementation
+;;; IDriverTestExtensions implementation
 
 (u/strict-extend PrestoDriver
-  i/IDatasetLoader
-  (merge i/IDatasetLoaderDefaultsMixin
+  i/IDriverTestExtensions
+  (merge i/IDriverTestExtensionsDefaultsMixin
          {:engine                             (constantly :presto)
           :database->connection-details       (u/drop-first-arg database->connection-details)
           :create-db!                         (u/drop-first-arg create-db!)

test/metabase/test/data/redshift.clj

@@ -57,8 +57,8 @@
           :pk-sql-type               (constantly "INTEGER IDENTITY(1,1)")
           :qualified-name-components (partial i/single-db-qualified-name-components session-schema-name)})
 
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details (fn [& _]
                                           @db-connection-details)
           :default-schema               (constantly session-schema-name)

test/metabase/test/data/sqlite.clj

@@ -41,7 +41,7 @@
           :load-data!                (generic/make-load-data-fn load-data-stringify-dates generic/load-data-chunked)
           :pk-sql-type               (constantly "INTEGER")
           :field-base-type->sql-type (u/drop-first-arg field-base-type->sql-type)})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details (u/drop-first-arg database->connection-details)
           :engine                       (constantly :sqlite)}))

test/metabase/test/data/sqlserver.clj

@@ -82,8 +82,8 @@
           :field-base-type->sql-type (u/drop-first-arg field-base-type->sql-type)
           :pk-sql-type               (constantly "INT IDENTITY(1,1)")
           :qualified-name-components (u/drop-first-arg qualified-name-components)})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details (u/drop-first-arg database->connection-details)
           :default-schema               (constantly "dbo")
           :engine                       (constantly :sqlserver)}))

test/metabase/test/data/vertica.clj

@@ -50,8 +50,8 @@
           :pk-sql-type               (constantly "INTEGER")
           :qualified-name-components (u/drop-first-arg qualified-name-components)
           :execute-sql!              generic/sequentially-execute-sql!})
-  i/IDatasetLoader
-  (merge generic/IDatasetLoaderMixin
+  i/IDriverTestExtensions
+  (merge generic/IDriverTestExtensionsMixin
          {:database->connection-details       (fn [& _] @db-connection-details)
           :default-schema                     (constantly "public")
           :engine                             (constantly :vertica)

test/metabase/test/util.clj

@@ -26,19 +26,38 @@
             [metabase.util :as u]
             [toucan.util.test :as test]))
 
-(declare $->prop)
-
 ;; ## match-$
 
-(defmacro match-$
+(defn- $->prop
+  "If FORM is a symbol starting with a `$`, convert it to the form `(form-keyword SOURCE-OBJ)`.
+
+    ($->prop my-obj 'fish)  -> 'fish
+    ($->prop my-obj '$fish) -> '(:fish my-obj)"
+  [source-obj form]
+  (or (when (and (symbol? form)
+                 (= (first (name form)) \$)
+                 (not= form '$))
+        (if (= form '$$)
+          source-obj
+          `(~(keyword (apply str (rest (name form)))) ~source-obj)))
+      form))
+
+(defmacro ^:deprecated match-$
   "Walk over map DEST-OBJECT and replace values of the form `$`, `$key`, or `$$` as follows:
 
     {k $}     -> {k (k SOURCE-OBJECT)}
     {k $symb} -> {k (:symb SOURCE-OBJECT)}
     $$        -> {k SOURCE-OBJECT}
+
   ex.
 
     (match-$ m {:a $, :b 3, :c $b}) -> {:a (:a m), b 3, :c (:b m)}"
+  ;; DEPRECATED - This is an old pattern for writing tests and is probably best avoided going forward.
+  ;; Tests that use this macro end up being huge, often with giant maps with many values that are `$`.
+  ;; It's better just to write a helper function that only keeps values relevant to the tests you're writing
+  ;; and use that to pare down the results (e.g. only keeping a handful of keys relevant to the test).
+  ;; Alternatively, you can also consider converting fields that naturally change to boolean values indiciating their presence;
+  ;; see the `boolean-ids-and-timestamps` function below
   [source-obj dest-object]
   {:pre [(map? dest-object)]}
   (let [source##    (gensym)
@@ -46,32 +65,20 @@
                                {k (condp = v
                                     '$ `(~k ~source##)
                                     '$$ source##
-                                        v)}))]
+                                    v)}))]
     `(let [~source## ~source-obj]
-       ~(clojure.walk/prewalk (partial $->prop source##)
-                              dest-object))))
-
-(defn- $->prop
-  "If FORM is a symbol starting with a `$`, convert it to the form `(form-keyword SOURCE-OBJ)`.
-
-    ($->prop my-obj 'fish)  -> 'fish
-    ($->prop my-obj '$fish) -> '(:fish my-obj)"
-  [source-obj form]
-  (or (when (and (symbol? form)
-                 (= (first (name form)) \$)
-                 (not= form '$))
-        (if (= form '$$)
-          source-obj
-          `(~(keyword (apply str (rest (name form)))) ~source-obj)))
-      form))
+       ~(walk/prewalk (partial $->prop source##)
+                      dest-object))))
 
 
 ;;; random-name
-(let [random-uppercase-letter (partial rand-nth (mapv char (range (int \A) (inc (int \Z)))))]
-  (defn random-name
-    "Generate a random string of 20 uppercase letters."
-    []
-    (apply str (repeatedly 20 random-uppercase-letter))))
+(def ^:private ^{:arglists '([])} random-uppercase-letter
+  (partial rand-nth (mapv char (range (int \A) (inc (int \Z))))))
+
+(defn random-name
+  "Generate a random string of 20 uppercase letters."
+  []
+  (apply str (repeatedly 20 random-uppercase-letter)))
 
 (defn random-email
   "Generate a random email address."
@@ -100,7 +107,7 @@
   (require 'metabase.test.data.users)
   ((resolve 'metabase.test.data.users/user->id) username))
 
-(defn- rasta-id     [] (user-id :rasta))
+(defn- rasta-id [] (user-id :rasta))
 
 
 (u/strict-extend (class Card)