Add with-temp-env-var-value and with-temp-file test util macros (#15921)

modules/drivers/vertica/test/metabase/test/data/vertica.clj

@@ -11,6 +11,7 @@
             [metabase.test.data.sql-jdbc :as sql-jdbc.tx]
             [metabase.test.data.sql-jdbc.execute :as execute]
             [metabase.test.data.sql-jdbc.load-data :as load-data]
+            [metabase.test :as mt]
             [metabase.util :as u]
             [metabase.util.files :as files]))
 
@@ -142,7 +143,7 @@
 (defmethod load-data/load-data! :vertica
   [driver dbdef {:keys [rows], :as tabledef}]
   (try
-    (let [filename (str (files/get-path (System/getProperty "java.io.tmpdir") "vertica-rows.csv"))]
+    (mt/with-temp-file [filename "vertica-rows.csv"]
       (dump-table-rows-to-csv! tabledef filename)
       (load-rows-from-csv! driver dbdef tabledef filename))
     (catch Throwable e

test/metabase/cmd/dump_to_h2_test.clj

@@ -16,33 +16,32 @@
             [metabase.test :as mt]
             [metabase.test.data.interface :as tx]
             [metabase.util.encryption-test :as eu]
-            [metabase.util.files :as u.files]
             [metabase.util.i18n.impl :as i18n.impl]
             [toucan.db :as db]))
 
 (deftest dump-deletes-target-db-files-tests
   ;; test fails when the application db is anything but H2 presently
   ;; TODO: make this test work with postgres / mysql / mariadb
-  (let [tmp-h2-db     (str (u.files/get-path (System/getProperty "java.io.tmpdir") "mbtest_dump.h2"))
-        tmp-h2-db-mv  (str tmp-h2-db ".mv.db")
-        file-contents {tmp-h2-db    "Not really an H2 DB"
-                       tmp-h2-db-mv "Not really another H2 DB"}]
-    ;; 1. Don't actually run the copy steps themselves
-    (with-redefs [copy/copy! (constantly nil)]
-      (try
-        (doseq [[filename contents] file-contents]
-          (spit filename contents))
-        (dump-to-h2/dump-to-h2! tmp-h2-db)
+  (mt/with-temp-file [tmp-h2-db "mbtest_dump.h2"]
+    (let [tmp-h2-db-mv  (str tmp-h2-db ".mv.db")
+          file-contents {tmp-h2-db    "Not really an H2 DB"
+                         tmp-h2-db-mv "Not really another H2 DB"}]
+      ;; 1. Don't actually run the copy steps themselves
+      (with-redefs [copy/copy! (constantly nil)]
+        (try
+          (doseq [[filename contents] file-contents]
+            (spit filename contents))
+          (dump-to-h2/dump-to-h2! tmp-h2-db)
 
-        (doseq [filename (keys file-contents)]
-          (testing (str filename " was deleted")
-            (is (false? (.exists (io/file filename))))))
+          (doseq [filename (keys file-contents)]
+            (testing (str filename " was deleted")
+              (is (false? (.exists (io/file filename))))))
 
-        (finally
-          (doseq [filename (keys file-contents)
-                  :let     [file (io/file filename)]]
-            (when (.exists file)
-              (io/delete-file file))))))))
+          (finally
+            (doseq [filename (keys file-contents)
+                    :let     [file (io/file filename)]]
+              (when (.exists file)
+                (io/delete-file file)))))))))
 
 (deftest cmd-dump-to-h2-returns-code-from-dump-test
   (with-redefs [dump-to-h2/dump-to-h2! #(throw "err")
@@ -67,43 +66,43 @@
 (deftest dump-to-h2-dump-plaintext-test
   (testing "dump-to-h2 --dump-plaintext"
     (let [h2-fixture-db-file  @cmd.test-util/fixture-db-file-path
-          h2-file-plaintext   (format "/tmp/out-%s.db" (mt/random-name))
-          h2-file-enc         (format "/tmp/out-%s.db" (mt/random-name))
-          h2-file-default-enc (format "/tmp/out-%s.db" (mt/random-name))
           db-name             (str "test_" (mt/random-name))]
-      (mt/test-drivers #{:h2 :postgres :mysql}
-        (with-redefs [i18n.impl/site-locale-from-setting-fn (atom (constantly false))]
-          (binding [setting/*disable-cache*    true
-                    mdb.connection/*db-type*   driver/*driver*
-                    mdb.connection/*jdbc-spec* (persistent-jdbcspec driver/*driver* db-name)
-                    db/*db-connection*         (persistent-jdbcspec driver/*driver* db-name)
-                    db/*quoting-style*         driver/*driver*]
-            (when-not (= driver/*driver* :h2)
-              (tx/create-db! driver/*driver* {:database-name db-name}))
-            (load-from-h2/load-from-h2! h2-fixture-db-file)
-            (eu/with-secret-key "89ulvIGoiYw6mNELuOoEZphQafnF/zYe+3vT+v70D1A="
-              (db/insert! Setting {:key "my-site-admin", :value "baz"})
-              (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"})
-              (dump-to-h2/dump-to-h2! h2-file-plaintext {:dump-plaintext? true})
-              (dump-to-h2/dump-to-h2! h2-file-enc {:dump-plaintext? false})
-              (dump-to-h2/dump-to-h2! h2-file-default-enc))
+      (mt/with-temp-file [h2-file-plaintext   (format "out-%s.db" (mt/random-name))
+                          h2-file-enc         (format "out-%s.db" (mt/random-name))
+                          h2-file-default-enc (format "out-%s.db" (mt/random-name))]
+        (mt/test-drivers #{:h2 :postgres :mysql}
+          (with-redefs [i18n.impl/site-locale-from-setting-fn (atom (constantly false))]
+            (binding [setting/*disable-cache*    true
+                      mdb.connection/*db-type*   driver/*driver*
+                      mdb.connection/*jdbc-spec* (persistent-jdbcspec driver/*driver* db-name)
+                      db/*db-connection*         (persistent-jdbcspec driver/*driver* db-name)
+                      db/*quoting-style*         driver/*driver*]
+              (when-not (= driver/*driver* :h2)
+                (tx/create-db! driver/*driver* {:database-name db-name}))
+              (load-from-h2/load-from-h2! h2-fixture-db-file)
+              (eu/with-secret-key "89ulvIGoiYw6mNELuOoEZphQafnF/zYe+3vT+v70D1A="
+                (db/insert! Setting {:key "my-site-admin", :value "baz"})
+                (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"})
+                (dump-to-h2/dump-to-h2! h2-file-plaintext {:dump-plaintext? true})
+                (dump-to-h2/dump-to-h2! h2-file-enc {:dump-plaintext? false})
+                (dump-to-h2/dump-to-h2! h2-file-default-enc))
 
-            (testing "decodes settings and dashboard.details"
-              (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-plaintext)]
-                (is (= "baz" (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';")))))
-                (is (= "{\"db\":\"/tmp/test.db\"}"
-                       (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;")))))))
+              (testing "decodes settings and dashboard.details"
+                (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-plaintext)]
+                  (is (= "baz" (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';")))))
+                  (is (= "{\"db\":\"/tmp/test.db\"}"
+                         (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;")))))))
 
-            (testing "when flag is set to false, encrypted settings and dashboard.details are still encrypted"
-              (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-enc)]
-                (is (not (= "baz"
-                            (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';"))))))
-                (is (not (= "{\"db\":\"/tmp/test.db\"}"
-                            (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;"))))))))
+              (testing "when flag is set to false, encrypted settings and dashboard.details are still encrypted"
+                (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-enc)]
+                  (is (not (= "baz"
+                              (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';"))))))
+                  (is (not (= "{\"db\":\"/tmp/test.db\"}"
+                              (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;"))))))))
 
-            (testing "defaults to not decrypting"
-              (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-default-enc)]
-                (is (not (= "baz"
-                            (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';"))))))
-                (is (not (= "{\"db\":\"/tmp/test.db\"}"
-                            (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;"))))))))))))))
+              (testing "defaults to not decrypting"
+                (jdbc/with-db-connection [target-conn (copy.h2/h2-jdbc-spec h2-file-default-enc)]
+                  (is (not (= "baz"
+                              (:value (first (jdbc/query target-conn "select value from SETTING where key='my-site-admin';"))))))
+                  (is (not (= "{\"db\":\"/tmp/test.db\"}"
+                              (:details (first (jdbc/query target-conn "select details from metabase_database where id=1;")))))))))))))))

test/metabase/cmd/test_util.clj

@@ -4,5 +4,5 @@
 (def fixture-db-file-path
   (delay
     (let [original-file "frontend/test/__runner__/test_db_fixture.db.mv.db"]
-      (files/copy-file! (files/get-path original-file) (files/get-path "/tmp/test_db_fixture.db.mv.db"))
-      "/tmp/test_db_fixture.db")))
+      (files/copy-file! (files/get-path original-file) (files/get-path (System/getProperty "java.io.tmpdir") "test_db_fixture.db.mv.db"))
+      (str (files/get-path (System/getProperty "java.io.tmpdir") "test_db_fixture.db")))))

test/metabase/models/login_history_test.clj

 (ns metabase.models.login-history-test
   (:require [clojure.string :as str]
             [clojure.test :refer :all]
-            [environ.core :as env]
             [java-time :as t]
             [metabase.models :refer [LoginHistory User]]
             [metabase.models.login-history :as login-history]
-            [metabase.models.setting.cache :as setting.cache]
             [metabase.server.request.util :as request.u]
             [metabase.test :as mt]
             [metabase.util.date-2 :as u.date]
@@ -81,17 +79,10 @@
 
   (testing "don't send email if the setting is disabled by setting MB_SEND_EMAIL_ON_FIRST_LOGIN_FROM_NEW_DEVICE=FALSE"
     (mt/with-temp User [{user-id :id, email :email, first-name :first_name}]
-      (try
-        (mt/with-fake-inbox
-          ;; can't use `mt/with-temporary-setting-values` here because it's a read-only setting
-          (with-redefs [env/env (assoc env/env :mb-send-email-on-first-login-from-new-device "FALSE")]
-            ;; flush the Setting cache so it picks up the env var value for the `send-email-on-first-login-from-new-device` setting
-            (setting.cache/restore-cache!)
-            (mt/with-temp* [LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]
-                            LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]]
-              (is (= {}
-                     @mt/inbox)))))
-        (finally
-          ;; flush the cache again so the original value of `send-email-on-first-login-from-new-device` gets
-          ;; restored
-          (setting.cache/restore-cache!))))))
+      (mt/with-fake-inbox
+        ;; can't use `mt/with-temporary-setting-values` here because it's a read-only setting
+        (mt/with-temp-env-var-value [mb-send-email-on-first-login-from-new-device "FALSE"]
+          (mt/with-temp* [LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]
+                          LoginHistory [_ {:user_id user-id, :device_id (str (java.util.UUID/randomUUID))}]]
+            (is (= {}
+                   @mt/inbox))))))))

test/metabase/public_settings_test.clj

 (ns metabase.public-settings-test
   (:require [clojure.test :refer :all]
-            [environ.core :as env]
             [metabase.models.setting :as setting]
             [metabase.public-settings :as public-settings]
             [metabase.test :as mt]
@@ -58,7 +57,7 @@
 
 (deftest site-url-settings-normalize
   (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)"
-    (with-redefs [env/env (assoc env/env :mb-site-url "localhost:3000/")]
+    (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"]
       (mt/with-temporary-setting-values [site-url nil]
         (is (= "localhost:3000/"
                (setting/get-string :site-url)))
@@ -69,7 +68,7 @@
   (testing (str "If `site-url` is set via an env var, and it's invalid, we should return `nil` rather than having the"
                 " whole instance break")
     (mt/suppress-output
-      (with-redefs [env/env (assoc env/env :mb-site-url "asd_12w31%$;")]
+      (mt/with-temp-env-var-value [mb-site-url "asd_12w31%$;"]
         (mt/with-temporary-setting-values [site-url nil]
           (is (= "asd_12w31%$;"
                  (setting/get-string :site-url)))

test/metabase/query_processor/reducible_test.clj

@@ -57,7 +57,7 @@
      :rff     print-rows-rff}))
 
 (deftest write-rows-to-file-test
-  (let [filename (str (u.files/get-path (System/getProperty "java.io.tmpdir") "out.txt"))]
+  (mt/with-temp-file [filename "out.txt"]
     (try
       (is (= 3
              (qp/process-query
@@ -71,7 +71,7 @@
              (str/split-lines (slurp filename))))
       (finally
         (u/ignore-exceptions
-          (.delete (io/file filename)))))))
+         (.delete (io/file filename)))))))
 
 (defn- maps-rff [metadata]
   (let [ks (mapv (comp keyword :name) (:cols metadata))]

test/metabase/test.clj

@@ -187,6 +187,8 @@
   with-non-admin-groups-no-root-collection-for-namespace-perms
   with-non-admin-groups-no-root-collection-perms
   with-scheduler
+  with-temp-env-var-value
+  with-temp-file
   with-temp-scheduler
   with-temp-vals-in-db
   with-temporary-setting-values]

test/metabase/test/util.clj

 (ns metabase.test.util
   "Helper functions and macros for writing unit tests."
   (:require [cheshire.core :as json]
+            [clojure.java.io :as io]
             [clojure.set :as set]
             [clojure.string :as str]
             [clojure.test :refer :all]
@@ -17,12 +18,15 @@
             [metabase.models.permissions :as perms]
             [metabase.models.permissions-group :as group]
             [metabase.models.setting :as setting]
+            [metabase.models.setting.cache :as setting.cache]
             [metabase.plugins.classloader :as classloader]
             [metabase.task :as task]
             [metabase.test.data :as data]
+            [metabase.test.fixtures :as fixtures]
             [metabase.test.initialize :as initialize]
             [metabase.test.util.log :as tu.log]
             [metabase.util :as u]
+            [metabase.util.files :as u.files]
             [potemkin :as p]
             [schema.core :as s]
             [toucan.db :as db]
@@ -35,6 +39,8 @@
 
 (comment tu.log/keep-me)
 
+(use-fixtures :once (fixtures/initialize :db))
+
 ;; these are imported because these functions originally lived in this namespace, and some tests might still be
 ;; referencing them from here. We can remove the imports once everyone is using `metabase.test` instead of using this
 ;; namespace directly.
@@ -297,7 +303,9 @@
   works much the same way as `binding`.
 
      (with-temporary-setting-values [google-auth-auto-create-accounts-domain \"metabase.com\"]
-       (google-auth-auto-create-accounts-domain)) -> \"metabase.com\""
+       (google-auth-auto-create-accounts-domain)) -> \"metabase.com\"
+
+  To temporarily override the value of *read-only* env-var based Settings, use `with-temp-env-var-value`."
   [[setting-k value & more :as bindings] & body]
   (assert (even? (count bindings)) "mismatched setting/value pairs: is each setting name followed by a value?")
   (if (empty? bindings)
@@ -825,3 +833,160 @@
   {:arglists '([rename-fn & body])}
   [rename-fn & body]
   `(do-with-env-keys-renamed-by ~rename-fn (fn [] ~@body)))
+
+(defn do-with-temp-file
+  "Impl for `with-temp-file` macro."
+  [filename f]
+  {:pre [(or (string? filename) (nil? filename))]}
+  (let [filename (if (string? filename)
+                   filename
+                   (random-name))
+        filename (str (u.files/get-path (System/getProperty "java.io.tmpdir") filename))]
+    ;; delete file if it already exists
+    (io/delete-file (io/file filename) :silently)
+    (try
+      (f filename)
+      (finally
+        (io/delete-file (io/file filename) :silently)))))
+
+(defmacro with-temp-file
+  "Execute `body` with newly created temporary file(s) in the system temporary directory. You may optionally specify the
+  `filename` (without directory components) to be created in the temp directory; if `filename` is nil, a random
+  filename will be used. The file will be deleted if it already exists, but will not be touched; use `spit` to load
+  something in to it.
+
+    ;; create a random temp filename. File is deleted if it already exists.
+    (with-temp-file [filename]
+      ...)
+
+    ;; get a temp filename ending in `parrot-list.txt`
+    (with-temp-file [filename \"parrot-list.txt\"]
+      ...)"
+  [[filename-binding filename-or-nil & more :as bindings] & body]
+  {:pre [(vector? bindings) (>= (count bindings) 1)]}
+  `(do-with-temp-file
+    ~filename-or-nil
+    (fn [~(vary-meta filename-binding assoc :tag `String)]
+      ~@(if (seq more)
+          [`(with-temp-file ~(vec more) ~@body)]
+          body))))
+
+(deftest with-temp-file-test
+  (testing "random filename"
+    (let [temp-filename (atom nil)]
+      (with-temp-file [filename]
+        (is (string? filename))
+        (is (not (.exists (io/file filename))))
+        (spit filename "wow")
+        (reset! temp-filename filename))
+      (testing "File should be deleted at end of macro form"
+        (is (not (.exists (io/file @temp-filename)))))))
+
+  (testing "explicit filename"
+    (with-temp-file [filename "parrot-list.txt"]
+      (is (string? filename))
+      (is (not (.exists (io/file filename))))
+      (is (str/ends-with? filename "parrot-list.txt"))
+      (spit filename "wow")
+      (testing "should delete existing file"
+        (with-temp-file [filename "parrot-list.txt"]
+          (is (not (.exists (io/file filename))))))))
+
+  (testing "multiple bindings"
+    (with-temp-file [filename nil, filename-2 "parrot-list.txt"]
+      (is (string? filename))
+      (is (string? filename-2))
+      (is (not (.exists (io/file filename))))
+      (is (not (.exists (io/file filename-2))))
+      (is (not (str/ends-with? filename "parrot-list.txt")))
+      (is (str/ends-with? filename-2 "parrot-list.txt"))))
+
+  (testing "should delete existing file"
+    (with-temp-file [filename "parrot-list.txt"]
+      (spit filename "wow")
+      (with-temp-file [filename "parrot-list.txt"]
+        (is (not (.exists (io/file filename)))))))
+
+  (testing "validation"
+    (are [form] (thrown?
+                 clojure.lang.Compiler$CompilerException
+                 (macroexpand form))
+      `(with-temp-file [])
+      `(with-temp-file (+ 1 2)))))
+
+(defn- ->lisp-case-keyword [s]
+  (-> (name s)
+      (str/replace #"_" "-")
+      str/lower-case
+      keyword))
+
+(defn do-with-temp-env-var-value
+  "Impl for `with-temp-env-var-value` macro."
+  [env-var-keyword value thunk]
+  (let [value (str value)]
+    (testing (colorize/blue (format "\nEnv var %s = %s\n" env-var-keyword (pr-str value)))
+      (try
+        ;; temporarily override the underlying environment variable value
+        (with-redefs [env/env (assoc env/env env-var-keyword value)]
+          ;; flush the Setting cache so it picks up the env var value for the Setting (if applicable)
+          (setting.cache/restore-cache!)
+          (thunk))
+        (finally
+          ;; flush the cache again so the original value of any env var Settings get restored
+          (setting.cache/restore-cache!))))))
+
+(defmacro with-temp-env-var-value
+  "Temporarily override the value of one or more environment variables and execute `body`. Resets the Setting cache so
+  any env var Settings will see the updated value, and resets the cache again at the conclusion of `body` so the
+  original values are restored.
+
+    (with-temp-env-var-value [mb-send-email-on-first-login-from-new-device \"FALSE\"]
+      ...)"
+  [[env-var value & more :as bindings] & body]
+  {:pre [(vector? bindings) (even? (count bindings))]}
+  `(do-with-temp-env-var-value
+    ~(->lisp-case-keyword env-var)
+    ~value
+    (fn [] ~@(if (seq more)
+               [`(with-temp-env-var-value ~(vec more) ~@body)]
+               body))))
+
+(setting/defsetting with-temp-env-var-value-test-setting
+  "Setting for the `with-temp-env-var-value-test` test."
+  :visibility :internal
+  :setter     :none
+  :default    "abc")
+
+(deftest with-temp-env-var-value-test
+  (is (= "abc"
+         (with-temp-env-var-value-test-setting)))
+  (with-temp-env-var-value [mb-with-temp-env-var-value-test-setting "def"]
+    (testing "env var value"
+      (is (= "def"
+             (env/env :mb-with-temp-env-var-value-test-setting))))
+    (testing "Setting value"
+      (is (= "def"
+             (with-temp-env-var-value-test-setting)))))
+  (testing "original value should be restored"
+    (testing "env var value"
+      (is (= nil
+             (env/env :mb-with-temp-env-var-value-test-setting))))
+    (testing "Setting value"
+      (is (= "abc"
+             (with-temp-env-var-value-test-setting)))))
+
+  (testing "override multiple env vars"
+    (with-temp-env-var-value [some-fake-env-var 123, "ANOTHER_FAKE_ENV_VAR" "def"]
+      (testing "Should convert values to strings"
+        (is (= "123"
+               (:some-fake-env-var env/env))))
+      (testing "should handle CAPITALS/SNAKE_CASE"
+        (is (= "def"
+               (:another-fake-env-var env/env))))))
+
+  (testing "validation"
+    (are [form] (thrown?
+                 clojure.lang.Compiler$CompilerException
+                 (macroexpand form))
+      (list `with-temp-env-var-value '[a])
+      (list `with-temp-env-var-value '[a b c]))))

test/metabase/util/encryption_test.clj

@@ -11,9 +11,13 @@
   ;; flush the Setting cache so unencrypted values have to be fetched from the DB again
   (initialize/initialize-if-needed! :db)
   (setting.cache/restore-cache!)
-  (with-redefs [encryption/default-secret-key (when (seq secret-key)
-                                                (encryption/secret-key->hash secret-key))]
-    (thunk)))
+  (try
+    (with-redefs [encryption/default-secret-key (when (seq secret-key)
+                                                  (encryption/secret-key->hash secret-key))]
+      (thunk))
+    (finally
+      ;; reset the cache again so nothing that happened during the test is persisted.
+      (setting.cache/restore-cache!))))
 
 (defmacro with-secret-key
   "Run `body` with the encryption secret key temporarily bound to `secret-key`. Useful for testing how functions behave