Merge pull request #2891 from metabase/more-test-cleanup

More test cleanup 

test/metabase/api/activity_test.clj

@@ -9,7 +9,7 @@
                              [view-log :refer [ViewLog]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name expect-with-temp]]
+            [metabase.test.util :refer [match-$ random-name expect-with-temp]]
             [metabase.util :as u]))
 
 ;; GET /

test/metabase/api/card_test.clj

@@ -13,7 +13,7 @@
                              [view-log :refer [ViewLog]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp with-temp* obj->json->obj expect-with-temp]]
+            [metabase.test.util :refer [match-$ random-name with-temp with-temp* obj->json->obj expect-with-temp]]
             [metabase.util :as u]))
 
 ;; # CARD LIFECYCLE

test/metabase/api/database_test.clj

@@ -8,7 +8,7 @@
             [metabase.test.data :refer :all]
             (metabase.test.data [datasets :as datasets]
                                 [users :refer :all])
-            [metabase.test.util :refer [match-$ random-name expect-eval-actual-first]]
+            [metabase.test.util :refer [match-$ random-name expect-eval-actual-first], :as tu]
             [metabase.util :as u]))
 
 ;; HELPER FNS

test/metabase/api/pulse_test.clj

@@ -8,7 +8,7 @@
                              [common :as common]
                              [database :refer [Database]]
                              [pulse :refer [Pulse create-pulse] :as pulse])
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp]]
+            [metabase.test.util :refer [match-$ random-name with-temp]]
             [metabase.test.data.users :refer :all]
             [metabase.test.data :refer :all]
             [metabase.util :as u]))

test/metabase/api/revision_test.clj

@@ -8,7 +8,7 @@
                              [revision :refer [Revision push-revision! revert! revisions]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [expect-eval-actual-first random-name expect-with-temp with-temp]]))
+            [metabase.test.util :refer [expect-with-temp with-temp]]))
 
 (def ^:private rasta-revision-info
   (delay {:id (user->id :rasta) :common_name "Rasta Toucan", :first_name "Rasta", :last_name "Toucan"}))

test/metabase/api/util_test.clj

 (ns metabase.api.util-test
   "Tests for /api/util"
   (:require [expectations :refer :all]
-            [metabase.http-client :refer :all]
-            (metabase.models [session :refer [Session]]
-                             [user :refer [User]])
-            [metabase.test.data :refer :all]
-            [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [random-name expect-eval-actual-first]]))
+            [metabase.http-client :refer [client]]))
 
 
 ;; ## POST /api/util/password_check

test/metabase/driver/mongo_test.clj

@@ -8,8 +8,7 @@
                              [table :refer [Table] :as table])
             [metabase.query-processor :as qp]
             [metabase.test.data :as data]
-            [metabase.test.data.datasets :as datasets]
-            [metabase.test.util :refer [expect-eval-actual-first resolve-private-fns]])
+            [metabase.test.data.datasets :as datasets])
   (:import metabase.driver.mongo.MongoDriver))
 
 ;; ## Logic for selectively running mongo

test/metabase/events/last_login_test.clj

 (ns metabase.events.last-login-test
   (:require [expectations :refer :all]
-            [metabase.db :as db]
-            [metabase.events.last-login :refer :all]
-            (metabase.models [user :refer [User]])
-            [metabase.test.data :refer :all]
-            [metabase.test.util :refer [expect-eval-actual-first random-name]]
-            [metabase.test-setup :refer :all]))
-
-
-(defn- create-test-user []
-  (let [rand-name (random-name)]
-    (db/insert! User
-      :email      (str rand-name "@metabase.com")
-      :first_name rand-name
-      :last_name  rand-name
-      :password   rand-name)))
+            [metabase.events.last-login :refer [process-last-login-event]]
+            [metabase.models.user :refer [User]]
+            [metabase.test.util :as tu]))
 
 
 ;; `:user-login` event
