Test fixes & cleanup [ci drivers] 🔧 🚿

flyforfreedom · Nov 28, 2016 · a33f3dd · a33f3dd
1 parent 791dfdf
commit a33f3dd
Show file tree

Hide file tree

Showing 14 changed files with 112 additions and 197 deletions.
diff --git a/.gitignore b/.gitignore
@@ -43,3 +43,4 @@ bin/release/aws-eb/metabase-aws-eb.zip
 /plugins
 /build.xml
 /test-report-*
+/crate-*
diff --git a/test/metabase/driver/bigquery_test.clj b/test/metabase/driver/bigquery_test.clj
@@ -6,68 +6,6 @@
             (metabase.test.data [datasets :refer [expect-with-engine]]
                                 [interface :refer [def-database-definition]])))
 
-;; Make sure that paging works correctly for the bigquery driver when fetching a list of tables
-;; Default page size is 50 so if we have more than that number of tables make sure they all show up
-(def-database-definition ^:private fifty-one-different-tables
-  ["birds_1"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_2"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_3"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_4"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_5"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_6"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_7"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_8"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_9"  [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_10" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_11" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_12" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_13" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_14" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_15" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_16" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_17" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_18" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_19" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_20" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_21" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_22" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_23" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_24" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_25" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_26" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_27" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_28" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_29" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_30" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_31" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_32" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_33" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_34" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_35" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_36" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_37" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_38" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_39" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_40" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_41" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_42" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_43" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_44" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_45" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_46" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_47" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_48" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_49" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_50" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]]
-  ["birds_51" [{:field-name "name", :base-type :type/Text}] [["Rasta"] ["Lucky"]]])
-
-;; only run this test 1 out of every 5 times since it takes like 5-10 minutes just to sync the DB and we don't have all day
-(when (> (rand) 0.80)
-  (expect-with-engine :bigquery
-    51
-    (data/with-temp-db [db fifty-one-different-tables]
-      (count (database/tables db)))))
-
 
 ;; Test native queries
 (expect-with-engine :bigquery

diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
@@ -5,11 +5,21 @@
   (:require [clojure.set :as set]
             [expectations :refer :all]
             [metabase.driver :as driver]
-            #_[metabase.query-processor :refer :all]
             [metabase.test.data :as data]
             [metabase.test.data.datasets :as datasets]
+            metabase.test.data.interface
             [metabase.util :as u]))
 
+;; make sure all the driver test extension namespaces are loaded <3
+;; if this isn't done some things will get loaded at the wrong time which can end up causing test databases to be created more than once, which fails
+(doseq [engine (keys (driver/available-drivers))]
+  (let [test-ns (symbol (str "metabase.test.data." (name engine)))]
+    (try
+      (require test-ns)
+      (catch Throwable e
+        (println (format "Error loading %s: %s" test-ns (.getMessage e)))))))
+
+
 ;;; ------------------------------------------------------------ Helper Fns + Macros ------------------------------------------------------------
 
 ;; Event-Based DBs aren't tested here, but in `event-query-processor-test` instead.

diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
@@ -166,95 +166,68 @@
 
 ;; ## Loading / Deleting Test Datasets
 
+(defn- add-extra-metadata!
+  "Add extra metadata like Field base-type, etc."
+  [database-definition db]
+  (doseq [^TableDefinition table-definition (:table-definitions database-definition)]
+    (let [table-name (:table-name table-definition)
+          table      (delay (or  (i/metabase-instance table-definition db)
+                                 (throw (Exception. (format "Table '%s' not loaded from definiton:\n%s\nFound:\n%s"
+                                                            table-name
+                                                            (u/pprint-to-str (dissoc table-definition :rows))
+                                                            (u/pprint-to-str (db/select [Table :schema :name], :db_id (:id db))))))))]
+      (doseq [{:keys [field-name visibility-type special-type], :as field-definition} (:field-definitions table-definition)]
+        (let [field (delay (or (i/metabase-instance field-definition @table)
+                               (throw (Exception. (format "Field '%s' not loaded from definition:\n"
+                                                          field-name
+                                                          (u/pprint-to-str field-definition))))))]
+          (when visibility-type
+            (log/debug (format "SET VISIBILITY TYPE %s.%s -> %s" table-name field-name visibility-type))
+            (db/update! Field (:id @field) :visibility_type (name visibility-type)))
+          (when special-type
+            (log/debug (format "SET SPECIAL TYPE %s.%s -> %s" table-name field-name special-type))
+            (db/update! Field (:id @field) :special_type (u/keyword->qualified-name special-type))))))))
+
+(defn- create-database! [{:keys [database-name], :as database-definition} engine driver]
+  ;; Create the database
+  (i/create-db! driver database-definition)
+  ;; Add DB object to Metabase DB
+  (u/prog1 (db/insert! Database
+             :name    database-name
+             :engine  (name engine)
+             :details (i/database->connection-details driver :db database-definition))
+    ;; sync newly added DB
+    (sync-database/sync-database! <>)
+    ;; add extra metadata for fields
+    (add-extra-metadata! database-definition <>)))
+
 (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
+   DRIVER should be an object that implements `IDatasetLoader`; it defaults to the value returned by the method `driver` for the
    current dataset (`*driver*`), which is H2 by default."
-  ([^DatabaseDefinition database-definition]
+  ([database-definition]
    (get-or-create-database! *driver* database-definition))
-  ([dataset-loader {:keys [database-name], :as ^DatabaseDefinition database-definition}]
-   (let [engine (i/engine dataset-loader)]
+  ([driver database-definition]
+   (let [engine (i/engine driver)]
      (or (i/metabase-instance database-definition engine)
-         (do
-           ;; Create the database
-           (i/create-db! dataset-loader database-definition)
-
-           ;; Add DB object to Metabase DB
-           (let [db (db/insert! Database
-                      :name    database-name
-                      :engine  (name engine)
-                      :details (i/database->connection-details dataset-loader :db database-definition))]
-
-             ;; Sync the database
-             (sync-database/sync-database! db)
-
-             ;; Add extra metadata like Field base-type, etc.
-             (doseq [^TableDefinition table-definition (:table-definitions database-definition)]
-               (let [table-name (:table-name table-definition)
-                     table      (delay (or  (i/metabase-instance table-definition db)
-                                            (throw (Exception. (format "Table '%s' not loaded from definiton:\n%s\nFound:\n%s"
-                                                                       table-name
-                                                                       (u/pprint-to-str (dissoc table-definition :rows))
-                                                                       (u/pprint-to-str (db/select [Table :schema :name], :db_id (:id db))))))))]
-                 (doseq [{:keys [field-name visibility-type special-type], :as field-definition} (:field-definitions table-definition)]
-                   (let [field (delay (or (i/metabase-instance field-definition @table)
-                                          (throw (Exception. (format "Field '%s' not loaded from definition:\n"
-                                                                     field-name
-                                                                     (u/pprint-to-str field-definition))))))]
-                     (when visibility-type
-                       (log/debug (format "SET VISIBILITY TYPE %s.%s -> %s" table-name field-name visibility-type))
-                       (db/update! Field (:id @field) :visibility_type (name visibility-type)))
-                     (when special-type
-                       (log/debug (format "SET SPECIAL TYPE %s.%s -> %s" table-name field-name special-type))
-                       (db/update! Field (:id @field) :special_type (u/keyword->qualified-name special-type)))))))
-             db))))))
-
-(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 `*driver*`."
-  ([^DatabaseDefinition database-definition]
-   (remove-database! *driver* database-definition))
-  ([dataset-loader ^DatabaseDefinition database-definition]
-   ;; Delete the Metabase Database and associated objects
-   (db/cascade-delete! Database :id (:id (i/metabase-instance database-definition (i/engine dataset-loader))))
-
-   ;; now delete the DBMS database
-   (i/destroy-db! dataset-loader database-definition)))
-
-
-(def ^:private loader->loaded-db-def
-  (atom #{}))
-
-(defn destroy-loaded-temp-dbs!
-  "Destroy all temporary databases created by `with-temp-db`."
-  {:expectations-options :after-run}
-  []
-  (binding [db/*disable-db-logging* true]
-    (doseq [[loader dbdef] @loader->loaded-db-def]
-      (try
-        (remove-database! loader dbdef)
-        (catch Throwable e
-          (println "Error destroying database:" e)))))
-  (reset! loader->loaded-db-def #{}))
+         (create-database! database-definition engine driver)))))
 
 
 (defn do-with-temp-db
   "Execute F with DBDEF loaded as the current dataset. F takes a single argument, the `DatabaseInstance` that was loaded and synced from DBDEF."
   [^DatabaseDefinition dbdef, f]
-  (let [loader *driver*
+  (let [driver *driver*
         dbdef  (i/map->DatabaseDefinition dbdef)]
-    (swap! loader->loaded-db-def conj [loader dbdef])
     (binding [db/*disable-db-logging* true]
-      (let [db (get-or-create-database! loader dbdef)]
+      (let [db (get-or-create-database! driver dbdef)]
         (assert db)
-        (assert (db/exists? Database :id (:id db)))
+        (assert (db/exists? Database :id (u/get-id db)))
         (with-db db
           (f db))))))
 
 
 (defmacro with-temp-db
-  "Load and sync DATABASE-DEFINITION with DATASET-LOADER and execute BODY with the newly created `Database` bound to DB-BINDING,
+  "Load and sync DATABASE-DEFINITION with DRIVER and execute BODY with the newly created `Database` bound to DB-BINDING,
    and make it the current database for `metabase.test.data` functions like `id`.
 
      (with-temp-db [db tupac-sightings]
@@ -264,9 +237,8 @@
                                           :aggregation  [\"count\"]
                                           :filter       [\"<\" (:id &events.timestamp) \"1765-01-01\"]}}))
 
-   A given Database is only created once per run of the test suite, and is automatically destroyed at the conclusion of the suite.
-   (The created Database is added to `loader->loaded-db-def`, which can be destroyed with `destroy-loaded-temp-dbs!`, which is automatically ran at the end of the test suite.)"
-  [[db-binding ^DatabaseDefinition database-definition] & body]
+   A given Database is only created once per run of the test suite, and is automatically destroyed at the conclusion of the suite."
+  [[db-binding, ^DatabaseDefinition database-definition] & body]
   `(do-with-temp-db ~database-definition
      (fn [~db-binding]
        ~@body)))

diff --git a/test/metabase/test/data/bigquery.clj b/test/metabase/test/data/bigquery.clj
@@ -205,5 +205,4 @@
   (merge i/IDatasetLoaderDefaultsMixin
          {:engine                       (constantly :bigquery)
           :database->connection-details (u/drop-first-arg database->connection-details)
-          :create-db!                   (u/drop-first-arg create-db!)
-          :destroy-db!                  (constantly nil)}))
+          :create-db!                   (u/drop-first-arg create-db!)}))
diff --git a/test/metabase/test/data/crate.clj b/test/metabase/test/data/crate.clj
@@ -20,7 +20,7 @@
    :type/Time       "timestamp"})
 
 
-(defn- timestamp->CrateDateTime
+(defn- timestamp->crate-datetime
   [value]
   (cond
     (instance? java.sql.Timestamp value)    (.getTime ^java.sql.Timestamp value)
@@ -34,7 +34,7 @@
   (if (sequential? row-or-rows)
     (map escape-field-names row-or-rows)
     (into {} (for [[k v] row-or-rows]
-               {(sql/escape-field-name k) (timestamp->CrateDateTime v)}))))
+               {(sql/escape-field-name k) (timestamp->crate-datetime v)}))))
 
 (defn- make-load-data-fn
   "Create a `load-data!` function. This creates a function to actually insert a row or rows, wraps it with any WRAP-INSERT-FNS,
@@ -44,7 +44,7 @@
     (let [insert! ((apply comp wrap-insert-fns) (fn [row-or-rows]
                                                   (jdbc/insert-multi!
                                                     (generic/database->spec driver :db dbdef)
-                                                    (keyword (:table-name tabledef))
+                                                    (keyword (i/db-qualified-table-name (name (:database-name dbdef)) (name (:table-name tabledef))))
                                                     (escape-field-names row-or-rows)
                                                     {:transaction? false})))
           rows    (apply list (generic/load-data-get-rows driver dbdef tabledef))]
@@ -63,9 +63,10 @@
           :create-db-sql             (constantly nil)
           :add-fk-sql                (constantly nil)
           :drop-db-if-exists-sql     (constantly nil)
-          :load-data!                (make-load-data-fn generic/load-data-add-ids)})
+          :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
-         {:database->connection-details   database->connection-details
-          :engine                         (constantly :crate)
-          :default-schema                 (constantly "doc")}))
+         {:database->connection-details database->connection-details
+          :engine                       (constantly :crate)
+          :default-schema               (constantly "doc")}))
diff --git a/test/metabase/test/data/druid.clj b/test/metabase/test/data/druid.clj
@@ -21,8 +21,7 @@
   (merge i/IDatasetLoaderDefaultsMixin
          {:engine                       (constantly :druid)
           :database->connection-details database->connection-details
-          :create-db!                   (constantly nil)
-          :destroy-db!                  (constantly nil)}))
+          :create-db!                   (constantly nil)}))
 
 
 

diff --git a/test/metabase/test/data/generic_sql.clj b/test/metabase/test/data/generic_sql.clj
@@ -129,12 +129,9 @@
             (quot (pk-field-name driver)))))
 
 (defn- default-qualified-name-components
-  ([_ db-name]
-   [db-name])
-  ([_ db-name table-name]
-   [table-name])
-  ([_ db-name table-name field-name]
-   [table-name field-name]))
+  ([_ db-name]                       [db-name])
+  ([_ db-name table-name]            [table-name])
+  ([_ db-name table-name field-name] [table-name field-name]))
 
 (defn- default-quote-name [_ nm]
   (str \" nm \"))
@@ -321,14 +318,10 @@
   (doseq [tabledef table-definitions]
     (load-data! driver dbdef tabledef)))
 
-(defn- destroy-db! [driver dbdef]
-  (execute-sql! driver :server dbdef (drop-db-if-exists-sql driver dbdef)))
-
 (def IDatasetLoaderMixin
-  "Mixin for `IGenericSQLDatasetLoader` types to implemnt `create-db!` and `destroy-db!` from `IDatasetLoader`."
+  "Mixin for `IGenericSQLDatasetLoader` types to implement `create-db!` from `IDatasetLoader`."
   (merge i/IDatasetLoaderDefaultsMixin
-         {:create-db!  create-db!
-          :destroy-db! destroy-db!}))
+         {:create-db! create-db!}))
 
 
 ;;; ## Various Util Fns