Merge branch 'master' into newsletter_on_setup

src/metabase/models/field_values.clj

@@ -33,6 +33,7 @@
          (contains? field :base_type)
          (contains? field :special_type)]}
   (and (not= (keyword field_type) :sensitive)
+       (not (contains? #{:DateField :DateTimeField :TimeField} (keyword base_type)))
        (or (contains? #{:category :city :state :country} (keyword special_type))
            (= (keyword base_type) :BooleanField))))
 

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]])
@@ -19,7 +18,8 @@
                                                   :details {:host   "localhost"
                                                             :port   5432
                                                             :dbname "fakedb"
-                                                            :user   "cam"}}))
+                                                            :user   "cam"
+                                                            :ssl    false}}))
 
 ;; # DB LIFECYCLE ENDPOINTS
 
@@ -31,7 +31,7 @@
        :engine          "h2"
        :id              $
        :updated_at      $
-       :name            "Test Database"
+       :name            "test-data"
        :is_sample       false
        :organization_id nil
        :description     nil})
@@ -45,7 +45,7 @@
        :id              $
        :details         $
        :updated_at      $
-       :name            "Test Database"
+       :name            "test-data"
        :is_sample       false
        :organization_id nil
        :description     nil})
@@ -59,7 +59,7 @@
         {:created_at      $
          :engine          "postgres" ; string because it's coming back from API instead of DB
          :id              $
-         :details         {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
+         :details         {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
          :updated_at      $
          :name            db-name
          :is_sample       false
@@ -83,23 +83,17 @@
 (expect-let [[old-name new-name] (repeatedly 2 random-name)
              {db-id :id} (create-db old-name)
              sel-db (fn [] (sel :one :fields [Database :name :engine :details] :id db-id))]
-  [{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
+  [{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
     :engine  :postgres
     :name    old-name}
    {:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
     :engine  :h2
-    :name    new-name}
-   {:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
-    :engine  :h2
-    :name    old-name}]
+    :name    new-name}]
   [(sel-db)
    ;; Check that we can update all the fields
    (do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name    new-name
-                                                                        :engine  "h2"
-                                                                        :details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
-       (sel-db))
-   ;; Check that we can update just a single field
-   (do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name old-name})
+                                                                         :engine  "h2"
+                                                                         :details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
        (sel-db))])
 
 ;; # DATABASES FOR ORG
@@ -107,17 +101,17 @@
 ;; ## GET /api/database
 ;; Test that we can get all the DBs for an Org, ordered by name
 ;; Database details *should not* come back for Rasta since she's not a superuser
-(let [db-name (str "A" (random-name))] ; make sure this name comes before "Test Database"
+(let [db-name (str "A" (random-name))] ; make sure this name comes before "test-data"
   (expect-eval-actual-first
       (set (filter identity
-                   (conj (for [dataset-name datasets/all-valid-dataset-names]
-                           (datasets/when-testing-dataset dataset-name
-                             (match-$ (datasets/db (datasets/dataset-name->dataset dataset-name))
+                   (conj (for [engine datasets/all-valid-engines]
+                           (datasets/when-testing-engine engine
+                             (match-$ (datasets/db (datasets/engine->loader engine))
                                {:created_at      $
                                 :engine          (name $engine)
                                 :id              $
                                 :updated_at      $
-                                :name            "Test Database"
+                                :name            "test-data"
                                 :is_sample       false
                                 :organization_id nil
                                 :description     nil})))
@@ -133,9 +127,9 @@
     (do
       ;; Delete all the randomly created Databases we've made so far
       (cascade-delete Database :id [not-in (set (filter identity
-                                                        (for [dataset-name datasets/all-valid-dataset-names]
-                                                          (datasets/when-testing-dataset dataset-name
-                                                            (:id (datasets/db (datasets/dataset-name->dataset dataset-name)))))))])
+                                                        (for [engine datasets/all-valid-engines]
+                                                          (datasets/when-testing-engine engine
+                                                            (:id (datasets/db (datasets/engine->loader engine)))))))])
       ;; Add an extra DB so we have something to fetch besides the Test DB
       (create-db db-name)
       ;; Now hit the endpoint
@@ -150,7 +144,7 @@
        :engine          "h2"
        :id              $
        :updated_at      $
-       :name            "Test Database"
+       :name            "test-data"
        :is_sample       false
        :organization_id nil
        :description     nil

test/metabase/api/field_test.clj

@@ -24,7 +24,7 @@
                                                :engine          "h2"
                                                :id              $
                                                :updated_at      $
-                                               :name            "Test Database"
+                                               :name            "test-data"
                                                :is_sample       false
                                                :organization_id nil
                                                :description     nil})

test/metabase/api/table_test.clj

@@ -2,14 +2,13 @@
   "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]]
                              [foreign-key :refer [ForeignKey]]
                              [table :refer [Table]])
             [metabase.test.data :refer :all]
-            (metabase.test.data [data :as data]
+            (metabase.test.data [dataset-definitions :as defs]
                                 [datasets :as datasets]
                                 [users :refer :all])
             [metabase.test.util :refer [match-$ expect-eval-actual-first]]))
@@ -25,8 +24,8 @@
 
 ;; ## GET /api/table?org
 ;; These should come back in alphabetical order and include relevant metadata
-(expect (set (reduce concat (for [dataset-name datasets/test-dataset-names]
-                              (datasets/with-dataset-when-testing dataset-name
+(expect (set (reduce concat (for [engine datasets/test-engines]
+                              (datasets/with-engine-when-testing engine
                                 [{:name                (format-name "categories")
                                   :display_name        "Categories"
                                   :db_id               (id)
@@ -66,7 +65,7 @@
                            :engine          "h2"
                            :id              $
                            :updated_at      $
-                           :name            "Test Database"
+                           :name            "test-data"
                            :is_sample       false
                            :organization_id nil
                            :description     nil})
@@ -129,7 +128,7 @@
                            :engine          "h2"
                            :id              $
                            :updated_at      $
-                           :name            "Test Database"
+                           :name            "test-data"
                            :is_sample       false
                            :organization_id nil
                            :description     nil})
@@ -189,7 +188,7 @@
                               (+ (.getYear inst) 1900)
                               (+ (.getMonth inst) 1)
                               (.getDate inst)))]
-    (->> data/test-data
+    (->> defs/test-data
          :table-definitions
          first
          :rows
@@ -211,7 +210,7 @@
                            :engine          "h2"
                            :id              $
                            :updated_at      $
-                           :name            "Test Database"
+                           :name            "test-data"
                            :is_sample       false
                            :organization_id nil
                            :description     nil})
@@ -292,10 +291,7 @@
        :active          true
        :id              (id :users)
        :db_id           (id)
-       :field_values    {(keyword (str (id :users :last_login)))
-                         user-last-login-date-strs
-
-                         (keyword (str (id :users :name)))
+       :field_values    {(keyword (str (id :users :name)))
                          ["Broen Olujimi"
                           "Conchúr Tihomir"
                           "Dwight Gresham"
@@ -326,7 +322,7 @@
                            :engine          "h2"
                            :id              $
                            :updated_at      $
-                           :name            "Test Database"
+                           :name            "test-data"
                            :is_sample       false
                            :organization_id nil
                            :description     nil})
@@ -390,10 +386,7 @@
        :active          true
        :id              (id :users)
        :db_id           (id)
-       :field_values    {(keyword (str (id :users :last_login)))
-                         user-last-login-date-strs
-
-                         (keyword (str (id :users :name)))
+       :field_values    {(keyword (str (id :users :name)))
                          ["Broen Olujimi"
                           "Conchúr Tihomir"
                           "Dwight Gresham"
@@ -425,7 +418,7 @@
        :db              (match-$ (db)
                           {:description     nil
                            :organization_id $
-                           :name            "Test Database"
+                           :name            "test-data"
                            :is_sample       false
                            :updated_at      $
                            :details         $
@@ -494,7 +487,7 @@
                                              :db              (match-$ (db)
                                                                 {:description     nil,
                                                                  :organization_id nil,
-                                                                 :name            "Test Database",
+                                                                 :name            "test-data",
                                                                  :is_sample       false,
                                                                  :updated_at      $,
                                                                  :id              $,

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 [data :as data]
-                                [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) data/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,20 +5,21 @@
             [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 [data :refer [test-data]]
-                                [datasets :as datasets])
+            [metabase.test.data.datasets :as datasets]
             [metabase.test.util :refer [expect-eval-actual-first resolve-private-fns]]))
 
 ;; ## Logic for selectively running mongo
 
 (defmacro expect-when-testing-mongo [expected actual]
-  `(datasets/expect-when-testing-dataset :mongo
+  `(datasets/expect-when-testing-engine :mongo
      ~expected
      ~actual))
 
+(defn- mongo-db []
+  (datasets/db (datasets/engine->loader :mongo)))
+
 ;; ## Constants + Helper Fns/Macros
 ;; TODO - move these to metabase.test-data ?
 (def ^:private ^:const table-names
@@ -50,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
@@ -115,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
@@ -174,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
@@ -197,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/driver/postgres_test.clj

 (ns metabase.driver.postgres-test
   (:require [expectations :refer :all]
             [metabase.driver.postgres :refer [postgres]]
-            (metabase.test.data [datasets :refer [expect-with-dataset]]
+            (metabase.test.data [datasets :refer [expect-with-engine]]
                                 [interface :refer [def-database-definition]])
             [metabase.test.util.q :refer [Q]]))
 
@@ -45,7 +45,7 @@
     [#uuid "84ed434e-80b4-41cf-9c88-e334427104ae"]]])
 
 ;; Check that we can load a Postgres Database with a :UUIDField
-(expect-with-dataset :postgres
+(expect-with-engine :postgres
   {:cols    [{:description nil, :base_type :IntegerField, :schema_name "public", :name "id", :display_name "Id", :preview_display true, :special_type :id, :target nil, :extra_info {}}
              {:description nil, :base_type :UUIDField, :schema_name "public", :name "user_id", :display_name "User Id", :preview_display true, :special_type :category, :target nil, :extra_info {}}],
    :columns ["id" "user_id"],
@@ -60,7 +60,7 @@
       (update :cols (partial mapv #(dissoc % :id :table_id)))))
 
 ;; Check that we can filter by a UUID Field
-(expect-with-dataset :postgres
+(expect-with-engine :postgres
   [[2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]]
   (Q dataset metabase.driver.postgres-test/with-uuid use postgres
      return rows

test/metabase/driver/sync_test.clj

@@ -171,7 +171,7 @@
 (expect false (values-are-valid-json? ["false"]))
 
 
-(datasets/expect-with-dataset :postgres
+(datasets/expect-with-engine :postgres
   :json
   (with-temp-db
     [_

test/metabase/models/field_test.clj

@@ -5,6 +5,60 @@
                              [field-values :refer :all])
             [metabase.test.data :refer :all]))
 
+
+;; field-should-have-field-values?
+
+;; sensitive fields should always be excluded
+(expect false (field-should-have-field-values? {:base_type    :BooleanField
+                                                :special_type :category
+                                                :field_type   :sensitive}))
+;; date/time based fields should always be excluded
+(expect false (field-should-have-field-values? {:base_type    :DateField
+                                                :special_type :category
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :DateTimeField
+                                                :special_type :category
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :TimeField
+                                                :special_type :category
+                                                :field_type   :dimension}))
+;; most special types should be excluded
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :image
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :id
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :fk
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :latitude
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :number
+                                                :field_type   :dimension}))
+(expect false (field-should-have-field-values? {:base_type    :CharField
+                                                :special_type :timestamp_milliseconds
+                                                :field_type   :dimension}))
+;; boolean fields + category/city/state/country fields are g2g
+(expect true (field-should-have-field-values? {:base_type    :BooleanField
+                                               :special_type :number
+                                               :field_type   :dimension}))
+(expect true (field-should-have-field-values? {:base_type    :CharField
+                                               :special_type :category
+                                               :field_type   :dimension}))
+(expect true (field-should-have-field-values? {:base_type    :TextField
+                                               :special_type :city
+                                               :field_type   :dimension}))
+(expect true (field-should-have-field-values? {:base_type    :TextField
+                                               :special_type :state
+                                               :field_type   :dimension}))
+(expect true (field-should-have-field-values? {:base_type    :TextField
+                                               :special_type :country
+                                               :field_type   :dimension}))
+
+
 ;; Check that setting a Field's special_type to :category will cause a corresponding FieldValues to be created asynchronously
 (expect
     [nil

test/metabase/test/data.clj

 (ns metabase.test.data
   "Code related to creating and deleting test databases + datasets."
-  (:require (clojure [string :as s]
-                     [walk :as walk])
-            [clojure.tools.logging :as log]
-            [colorize.core :as color]
-            [medley.core :as m]
+  (:require [clojure.tools.logging :as log]
             (metabase [db :refer :all]
                       [driver :as driver])
             (metabase.models [database :refer [Database]]
                              [field :refer [Field] :as field]
                              [table :refer [Table]])
-            (metabase.test.data [data :as data]
-                                [datasets :as datasets :refer [*dataset*]]
+            (metabase.test.data [datasets :as datasets :refer [*data-loader*]]
                                 [h2 :as h2]
                                 [interface :refer :all])
             [metabase.util :as u])
@@ -24,7 +19,7 @@
 ;; These functions offer a generic way to get bits of info like Table + Field IDs from any of our many driver/dataset combos.
 
 (def ^:dynamic *get-db*
-  (fn [] (datasets/db *dataset*)))
+  (fn [] (datasets/db *data-loader*)))
 
 (defn db
   "Return the current database.
@@ -41,7 +36,7 @@
        ~@body)))
 
 (defn format-name [nm]
-  (datasets/format-name *dataset* (name nm)))
+  (datasets/format-name *data-loader* (name nm)))
 
 (defn- get-table-id-or-explode [db-id table-name]
   (let [table-name (format-name table-name)]
@@ -86,11 +81,10 @@
   []
   (contains? (:features (driver)) :foreign-keys))
 
-(defn default-schema []       (datasets/default-schema *dataset*))
-(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*))
+(defn default-schema []       (datasets/default-schema *data-loader*))
+(defn id-field-type []        (datasets/id-field-type *data-loader*))
+(defn sum-field-type []       (datasets/sum-field-type *data-loader*))
+(defn timestamp-field-type [] (datasets/timestamp-field-type *data-loader*))
 
 
 ;; ## Loading / Deleting Test Datasets
@@ -98,9 +92,9 @@
 (defn get-or-create-database!
   "Create DBMS database associated with DATABASE-DEFINITION, create corresponding Metabase `Databases`/`Tables`/`Fields`, and sync the `Database`.
    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."
+   current dataset (`*data-loader*`), which is H2 by default."
   ([^DatabaseDefinition database-definition]
-   (get-or-create-database! (dataset-loader) database-definition))
+   (get-or-create-database! *data-loader* database-definition))
   ([dataset-loader {:keys [database-name], :as ^DatabaseDefinition database-definition}]
    (let [engine (engine dataset-loader)]
      (or (metabase-instance database-definition engine)
@@ -141,9 +135,9 @@
 (defn remove-database!
   "Delete Metabase `Database`, `Fields` and `Tables` associated with DATABASE-DEFINITION, then remove the physical database from the associated DBMS.
    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*`."
+   current dataset, bound to `*data-loader*`."
   ([^DatabaseDefinition database-definition]
-   (remove-database! (dataset-loader) database-definition))
+   (remove-database! *data-loader* 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))))
@@ -153,7 +147,7 @@
 
 
 (defn -with-temp-db [^DatabaseDefinition dbdef f]
-  (let [loader (dataset-loader)
+  (let [loader *data-loader*
         dbdef  (map->DatabaseDefinition (assoc dbdef :short-lived? true))]
     (try
       (with-db (binding [*sel-disable-logging* true]

test/metabase/test/data/dataset_definitions.clj

@@ -3,24 +3,6 @@
   (:require [clojure.tools.reader.edn :as edn]
             [metabase.test.data.interface :refer [def-database-definition]]))
 
-;; ## Helper Functions
-
-(defn- unix-timestamp-ms
-  "Create a Unix timestamp (in milliseconds).
-
-     (unix-timestamp-ms :year 2012 :month 12 :date 27)"
-  ^Long [& {:keys [year month date hour minute second nano]
-            :or   {year 0, month 1, date 1, hour 0, minute 0, second 0, nano 0}}]
-  (-> (java.sql.Timestamp. (- year 1900) (- month 1) date hour minute second nano)
-      .getTime
-      long)) ; coerce to long since Korma doesn't know how to insert bigints
-
-
-(defn- unix-timestamp
-  "Create a Unix timestamp, in seconds."
-  ^Long [& args]
-  (/ (apply unix-timestamp-ms args) 1000))
-
 
 ;; ## Datasets
 
@@ -29,9 +11,12 @@
 ;; TODO - move this to interface
 ;; TODO - make rows be lazily loadable for DB definitions from a file
 (defmacro ^:private def-database-definition-edn [dbname]
-  `(def-database-definition ~dbname
+  `(def-database-definition ~(vary-meta dbname assoc :const true)
      ~@(edn/read-string (slurp (str edn-definitions-dir (name dbname) ".edn")))))
 
+;; The O.G. "Test Database" dataset
+(def-database-definition-edn test-data)
+
 ;; Times when the Toucan cried
 (def-database-definition-edn sad-toucan-incidents)
 

test/metabase/test/data/datasets.clj

@@ -7,16 +7,20 @@
             [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 [data :as data]
+            (metabase.test.data [dataset-definitions :as defs]
                                 [h2 :as h2]
                                 [mongo :as mongo]
                                 [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,226 +47,201 @@
 
 ;; ## 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) data/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)}))
+                               (s/upper-case table-or-field-name))
+          :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)}))
 
 
 ;; # Concrete Instances
 
-(def ^:private dataset-name->dataset*
+(def ^:private engine->loader*
   "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
+(def ^:const all-valid-engines
   "Set of names of all valid datasets."
-  (set (keys dataset-name->dataset*)))
+  (set (keys engine->loader*)))
 
-(defn dataset-name->dataset [engine]
-  (or (dataset-name->dataset* engine)
-      (throw (Exception.(format "Invalid engine: %s\nMust be one of: %s" engine all-valid-dataset-names)))))
+(defn engine->loader [engine]
+  (or (engine->loader* engine)
+      (throw (Exception.(format "Invalid engine: %s\nMust be one of: %s" engine all-valid-engines)))))
 
 
 ;; # Logic for determining which datasets to test against
 
 ;; By default, we'll test against against only the :h2 (H2) dataset; otherwise, you can specify which
-;; datasets to test against by setting the env var `MB_TEST_DATASETS` to a comma-separated list of dataset names, e.g.
+;; datasets to test against by setting the env var `ENGINES` to a comma-separated list of dataset names, e.g.
 ;;
 ;;    # test against :h2 and :mongo
-;;    MB_TEST_DATASETS=generic-sql,mongo
+;;    ENGINES=generic-sql,mongo
 ;;
 ;;    # just test against :h2 (default)
-;;    MB_TEST_DATASETS=generic-sql
+;;    ENGINES=generic-sql
 
-(defn- get-test-datasets-from-env
-  "Return a set of dataset names to test against from the env var `MB_TEST_DATASETS`."
+(defn- get-engines-from-env
+  "Return a set of dataset names to test against from the env var `ENGINES`."
   []
-  (when-let [env-drivers (some-> (env :mb-test-datasets)
-                                 s/lower-case)]
-    (some->> (s/split env-drivers #",")
+  (when-let [env-engines (some-> (env :engines) s/lower-case)]
+    (some->> (s/split env-engines #",")
              (map keyword)
              ;; Double check that the specified datasets are all valid
-             (map (fn [dataset-name]
-                    (assert (contains? all-valid-dataset-names dataset-name)
-                      (format "Invalid dataset specified in MB_TEST_DATASETS: %s" (name dataset-name)))
-                    dataset-name))
+             (map (fn [engine]
+                    (assert (contains? all-valid-engines engine)
+                      (format "Invalid dataset specified in ENGINES: %s" (name engine)))
+                    engine))
              set)))
 
-(defonce ^:const
-  ^{:doc (str "Set of names of drivers we should run tests against. "
-              "By default, this only contains `:h2` but can be overriden by setting env var `MB_TEST_DATASETS`.")}
-  test-dataset-names
-  (let [datasets (or (get-test-datasets-from-env)
-                     #{:h2})]
-    (log/info (color/green "Running QP tests against these datasets: " datasets))
-    datasets))
+(def ^:const test-engines
+  "Set of names of drivers we should run tests against.
+   By default, this only contains `:h2` but can be overriden by setting env var `ENGINES`."
+  (let [engines (or (get-engines-from-env)
+                    #{:h2})]
+    (log/info (color/green "Running QP tests against these engines: " engines))
+    engines))
 
 
 ;; # Helper Macros
 
-(def ^:dynamic *dataset*
-  "The dataset we're currently testing against, bound by `with-dataset`.
-   Defaults to `(dataset-name->dataset :h2)`."
-  (dataset-name->dataset (if (contains? test-dataset-names :h2) :h2
-                             (first test-dataset-names))))
+(def ^:private ^:const default-engine
+  (if (contains? test-engines :h2) :h2
+      (first test-engines)))
 
 (def ^:dynamic *engine*
   "Keyword name of the engine that we're currently testing against. Defaults to `:h2`."
-  :h2)
+  default-engine)
+
+(def ^:dynamic *data-loader*
+  "The dataset we're currently testing against, bound by `with-engine`.
+   Defaults to `(engine->loader :h2)`."
+  (engine->loader default-engine))
+
+
 
-(defmacro with-dataset
-  "Bind `*dataset*` to the dataset with DATASET-NAME and execute BODY."
-  [dataset-name & body]
-  `(let [engine# ~dataset-name]
+(defmacro with-engine
+  "Bind `*data-loader*` to the dataset with ENGINE and execute BODY."
+  [engine & body]
+  `(let [engine# ~engine]
      (binding [*engine*  engine#
-               *dataset* (dataset-name->dataset engine#)]
+               *data-loader* (engine->loader engine#)]
        ~@body)))
 
-(defmacro when-testing-dataset
-  "Execute BODY only if we're currently testing against DATASET-NAME."
-  [dataset-name & body]
-  `(when (contains? test-dataset-names ~dataset-name)
+(defmacro when-testing-engine
+  "Execute BODY only if we're currently testing against ENGINE."
+  [engine & body]
+  `(when (contains? test-engines ~engine)
      ~@body))
 
-(defmacro with-dataset-when-testing
-  "When testing DATASET-NAME, binding `*dataset*` and executes BODY."
-  [dataset-name & body]
-  `(when-testing-dataset ~dataset-name
-     (with-dataset ~dataset-name
+(defmacro with-engine-when-testing
+  "When testing ENGINE, binding `*data-loader*` and executes BODY."
+  [engine & body]
+  `(when-testing-engine ~engine
+     (with-engine ~engine
        ~@body)))
 
-(defmacro expect-when-testing-dataset
-  "Generate a unit test that only runs if we're currently testing against DATASET-NAME."
-  [dataset-name expected actual]
-  `(expect
-       (when-testing-dataset ~dataset-name
-         ~expected)
-     (when-testing-dataset ~dataset-name
+(defmacro expect-when-testing-engine
+  "Generate a unit test that only runs if we're currently testing against ENGINE."
+  [engine expected actual]
+  `(when-testing-engine ~engine
+     (expect ~expected
        ~actual)))
 
-(defmacro expect-with-dataset
-  "Generate a unit test that only runs if we're currently testing against DATASET-NAME, and that binds `*dataset*` to the current dataset."
-  [dataset-name expected actual]
-  `(expect-when-testing-dataset ~dataset-name
-     (with-dataset ~dataset-name
+(defmacro expect-with-engine
+  "Generate a unit test that only runs if we're currently testing against ENGINE, and that binds `*data-loader*` to the current dataset."
+  [engine expected actual]
+  `(expect-when-testing-engine ~engine
+     (with-engine ~engine
        ~expected)
-     (with-dataset ~dataset-name
+     (with-engine ~engine
        ~actual)))
 
-(defmacro expect-with-datasets
-  "Generate unit tests for all datasets in DATASET-NAMES; each test will only run if we're currently testing the corresponding dataset.
-   `*dataset*` is bound to the current dataset inside each test."
-  [dataset-names expected actual]
-  `(do ~@(for [dataset-name (eval dataset-names)]
-           `(expect-with-dataset ~dataset-name ~expected ~actual))))
+(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.
+   `*data-loader*` is bound to the current dataset inside each test."
+  [engines expected actual]
+  `(do ~@(for [engine (eval engines)]
+           `(expect-with-engine ~engine ~expected ~actual))))
 
-(defmacro expect-with-all-datasets
+(defmacro expect-with-all-engines
   "Generate unit tests for all valid datasets; each test will only run if we're currently testing the corresponding dataset.
-  `*dataset*` is bound to the current dataset inside each test."
+  `*data-loader*` is bound to the current dataset inside each test."
   [expected actual]
-  `(expect-with-datasets ~all-valid-dataset-names ~expected ~actual))
+  `(expect-with-engines ~all-valid-engines ~expected ~actual))
 
-(defmacro dataset-case
+(defmacro engine-case
   "Case statement that switches off of the current dataset.
 
-     (dataset-case
+     (engine-case
        :h2       ...
        :postgres ...)"
   [& pairs]
-  `(cond ~@(mapcat (fn [[dataset then]]
-                     (assert (contains? all-valid-dataset-names dataset))
-                     [`(= *engine* ~dataset)
+  `(cond ~@(mapcat (fn [[engine then]]
+                     (assert (contains? all-valid-engines engine))
+                     [`(= *engine* ~engine)
                       then])
                    (partition 2 pairs))))

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))