Fix failing tests [ci all]

modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj

@@ -160,9 +160,9 @@
        "ORDER BY `name` ASC")
   ;; normally for test purposes BigQuery doesn't support foreign keys so override the function that checks that and
   ;; make it return `true` so this test proceeds as expected
-  (with-redefs [driver/supports?                (constantly true)]
-    (tu/with-temp-vals-in-db 'Field (data/id :venues :category_id) {:fk_target_field_id (data/id :categories :id)
-                                                                    :special_type       "type/FK"}
+  (with-redefs [driver/supports? (constantly true)]
+    (tu/with-temp-vals-in-db Field (data/id :venues :category_id) {:fk_target_field_id (data/id :categories :id)
+                                                                   :special_type       "type/FK"}
       (let [results (qp/process-query
                      {:database (data/id)
                       :type     "query"

modules/drivers/oracle/test/metabase/driver/oracle_test.clj

 (ns metabase.driver.oracle-test
   "Tests for specific behavior of the Oracle driver."
   (:require [clojure.java.jdbc :as jdbc]
-            [expectations :refer :all]
+            [expectations :refer [expect]]
             [metabase
              [driver :as driver]
              [query-processor :as qp]

src/metabase/api/embed.clj

@@ -357,6 +357,7 @@
   [token dashcard-id card-id & query-params]
   (card-for-signed-token token dashcard-id card-id query-params ))
 
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                        FieldValues, Search, Remappings                                         |
 ;;; +----------------------------------------------------------------------------------------------------------------+

src/metabase/driver.clj

@@ -123,7 +123,7 @@
           (when-not (registered? driver)
             (throw (Exception. (str (tru "Driver not registered after loading: {0}" driver))))))))))
 
-(defn the-driver
+(s/defn the-driver
   "Like Clojure core `the-ns`. Converts argument to a keyword, then loads and registers the driver if not already done,
   throwing an Exception if it fails or is invalid. Returns keyword.
 
@@ -140,7 +140,7 @@
 
     (the-driver :postgres) ; -> :postgres
     (the-driver :baby)     ; -> Exception"
-  [driver]
+  [driver :- (s/cond-pre s/Str s/Keyword)]
   (let [driver (keyword driver)]
     (load-driver-namespace-if-needed driver)
     driver))

src/metabase/driver/sql_jdbc/execute.clj

@@ -147,16 +147,20 @@
 ;;; |                                                Running Queries                                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+;; TODO - this is pretty similar to what `jdbc/with-db-connection` does, but not exactly the same. See if we can
+;; switch to using that instead?
+(defn- do-with-ensured-connection [db f]
+  (if-let [conn (jdbc/db-find-connection db)]
+    (f conn)
+    (with-open [conn (jdbc/get-connection db)]
+      (f conn))))
+
 (defmacro ^:private with-ensured-connection
   "In many of the clojure.java.jdbc functions, it checks to see if there's already a connection open before opening a
   new one. This macro checks to see if one is open, or will open a new one. Will bind the connection to `conn-sym`."
   {:style/indent 1}
-  [[conn-sym db] & body]
-  `(let [db# ~db]
-     (if-let [~conn-sym (jdbc/db-find-connection db#)]
-       (do ~@body)
-       (with-open [~conn-sym (jdbc/get-connection db#)]
-         ~@body))))
+  [[conn-binding db] & body]
+  `(do-with-ensured-connection ~db (fn [~conn-binding] ~@body)))
 
 (defn- cancelable-run-query
   "Runs `sql` in such a way that it can be interrupted via a `future-cancel`"
@@ -206,10 +210,11 @@
   and rethrowing the exception as an Exception with a nicely formatted error message."
   {:style/indent 0}
   [f]
-  (try (f)
-       (catch SQLException e
-         (log/error (jdbc/print-sql-exception-chain e))
-         (throw (Exception. (exception->nice-error-message e))))))
+  (try
+    (f)
+    (catch SQLException e
+      (log/error (jdbc/print-sql-exception-chain e))
+      (throw (Exception. (exception->nice-error-message e) e)))))
 
 (defn- do-with-auto-commit-disabled
   "Disable auto-commit for this transaction, and make the transaction `rollback-only`, which means when the
@@ -222,8 +227,9 @@
   (.setAutoCommit (jdbc/get-connection conn) false)
   ;; TODO - it would be nice if we could also `.setReadOnly` on the transaction as well, but that breaks setting the
   ;; timezone. Is there some way we can have our cake and eat it too?
-  (try (f)
-       (finally (.rollback (jdbc/get-connection conn)))))
+  (try
+    (f)
+    (finally (.rollback (jdbc/get-connection conn)))))
 
 (defn- do-in-transaction [connection f]
   (jdbc/with-db-transaction [transaction-connection connection]

test/metabase/api/permissions_test.clj

@@ -18,7 +18,10 @@
 (expect
   #{{:id (u/get-id (group/all-users)), :name "All Users",      :member_count 3}
     {:id (u/get-id (group/admin)),     :name "Administrators", :member_count 1}}
-  (fetch-groups))
+  ;; make sure test users are created first, otherwise we're possibly going to have some WEIRD results
+  (do
+    (test-users/create-users-if-needed!)
+    (fetch-groups)))
 
 ;; The endpoint should however return empty groups!
 (tt/expect-with-temp [PermissionsGroup [group]]
@@ -34,9 +37,11 @@
   #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (test-users/user->id :crowberto), :membership_id true}
     {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (test-users/user->id :lucky),     :membership_id true}
     {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (test-users/user->id :rasta),     :membership_id true}}
-  (set
-   (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
-     (update member :membership_id some?))))
+  (do
+    (test-users/create-users-if-needed!)
+    (set
+     (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
+       (update member :membership_id some?)))))
 
 
 ;; make sure we can update the perms graph from the API
@@ -46,6 +51,7 @@
   (data/id :users)      :none
   (data/id :venues)     :all}
  (tt/with-temp PermissionsGroup [group]
+   (test-users/create-users-if-needed!)
    ((test-users/user->client :crowberto) :put 200 "permissions/graph"
     (assoc-in (perms/graph)
               [:groups (u/get-id group) (data/id) :schemas]

test/metabase/api/setup_test.clj

 (ns metabase.api.setup-test
   "Tests for /api/setup endpoints."
-  (:require [expectations :refer :all]
+  (:require [expectations :refer [expect]]
             [metabase
              [http-client :as http]
              [public-settings :as public-settings]
              [setup :as setup]]
             [metabase.models.user :refer [User]]
+            [metabase.test.data.users :as test-users]
             [metabase.test.util :as tu]
             [toucan.db :as db]))
 
@@ -13,20 +14,25 @@
 ;; Check that we can create a new superuser via setup-token
 (let [email (tu/random-email)]
   (expect
-    [true
-     email]
+    {:uuid? true, :admin-email email}
     (try
+      ;; make sure the default test users are created before running this test, otherwise we're going to run into
+      ;; issues if it attempts to delete this user and it is the only admin test user
+      (test-users/create-users-if-needed!)
       (tu/with-temporary-setting-values [admin-email nil]
-        [(tu/is-uuid-string? (:id (http/client :post 200 "setup" {:token (setup/create-token!)
+        {:uuid?
+         (tu/is-uuid-string? (:id (http/client :post 200 "setup" {:token (setup/create-token!)
                                                                   :prefs {:site_name "Metabase Test"}
                                                                   :user  {:first_name (tu/random-name)
                                                                           :last_name  (tu/random-name)
                                                                           :email      email
                                                                           :password   "anythingUP12!!"}})))
+
+         :admin-email
          (do
            ;; reset our setup token
            (setup/create-token!)
-           (public-settings/admin-email))])
+           (public-settings/admin-email))})
       (finally
         (db/delete! User :email email)))))
 

test/metabase/test/util.clj

@@ -305,7 +305,8 @@
   [model object-or-id column->temp-value f]
   ;; use low-level `query` and `execute` functions here, because Toucan `select` and `update` functions tend to do
   ;; things like add columns like `common_name` that don't actually exist, causing subsequent update to fail
-  (let [[original-column->value] (db/query {:select (keys column->temp-value)
+  (let [model                    (db/resolve-model model)
+        [original-column->value] (db/query {:select (keys column->temp-value)
                                             :from   [model]
                                             :where  [:= :id (u/get-id object-or-id)]})]
     (try