Merge pull request #1491 from metabase/simplify-test-dataset-loaders

Simplify test dataset loaders

test/metabase/api/database_test.clj

@@ -2,7 +2,6 @@
   (:require [expectations :refer :all]
             [metabase.db :refer :all]
             [metabase.driver :as driver]
-            [metabase.driver.mongo.test-data :as mongo-test-data]
             (metabase.models [database :refer [Database]]
                              [field :refer [Field]]
                              [table :refer [Table]])

test/metabase/api/table_test.clj

@@ -2,7 +2,6 @@
   "Tests for /api/table endpoints."
   (:require [expectations :refer :all]
             [metabase.db :refer :all]
-            [metabase.driver.mongo.test-data :as mongo-data :refer [mongo-test-db-id]]
             (metabase [http-client :as http]
                       [middleware :as middleware])
             (metabase.models [field :refer [Field]]

test/metabase/driver/mongo/test_data.clj

-(ns metabase.driver.mongo.test-data
-  "Functionality related to creating / loading a test database for the Mongo driver."
-  (:require [medley.core :as m]
-            [metabase.db :refer :all]
-            (metabase.models [database :refer [Database]]
-                             [field :refer [Field]]
-                             [table :refer [Table]])
-            (metabase.test.data [dataset-definitions :as defs]
-                                [mongo :as loader])
-            [metabase.util :as u]))
-
-;; ## MONGO-TEST-DB + OTHER DELAYS
-
-(defonce
-  ^{:doc "A delay that fetches or creates the Mongo test `Database`.
-          If DB is created, `load-data` and `sync-database!` are called to get the DB in a state that we can use for testing."}
-  mongo-test-db
-  (delay (@(resolve 'metabase.test.data/get-or-create-database!) (loader/dataset-loader) defs/test-data)))
-
-(defonce
-  ^{:doc "A Delay that returns the ID of `mongo-test-db`, forcing creation of it if needed."}
-  mongo-test-db-id
-  (delay (let [id (:id @mongo-test-db)]
-           (assert (integer? id))
-           id)))

test/metabase/driver/mongo_test.clj

@@ -5,7 +5,6 @@
             [metabase.db :refer :all]
             [metabase.driver :as driver]
             [metabase.driver.mongo :refer [mongo]]
-            [metabase.driver.mongo.test-data :refer :all]
             (metabase.models [field :refer [Field]]
                              [table :refer [Table]])
             [metabase.test.data.datasets :as datasets]
@@ -18,6 +17,9 @@
      ~expected
      ~actual))
 
+(defn- mongo-db []
+  (datasets/db (datasets/dataset-name->dataset :mongo)))
+
 ;; ## Constants + Helper Fns/Macros
 ;; TODO - move these to metabase.test-data ?
 (def ^:private ^:const table-names
@@ -49,7 +51,7 @@
   "Return an object that can be passed like a `Table` to driver sync functions."
   [table-name]
   {:pre [(keyword? table-name)]}
-  {:db   mongo-test-db
+  {:db   (delay (mongo-db))
    :name (name table-name)})
 
 (defn- field-name->fake-field
@@ -114,7 +116,7 @@
       {:name "categories"}
       {:name "users"}
       {:name "venues"}}
-    ((:active-tables mongo) @mongo-test-db))
+    ((:active-tables mongo) (mongo-db)))
 
 ;; ### table->column-names
 (expect-when-testing-mongo
@@ -173,7 +175,7 @@
      {:rows 1000, :active true, :name "checkins"}
      {:rows 15,   :active true, :name "users"}
      {:rows 100,  :active true, :name "venues"}]
-  (sel :many :fields [Table :name :active :rows] :db_id @mongo-test-db-id (k/order :name)))
+  (sel :many :fields [Table :name :active :rows] :db_id (:id (mongo-db)) (k/order :name)))
 
 ;; Test that Fields got synced correctly, and types are correct
 (expect-when-testing-mongo
@@ -196,6 +198,6 @@
   (let [table->fields (fn [table-name]
                         (sel :many :fields [Field :name :base_type :special_type]
                              :active true
-                             :table_id (sel :one :id Table :db_id @mongo-test-db-id, :name (name table-name))
+                             :table_id (sel :one :id Table :db_id (:id (mongo-db)), :name (name table-name))
                              (k/order :name)))]
     (map table->fields table-names)))

test/metabase/test/data.clj

@@ -85,7 +85,6 @@
 (defn id-field-type []        (datasets/id-field-type *dataset*))
 (defn sum-field-type []       (datasets/sum-field-type *dataset*))
 (defn timestamp-field-type [] (datasets/timestamp-field-type *dataset*))
-(defn dataset-loader []       (datasets/dataset-loader *dataset*))
 
 
 ;; ## Loading / Deleting Test Datasets
@@ -95,7 +94,7 @@
    DATASET-LOADER should be an object that implements `IDatasetLoader`; it defaults to the value returned by the method `dataset-loader` for the
    current dataset (`*dataset*`), which is H2 by default."
   ([^DatabaseDefinition database-definition]
-   (get-or-create-database! (dataset-loader) database-definition))
+   (get-or-create-database! *dataset* database-definition))
   ([dataset-loader {:keys [database-name], :as ^DatabaseDefinition database-definition}]
    (let [engine (engine dataset-loader)]
      (or (metabase-instance database-definition engine)
@@ -138,7 +137,7 @@
    DATASET-LOADER should be an object that implements `IDatasetLoader`; by default it is the value returned by the method `dataset-loader` for the
    current dataset, bound to `*dataset*`."
   ([^DatabaseDefinition database-definition]
-   (remove-database! (dataset-loader) database-definition))
+   (remove-database! *dataset* database-definition))
   ([dataset-loader ^DatabaseDefinition database-definition]
    ;; Delete the Metabase Database and associated objects
    (cascade-delete Database :id (:id (metabase-instance database-definition (engine dataset-loader))))
@@ -148,7 +147,7 @@
 
 
 (defn -with-temp-db [^DatabaseDefinition dbdef f]
-  (let [loader (dataset-loader)
+  (let [loader *dataset*
         dbdef  (map->DatabaseDefinition (assoc dbdef :short-lived? true))]
     (try
       (with-db (binding [*sel-disable-logging* true]

test/metabase/test/data/datasets.clj

@@ -7,7 +7,6 @@
             [expectations :refer :all]
             [metabase.db :refer :all]
             [metabase.driver :as driver]
-            [metabase.driver.mongo.test-data :as mongo-data]
             (metabase.models [field :refer [Field]]
                              [table :refer [Table]])
             (metabase.test.data [dataset-definitions :as defs]
@@ -16,7 +15,12 @@
                                 [mysql :as mysql]
                                 [postgres :as postgres]
                                 [sqlserver :as sqlserver])
-            [metabase.util :as u]))
+            [metabase.util :as u])
+  (:import metabase.test.data.h2.H2DatasetLoader
+           metabase.test.data.mongo.MongoDatasetLoader
+           metabase.test.data.mysql.MySQLDatasetLoader
+           metabase.test.data.postgres.PostgresDatasetLoader
+           metabase.test.data.sqlserver.SQLServerDatasetLoader))
 
 ;; # IDataset
 
@@ -24,8 +28,6 @@
   "Functions needed to fetch test data for various drivers."
   (load-data! [this]
     "Load the test data for this dataset.")
-  (dataset-loader [this]
-    "Return a dataset loader (an object that implements `IDatasetLoader`) for this dataset/driver.")
   (db [this]
     "Return `Database` containing test data for this driver.")
   (default-schema [this]
@@ -45,96 +47,70 @@
 
 ;; ## Mongo
 
-(deftype MongoDriverData []
-  IDataset
-  (load-data! [_]
-    @mongo-data/mongo-test-db
-    (assert (integer? @mongo-data/mongo-test-db-id)))
-
-  (dataset-loader [_]
-    (mongo/dataset-loader))
-
-  (db [_]
-    @mongo-data/mongo-test-db)
-
-  (format-name [_ table-or-field-name]
-    (if (= table-or-field-name "id") "_id"
-        table-or-field-name))
+(defn- generic-load-data! [{:keys [dbpromise], :as this}]
+  (when-not (realized? dbpromise)
+    (deliver dbpromise (@(resolve 'metabase.test.data/get-or-create-database!) this defs/test-data)))
+  @dbpromise)
 
-  (default-schema       [_] nil)
-  (id-field-type        [_] :IntegerField)
-  (sum-field-type       [_] :IntegerField)
-  (timestamp-field-type [_] :DateField))
+(extend MongoDatasetLoader
+  IDataset
+  {:load-data!           generic-load-data!
+   :db                   generic-load-data!
+   :format-name          (fn [_ table-or-field-name]
+                           (if (= table-or-field-name "id") "_id"
+                               table-or-field-name))
+   :default-schema       (constantly nil)
+   :id-field-type        (constantly :IntegerField)
+   :sum-field-type       (constantly :IntegerField)
+   :timestamp-field-type (constantly :DateField)})
 
 
 ;; ## Generic SQL
 
-(defn- generic-sql-load-data! [{:keys [dbpromise], :as this}]
-  (when-not (realized? dbpromise)
-    (deliver dbpromise (@(resolve 'metabase.test.data/get-or-create-database!) (dataset-loader this) defs/test-data)))
-  @dbpromise)
-
 (def ^:private GenericSQLIDatasetMixin
-  {:load-data!           generic-sql-load-data!
-   :db                   generic-sql-load-data!
+  {:load-data!           generic-load-data!
+   :db                   generic-load-data!
    :format-name          (fn [_ table-or-field-name]
                            table-or-field-name)
    :timestamp-field-type (constantly :DateTimeField)
-   :id-field-type        (constantly :IntegerField)})
+   :id-field-type        (constantly :IntegerField)
+   :sum-field-type       (constantly :BigIntegerField)
+   :default-schema       (constantly nil)})
 
 
 ;;; ### H2
 
-(defrecord H2DriverData [dbpromise])
-
-(extend H2DriverData
+(extend H2DatasetLoader
   IDataset
   (merge GenericSQLIDatasetMixin
-         {:dataset-loader    (fn [_]
-                               (h2/dataset-loader))
-          :default-schema    (constantly "PUBLIC")
+         {:default-schema    (constantly "PUBLIC")
           :format-name       (fn [_ table-or-field-name]
                                (clojure.string/upper-case table-or-field-name))
-          :id-field-type     (constantly :BigIntegerField)
-          :sum-field-type    (constantly :BigIntegerField)}))
+          :id-field-type     (constantly :BigIntegerField)}))
 
 
 ;;; ### Postgres
 
-(defrecord PostgresDriverData [dbpromise])
-
-(extend PostgresDriverData
+(extend PostgresDatasetLoader
   IDataset
   (merge GenericSQLIDatasetMixin
-         {:dataset-loader (fn [_]
-                            (postgres/dataset-loader))
-          :default-schema (constantly "public")
+         {:default-schema (constantly "public")
           :sum-field-type (constantly :IntegerField)}))
 
 
 ;;; ### MySQL
 
-(defrecord MySQLDriverData [dbpromise])
-
-(extend MySQLDriverData
+(extend MySQLDatasetLoader
   IDataset
-  (merge GenericSQLIDatasetMixin
-         {:dataset-loader (fn [_]
-                            (mysql/dataset-loader))
-          :default-schema (constantly nil)
-          :sum-field-type (constantly :BigIntegerField)}))
+  GenericSQLIDatasetMixin)
 
 
 ;;; ### SQLServer
 
-(defrecord SQLServerDriverData [dbpromise])
-
-(extend SQLServerDriverData
+(extend SQLServerDatasetLoader
   IDataset
   (merge GenericSQLIDatasetMixin
-         {:dataset-loader (fn [_]
-                            (sqlserver/dataset-loader))
-          :default-schema (constantly "dbo")
+         {:default-schema (constantly "dbo")
           :sum-field-type (constantly :IntegerField)}))
 
 
@@ -142,11 +118,11 @@
 
 (def ^:private dataset-name->dataset*
   "Map of dataset keyword name -> dataset instance (i.e., an object that implements `IDataset`)."
-  {:mongo     (MongoDriverData.)
-   :h2        (H2DriverData.        (promise))
-   :postgres  (PostgresDriverData.  (promise))
-   :mysql     (MySQLDriverData.     (promise))
-   :sqlserver (SQLServerDriverData. (promise))})
+  {:mongo     (MongoDatasetLoader.     (promise))
+   :h2        (H2DatasetLoader.        (promise))
+   :postgres  (PostgresDatasetLoader.  (promise))
+   :mysql     (MySQLDatasetLoader.     (promise))
+   :sqlserver (SQLServerDatasetLoader. (promise))})
 
 (def ^:const all-valid-dataset-names
   "Set of names of all valid datasets."

test/metabase/test/data/generic_sql.clj

@@ -222,7 +222,7 @@
 (defn- destroy-db! [loader dbdef]
   (execute-sql! loader :server dbdef (drop-db-if-exists-sql loader dbdef)))
 
-(def ^:const IDatasetLoaderMixin
+(def IDatasetLoaderMixin
   "Mixin for `IGenericSQLDatasetLoader` types to implemnt `create-db!` and `destroy-db!` from `IDatasetLoader`."
   {:create-db!  create-db!
    :destroy-db! destroy-db!})

test/metabase/test/data/h2.clj

@@ -64,7 +64,7 @@
    (format "GRANT ALL ON %s TO GUEST;" (quote-name this table-name))))
 
 
-(defrecord H2DatasetLoader [])
+(defrecord H2DatasetLoader [dbpromise])
 
 (extend H2DatasetLoader
   generic/IGenericSQLDatasetLoader
@@ -91,6 +91,3 @@
   (merge generic/IDatasetLoaderMixin
          {:database->connection-details database->connection-details
           :engine                       (constantly :h2)}))
-
-(defn dataset-loader []
-  (->H2DatasetLoader))

test/metabase/test/data/mongo.clj

@@ -39,7 +39,7 @@
               (catch com.mongodb.MongoException _))))))))
 
 
-(defrecord MongoDatasetLoader [])
+(defrecord MongoDatasetLoader [dbpromise])
 
 (extend MongoDatasetLoader
   i/IDatasetLoader
@@ -47,6 +47,3 @@
    :destroy-db!                  destroy-db!
    :database->connection-details database->connection-details
    :engine                       (constantly :mongo)})
-
-(defn ^MongoDatasetLoader dataset-loader []
-  (->MongoDatasetLoader))

test/metabase/test/data/mysql.clj

@@ -37,7 +37,7 @@
     (when (seq statement)
       (generic/default-execute-sql! loader context dbdef statement))))
 
-(defrecord MySQLDatasetLoader [])
+(defrecord MySQLDatasetLoader [dbpromise])
 
 (extend MySQLDatasetLoader
   generic/IGenericSQLDatasetLoader
@@ -51,7 +51,3 @@
   (merge generic/IDatasetLoaderMixin
          {:database->connection-details database->connection-details
           :engine                       (constantly :mysql)}))
-
-
-(defn dataset-loader []
-  (->MySQLDatasetLoader))

test/metabase/test/data/postgres.clj

@@ -31,7 +31,7 @@
   (format "DROP TABLE IF EXISTS \"%s\" CASCADE;" table-name))
 
 
-(defrecord PostgresDatasetLoader [])
+(defrecord PostgresDatasetLoader [dbpromise])
 
 (extend PostgresDatasetLoader
   generic/IGenericSQLDatasetLoader
@@ -44,6 +44,3 @@
   (merge generic/IDatasetLoaderMixin
          {:database->connection-details database->connection-details
           :engine                       (constantly :postgres)}))
-
-(defn dataset-loader []
-  (->PostgresDatasetLoader))

test/metabase/test/data/sqlserver.clj

@@ -78,7 +78,7 @@
    [(+suffix db-name) "dbo" table-name field-name]))
 
 
-(defrecord SQLServerDatasetLoader [])
+(defrecord SQLServerDatasetLoader [dbpromise])
 
 (extend SQLServerDatasetLoader
   generic/IGenericSQLDatasetLoader
@@ -97,9 +97,6 @@
             :database->connection-details database->connection-details
             :engine                       (constantly :sqlserver)})))
 
-(defn dataset-loader []
-  (->SQLServerDatasetLoader))
-
 
 (defn- cleanup-leftover-dbs
   "Clean up any leftover DBs that weren't destroyed by the last test run (eg, if it failed for some reason).