diff --git a/modules/drivers/vertica/test/metabase/test/data/vertica.clj b/modules/drivers/vertica/test/metabase/test/data/vertica.clj index 30ccc7d456a3f1b6a804f9a416db6a8748e98dd5..b4875ec06958721152aa6226f76d232b74edd49e 100644 --- a/modules/drivers/vertica/test/metabase/test/data/vertica.clj +++ b/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 diff --git a/test/metabase/cmd/dump_to_h2_test.clj b/test/metabase/cmd/dump_to_h2_test.clj index eb045ca1061eba2590a4fde29bdba64a88b3634a..5a24bffb902da1637132bf175143d97b3b3ce791 100644 --- a/test/metabase/cmd/dump_to_h2_test.clj +++ b/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;"))))))))))))))) diff --git a/test/metabase/cmd/test_util.clj b/test/metabase/cmd/test_util.clj index 3f1807f1e36066bd962a4a035882ebbf05846fb6..1719560f5ab4bfe9913969aa679c4df22f849193 100644 --- a/test/metabase/cmd/test_util.clj +++ b/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"))))) diff --git a/test/metabase/models/login_history_test.clj b/test/metabase/models/login_history_test.clj index 6ffb194f767a319cb0a933e32ac55b8dd9ea6cf4..0eb97f231f4eb62894417ad6738d32cd5d0adb3c 100644 --- a/test/metabase/models/login_history_test.clj +++ b/test/metabase/models/login_history_test.clj @@ -1,11 +1,9 @@ (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)))))))) diff --git a/test/metabase/public_settings_test.clj b/test/metabase/public_settings_test.clj index d473f614cd9d350d621c2e1fabdae44552a75f98..dfb7f077a10bd8b9662c3c5670446fcdedd5f698 100644 --- a/test/metabase/public_settings_test.clj +++ b/test/metabase/public_settings_test.clj @@ -1,6 +1,5 @@ (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))) diff --git a/test/metabase/query_processor/reducible_test.clj b/test/metabase/query_processor/reducible_test.clj index 1c14c95a087f61e5547c8d4b26e41ce0ae85bbf4..26f1b179a9304a06d15dcc4029cebc4801d81623 100644 --- a/test/metabase/query_processor/reducible_test.clj +++ b/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))] diff --git a/test/metabase/test.clj b/test/metabase/test.clj index e672de086c25b3bd581c0bfb4dde17bf47f9c02c..21aed259b4337450612d8cfcec089bb9e6fe4f83 100644 --- a/test/metabase/test.clj +++ b/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] diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj index 6fff0264cd70bd65ab77c68ddcf91c70b38e6bc5..a86848ac9c990e31c997e6fac48d70759f7724f8 100644 --- a/test/metabase/test/util.clj +++ b/test/metabase/test/util.clj @@ -1,6 +1,7 @@ (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])))) diff --git a/test/metabase/util/encryption_test.clj b/test/metabase/util/encryption_test.clj index a40940c390adc71368f8eee00eb9542abc45e6c8..675f365393498f6393214aec55fea0c5a96c08ae 100644 --- a/test/metabase/util/encryption_test.clj +++ b/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