-(expect-let [{user-id :id last-login :last_login} (create-test-user)]
+(expect
   {:orig-last-login nil
    :upd-last-login  false}
-  (do
+  (tu/with-temp User [{user-id :id, last-login :last_login}]
     (process-last-login-event {:topic :user-login
                                :item  {:user_id    user-id
                                        :session_id "doesntmatter"}})

test/metabase/events/revision_test.clj

 (ns metabase.events.revision-test
   (:require [expectations :refer :all]
             [metabase.db :as db]
-            [metabase.events.revision :refer :all]
+            [metabase.events.revision :refer [process-revision-event]]
             (metabase.models [card :refer [Card]]
                              [dashboard :refer [Dashboard]]
                              [dashboard-card :refer [DashboardCard]]
@@ -12,171 +12,172 @@
                              [table :refer [Table]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [expect-eval-actual-first random-name]]
-            [metabase.test-setup :refer :all]
             [metabase.test.util :as tu]
             [metabase.util :as u]))
 
-(defn- create-test-card []
-  (let [rand-name (random-name)]
-    (db/insert! Card
-      :name                   rand-name
-      :description            rand-name
-      :public_perms           2
-      :display                "table"
-      :dataset_query          {:database (id)
-                               :type     "query"
-                               :query    {:aggregation ["rows"]
-                                          :source_table (id :categories)}}
-      :visualization_settings {}
-      :creator_id             (user->id :crowberto))))
+(defn- card-properties
+  "Some default properties for `Cards` for use in tests in this namespace."
+  []
+  {:public_perms           2
+   :display                "table"
+   :dataset_query          {:database (id)
+                            :type     "query"
+                            :query    {:aggregation ["rows"]
+                                       :source_table (id :categories)}}
+   :visualization_settings {}
+   :creator_id             (user->id :crowberto)})
 
-(defn- test-card-object [card]
-  {:description            (:name card)
+(defn- card->revision-object [card]
+  {:description            nil
    :table_id               (id :categories)
    :database_id            (id)
    :organization_id        nil
    :query_type             "query"
    :name                   (:name card)
-   :creator_id             (user->id :crowberto)
+   :creator_id             (:creator_id card)
    :dataset_query          (:dataset_query card)
    :id                     (:id card)
    :display                "table"
    :visualization_settings {}
-   :public_perms           2
+   :public_perms           (:public_perms card)
    :archived               false})
 
-(defn- create-test-dashboard []
-  (let [rand-name (random-name)]
-    (db/insert! Dashboard
-      :name                   rand-name
-      :description            rand-name
-      :public_perms           2
-      :creator_id             (user->id :crowberto))))
-
-(defn- test-dashboard-object [dashboard]
-  {:description (:name dashboard),
-   :name (:name dashboard),
-   :public_perms 2})
+(defn- dashboard->revision-object [dashboard]
+  {:description  nil
+   :name         (:name dashboard)
+   :public_perms (:public_perms dashboard)})
 
 
 ;; :card-create
-(expect-let [{card-id :id :as card} (create-test-card)]
+(tu/expect-with-temp [Card [{card-id :id, :as card} (card-properties)]]
   {:model        "Card"
    :model_id     card-id
    :user_id      (user->id :crowberto)
-   :object       (test-card-object card)
+   :object       (card->revision-object card)
    :is_reversion false
    :is_creation  true}
   (do
     (process-revision-event {:topic :card-create
                              :item  card})
-    (-> (Revision :model "Card", :model_id card-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model       "Card"
+      :model_id    card-id)))
+
 
 ;; :card-update
-(expect-let [{card-id :id :as card} (create-test-card)]
+(tu/expect-with-temp [Card [{card-id :id, :as card} (card-properties)]]
   {:model        "Card"
    :model_id     card-id
    :user_id      (user->id :crowberto)
-   :object       (test-card-object card)
+   :object       (card->revision-object card)
    :is_reversion false
    :is_creation  false}
   (do
     (process-revision-event {:topic :card-update
                              :item  card})
-    (-> (Revision :model "Card", :model_id card-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model       "Card"
+      :model_id    card-id)))
+
 
 ;; :dashboard-create
-(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)]
+(tu/expect-with-temp [Dashboard [{dashboard-id :id, :as dashboard}]]
   {:model        "Dashboard"
    :model_id     dashboard-id
-   :user_id      (user->id :crowberto)
-   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :user_id      (user->id :rasta)
+   :object       (assoc (dashboard->revision-object dashboard) :cards [])
    :is_reversion false
    :is_creation  true}
   (do
     (process-revision-event {:topic :dashboard-create
                              :item  dashboard})
-    (-> (Revision :model "Dashboard", :model_id dashboard-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model    "Dashboard"
+      :model_id dashboard-id)))
+
 
 ;; :dashboard-update
-(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)]
+(tu/expect-with-temp [Dashboard [{dashboard-id :id, :as dashboard}]]
   {:model        "Dashboard"
    :model_id     dashboard-id
-   :user_id      (user->id :crowberto)
-   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :user_id      (user->id :rasta)
+   :object       (assoc (dashboard->revision-object dashboard) :cards [])
    :is_reversion false
    :is_creation  false}
   (do
     (process-revision-event {:topic :dashboard-update
                              :item  dashboard})
-    (-> (Revision :model "Dashboard", :model_id dashboard-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model    "Dashboard"
+      :model_id dashboard-id)))
+
 
 ;; :dashboard-add-cards
-(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
-             {card-id :id}                    (create-test-card)
-             dashcard                         (db/insert! DashboardCard :card_id card-id :dashboard_id dashboard-id)]
+(tu/expect-with-temp [Dashboard     [{dashboard-id :id, :as dashboard}]
+                      Card          [{card-id :id}                     (card-properties)]
+                      DashboardCard [dashcard                          {:card_id card-id, :dashboard_id dashboard-id}]]
   {:model        "Dashboard"
    :model_id     dashboard-id
-   :user_id      (user->id :crowberto)
-   :object       (assoc (test-dashboard-object dashboard) :cards [(assoc (select-keys dashcard [:id :card_id :sizeX :sizeY :row :col]) :series [])])
+   :user_id      (user->id :rasta)
+   :object       (assoc (dashboard->revision-object dashboard) :cards [(assoc (select-keys dashcard [:id :card_id :sizeX :sizeY :row :col]) :series [])])
    :is_reversion false
    :is_creation  false}
   (do
     (process-revision-event {:topic :dashboard-add-cards
-                             :item  {:id       dashboard-id
-                                     :actor_id (user->id :crowberto)
+                             :item  {:id        dashboard-id
+                                     :actor_id  (user->id :rasta)
                                      :dashcards [dashcard]}})
-    (-> (Revision :model "Dashboard", :model_id dashboard-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model    "Dashboard"
+      :model_id dashboard-id)))
+
 
 ;; :dashboard-remove-cards
-(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
-             {card-id :id}                    (create-test-card)
-             dashcard                         (db/insert! DashboardCard :card_id card-id :dashboard_id dashboard-id)
-             _                                (db/delete! DashboardCard, :id (:id dashcard))]
+(tu/expect-with-temp [Dashboard     [{dashboard-id :id, :as dashboard}]
+                      Card          [{card-id :id}                     (card-properties)]
+                      DashboardCard [dashcard                          {:card_id card-id, :dashboard_id dashboard-id}]]
   {:model        "Dashboard"
    :model_id     dashboard-id
-   :user_id      (user->id :crowberto)
-   :object       (assoc (test-dashboard-object dashboard) :cards [])
+   :user_id      (user->id :rasta)
+   :object       (assoc (dashboard->revision-object dashboard) :cards [])
    :is_reversion false
    :is_creation  false}
   (do
+    (db/delete! DashboardCard, :id (:id dashcard))
     (process-revision-event {:topic :dashboard-remove-cards
                              :item  {:id       dashboard-id
-                                     :actor_id (user->id :crowberto)
+                                     :actor_id (user->id :rasta)
                                      :dashcards [dashcard]}})
-    (-> (Revision :model "Dashboard", :model_id dashboard-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model    "Dashboard"
+      :model_id dashboard-id)))
+
 
 ;; :dashboard-reposition-cards
-(expect-let [{dashboard-id :id :as dashboard} (create-test-dashboard)
-             {card-id :id}                    (create-test-card)
-             dashcard                         (u/prog1 (db/insert! DashboardCard :card_id card-id :dashboard_id dashboard-id)
-                                                (db/update! DashboardCard (:id <>), :sizeX 4))]
+(tu/expect-with-temp [Dashboard     [{dashboard-id :id, :as dashboard}]
+                      Card          [{card-id :id}                     (card-properties)]
+                      DashboardCard [dashcard                          {:card_id card-id, :dashboard_id dashboard-id}]]
   {:model        "Dashboard"
    :model_id     dashboard-id
    :user_id      (user->id :crowberto)
-   :object       (assoc (test-dashboard-object dashboard) :cards [{:id      (:id dashcard)
-                                                                   :card_id card-id
-                                                                   :sizeX   4
-                                                                   :sizeY   2
-                                                                   :row     nil
-                                                                   :col     nil
-                                                                   :series  []}])
+   :object       (assoc (dashboard->revision-object dashboard) :cards [{:id      (:id dashcard)
+                                                                        :card_id card-id
+                                                                        :sizeX   4
+                                                                        :sizeY   2
+                                                                        :row     nil
+                                                                        :col     nil
+                                                                        :series  []}])
    :is_reversion false
    :is_creation  false}
   (do
+    (db/update! DashboardCard (:id dashcard), :sizeX 4)
     (process-revision-event {:topic :dashboard-reeposition-cards
-                             :item  {:id       dashboard-id
-                                     :actor_id (user->id :crowberto)
+                             :item  {:id        dashboard-id
+                                     :actor_id  (user->id :crowberto)
                                      :dashcards [(assoc dashcard :sizeX 4)]}})
-    (-> (Revision :model "Dashboard", :model_id dashboard-id)
-        (select-keys [:model :model_id :user_id :object :is_reversion :is_creation]))))
+    (db/select-one [Revision :model :model_id :user_id :object :is_reversion :is_creation]
+      :model    "Dashboard"
+      :model_id dashboard-id)))
 
 
 ;; :metric-create
@@ -200,6 +201,7 @@
     (let [revision (db/select-one [Revision :model :user_id :object :is_reversion :is_creation :message], :model "Metric", :model_id (:id metric))]
       (assoc revision :object (dissoc (:object revision) :id :table_id)))))
 
+
 ;; :metric-update
 (expect
   {:model        "Metric"
@@ -222,6 +224,7 @@
     (let [revision (db/select-one [Revision :model :user_id :object :is_reversion :is_creation :message], :model "Metric", :model_id (:id metric))]
       (assoc revision :object (dissoc (:object revision) :id :table_id)))))
 
+
 ;; :metric-delete
 (expect
   {:model        "Metric"

test/metabase/test/util.clj

@@ -65,7 +65,7 @@
 ;; By default `expect` evaluates EXPECTED first. This isn't always what we want; for example, sometime API tests affect the DB
 ;; and we'd like to check the results.
 
-(defmacro -doexpect [e a]
+(defmacro ^:deprecated -doexpect [e a]
   `(let [a# (try ~a (catch java.lang.Throwable t# t#))
          e# (try ~e (catch java.lang.Throwable t# t#))]
      (report
@@ -73,8 +73,9 @@
            (catch java.lang.Throwable e2#
              (compare-expr e2# a# '~e '~a))))))
 
-(defmacro expect-eval-actual-first
-  "Identical to `expect` but evaluates `actual` first (instead of evaluating `expected` first)."
+(defmacro ^:deprecated expect-eval-actual-first
+  "Identical to `expect` but evaluates `actual` first (instead of evaluating `expected` first).
+   DEPRECATED: You shouldn't need to use this when writing new tests. `expect-with-temp` should be used instead, as it handles cleaning up after itself."
   {:style/indent 0}
   [expected actual]
   (let [fn-name (gensym)]

test/metabase/test_setup.clj

@@ -22,6 +22,11 @@
 ;; Don't run unit tests whenever JVM shuts down
 (expectations/disable-run-on-shutdown)
 
+;; For good editors (i.e. Emacs) mark `expect-let` as `^:deprecated` so it will show up as such when editing code, discouraging future use
+;; TODO - this can be removed once we upgrade to a newer version of expectations that removes this
+(u/ignore-exceptions
+  (alter-meta! (resolve 'expectations/expect-let) assoc :deprecated true))
+
 
 ;; ## EXPECTATIONS FORMATTING OVERRIDES