Skip to content
Snippets Groups Projects
Commit 05859d21 authored by Cam Saül's avatar Cam Saül Committed by GitHub
Browse files

Merge pull request #2909 from metabase/even-more-test-cleanup 

Even more test cleanup :shower:
parents e6c042e1 d468bf24
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/database.clj
+ 5
− 4
View file @ 05859d21
......@@ -63,9 +63,9 @@
(defendpoint POST "/"
"Add a new `Database`."
[:as {{:keys [name engine details is_full_sync]} :body}]
{name [Required NonEmptyString]
engine [Required DBEngine]
details [Required Dict]}
{name [Required NonEmptyString]
engine [Required DBEngine]
details [Required Dict]}
(check-superuser)
;; this function tries connecting over ssl and non-ssl to establish a connection
;; if it succeeds it returns the `details` that worked, otherwise it returns an error
......@@ -85,7 +85,8 @@
(if-not (false? (:valid details-or-error))
;; no error, proceed with creation
(let-500 [new-db (db/insert! Database, :name name, :engine engine, :details details-or-error, :is_full_sync is_full_sync)]
(events/publish-event :database-create new-db))
(events/publish-event :database-create new-db)
new-db)
;; failed to connect, return error
{:status 400
:body details-or-error})))
......
test/metabase/api/activity_test.clj
+ 40
− 39
View file @ 05859d21
......@@ -9,7 +9,7 @@
[view-log :refer [ViewLog]])
[metabase.test.data :refer :all]
[metabase.test.data.users :refer :all]
[metabase.test.util :refer [match-$ random-name expect-with-temp resolve-private-fns]]
[metabase.test.util :refer [match-$ expect-with-temp resolve-private-fns]]
[metabase.util :as u]))
;; GET /
......@@ -99,17 +99,28 @@
:table nil
:custom_id nil
:details $})]
(->> ((user->client :crowberto) :get 200 "activity")
(map #(dissoc % :timestamp))))
(for [activity ((user->client :crowberto) :get 200 "activity")]
(dissoc activity :timestamp)))
;; GET /recent_views
;;; GET /recent_views
; Things we are testing for:
; 1. ordering is sorted by most recent
; 2. results are filtered to current user
; 3. `:model_object` is hydrated in each result
; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
;; Things we are testing for:
;; 1. ordering is sorted by most recent
;; 2. results are filtered to current user
;; 3. `:model_object` is hydrated in each result
;; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
(defn- create-view! [user model model-id]
(db/insert! ViewLog
:user_id user
:model model
:model_id model-id
:timestamp (u/new-sql-timestamp))
;; we sleep a bit to ensure no events have the same timestamp
;; sadly, MySQL doesn't support milliseconds so we have to wait a second
;; otherwise our records are out of order and this test fails :(
(Thread/sleep 1000))
(expect-with-temp [Card [card1 {:name "rand-name"
:creator_id (user->id :crowberto)
......@@ -127,47 +138,37 @@
:display "table"
:dataset_query {}
:visualization_settings {}}]]
[{:cnt 1
:user_id (user->id :crowberto)
:model "card"
:model_id (:id card1)
[{:cnt 1
:user_id (user->id :crowberto)
:model "card"
:model_id (:id card1)
:model_object {:id (:id card1)
:name (:name card1)
:description (:description card1)
:display (name (:display card1))}}
{:cnt 1
:user_id (user->id :crowberto)
:model "dashboard"
:model_id (:id dash1)
{:cnt 1
:user_id (user->id :crowberto)
:model "dashboard"
:model_id (:id dash1)
:model_object {:id (:id dash1)
:name (:name dash1)
:description (:description dash1)}}
{:cnt 1
:user_id (user->id :crowberto)
:model "card"
:model_id (:id card2)
{:cnt 1
:user_id (user->id :crowberto)
:model "card"
:model_id (:id card2)
:model_object {:id (:id card2)
:name (:name card2)
:description (:description card2)
:display (name (:display card2))}}]
(let [create-view (fn [user model model-id]
(db/insert! ViewLog
:user_id user
:model model
:model_id model-id
:timestamp (u/new-sql-timestamp))
;; we sleep a bit to ensure no events have the same timestamp
;; sadly, MySQL doesn't support milliseconds so we have to wait a second
;; otherwise our records are out of order and this test fails :(
(Thread/sleep 1000))]
(do
(create-view (user->id :crowberto) "card" (:id card2))
(create-view (user->id :crowberto) "dashboard" (:id dash1))
(create-view (user->id :crowberto) "card" (:id card1))
(create-view (user->id :crowberto) "card" 36478)
(create-view (user->id :rasta) "card" (:id card1))
(->> ((user->client :crowberto) :get 200 "activity/recent_views")
(map #(dissoc % :max_ts))))))
(do
(create-view! (user->id :crowberto) "card" (:id card2))
(create-view! (user->id :crowberto) "dashboard" (:id dash1))
(create-view! (user->id :crowberto) "card" (:id card1))
(create-view! (user->id :crowberto) "card" 36478)
(create-view! (user->id :rasta) "card" (:id card1))
(for [recent-view ((user->client :crowberto) :get 200 "activity/recent_views")]
(dissoc recent-view :max_ts))))
;;; activities->referenced-objects, referenced-objects->existing-objects, add-model-exists-info
......
test/metabase/api/database_test.clj
+ 209
− 206
View file @ 05859d21
......@@ -13,22 +13,40 @@
;; HELPER FNS
(defn create-db
([db-name]
(create-db db-name true))
([db-name full-sync?]
((user->client :crowberto) :post 200 "database" {:engine :postgres
:name db-name
:details {:host "localhost"
:port 5432
:dbname "fakedb"
:user "cam"
:ssl false}
:is_full_sync full-sync?})))
(defn- create-db-via-api! [options]
((user->client :crowberto) :post 200 "database" (merge {:engine :postgres
:name (tu/random-name)
:details {:host "localhost"
:port 5432
:dbname "fakedb"
:user "cam"
:ssl false}
:is_full_sync true}
options)))
(defn- do-with-temp-db-created-via-api [db-options f]
(let [db (create-db-via-api! db-options)]
(assert (integer? (:id db)))
(try
(f db)
(finally
(db/cascade-delete! Database :id (:id db))))))
(defmacro ^:private expect-with-temp-db-created-via-api {:style/indent 1} [[binding & [options]] expected actual]
;; use `gensym` instead of auto gensym here so we can be sure it's a unique symbol every time. Otherwise since expectations hashes its body
;; to generate function names it will treat every usage this as the same test and only a single one will end up being ran
(let [result (gensym "result-")]
`(let [~result (delay (do-with-temp-db-created-via-api ~options (fn [~binding]
[~expected
~actual])))]
(expect
(u/ignore-exceptions (first @~result)) ; in case @result# barfs we don't want the test to succeed (Exception == Exception for expectations)
(second @~result)))))
(defn- db-details
([]
(db-details (db)))
(db-details (db)))
([db]
(match-$ db
{:created_at $
......@@ -43,23 +61,6 @@
:description nil
:features (mapv name (driver/features (driver/engine->driver (:engine db))))})))
(defn- table-details [table]
(match-$ table
{:description $
:entity_type $
:visibility_type $
:schema $
:name $
:display_name $
:rows $
:updated_at $
:entity_name $
:active $
:id $
:db_id $
:raw_table_id $
:created_at $}))
;; # DB LIFECYCLE ENDPOINTS
......@@ -76,140 +77,142 @@
;; ## POST /api/database
;; Check that we can create a Database
(let [db-name (random-name)]
(expect-eval-actual-first
(match-$ (Database :name db-name)
{:created_at $
:engine "postgres" ; string because it's coming back from API instead of DB
:id $
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
:updated_at $
:name db-name
:is_sample false
:is_full_sync false
:organization_id nil
:description nil
:features (mapv name (driver/features (driver/engine->driver :postgres)))})
(create-db db-name false)))
(expect-with-temp-db-created-via-api [db {:is_full_sync false}]
(match-$ db
{:created_at $
:engine :postgres
:id $
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
:updated_at $
:name $
:is_sample false
:is_full_sync false
:organization_id nil
:description nil
:features (into [] (driver/features (driver/engine->driver :postgres)))})
(Database (:id db)))
;; ## DELETE /api/database/:id
;; Check that we can delete a Database
(expect-let [db-name (random-name)
{db-id :id} (create-db db-name)
sel-db-name (fn [] (db/select-one-field :name Database, :id db-id))]
[db-name
nil]
[(sel-db-name)
(do ((user->client :crowberto) :delete 204 (format "database/%d" db-id))
(sel-db-name))])
(expect-with-temp-db-created-via-api [db]
false
(do ((user->client :crowberto) :delete 204 (format "database/%d" (:id db)))
(db/exists? 'Database :id (:id db))))
;; ## PUT /api/database/:id
;; Check that we can update fields in a Database
(expect-let [[old-name new-name] (repeatedly 2 random-name)
{db-id :id} (create-db old-name)
sel-db (fn [] (dissoc (into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id))
:features))]
[{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
:engine :postgres
:name old-name
:is_full_sync true}
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name new-name
:is_full_sync false}]
[(sel-db)
;; Check that we can update all the fields
(do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name new-name
:engine "h2"
:is_full_sync false
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
(sel-db))])
(expect-with-temp-db-created-via-api [{db-id :id}]
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name "Cam's Awesome Toucan Database"
:is_full_sync false}
(do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name "Cam's Awesome Toucan Database"
:engine "h2"
:is_full_sync false
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
(dissoc (into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id))
:features)))
(defn- table-details [table]
(match-$ table
{:description $
:entity_type $
:visibility_type $
:schema $
:name $
:display_name $
:rows $
:updated_at $
:entity_name $
:active $
:id $
:db_id $
:raw_table_id $
:created_at $}))
;; TODO - this is a test code smell, each test should clean up after itself and this step shouldn't be neccessary. One day we should be able to remove this!
;; If you're writing a test that needs this, fix your brain and your test
(defn- ^:deprecated delete-randomly-created-databases!
"Delete all the randomly created Databases we've made so far. Optionally specify one or more IDs to SKIP."
[& {:keys [skip]}]
(db/cascade-delete! Database :id [:not-in (into (set skip)
(for [engine datasets/all-valid-engines
:let [id (datasets/when-testing-engine engine
(:id (get-or-create-test-data-db! (driver/engine->driver engine))))]
:when id]
id))]))
;; # DATABASES FOR ORG
;; ## GET /api/database
;; Test that we can get all the DBs for an Org, ordered by name
;; Test that we can get all the DBs (ordered by name)
;; Database details *should not* come back for Rasta since she's not a superuser
(let [db-name (str "A" (random-name))] ; make sure this name comes before "test-data"
(expect-eval-actual-first
(set (filter identity (conj (for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(match-$ (get-or-create-test-data-db! (driver/engine->driver engine))
{:created_at $
:engine (name $engine)
:id $
:updated_at $
:name "test-data"
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:features (mapv name (driver/features (driver/engine->driver engine)))})))
;; (?) I don't remember why we have to do this for postgres but not any other of the bonus drivers
(match-$ (Database :name db-name)
(expect-with-temp-db-created-via-api [{db-id :id}]
(set (filter identity (conj (for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(match-$ (get-or-create-test-data-db! (driver/engine->driver engine))
{:created_at $
:engine "postgres"
:engine (name $engine)
:id $
:updated_at $
:name $
:name "test-data"
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:features (mapv name (driver/features (driver/engine->driver :postgres)))}))))
(do
;; Delete all the randomly created Databases we've made so far
(db/cascade-delete! Database :id [:not-in (filter identity
(for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(:id (get-or-create-test-data-db! (driver/engine->driver engine))))))])
;; Add an extra DB so we have something to fetch besides the Test DB
(create-db db-name)
;; Now hit the endpoint
(set ((user->client :rasta) :get 200 "database")))))
:features (map name (driver/features (driver/engine->driver engine)))})))
(match-$ (Database db-id)
{:created_at $
:engine "postgres"
:id $
:updated_at $
:name $
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:features (map name (driver/features (driver/engine->driver :postgres)))}))))
(do
(delete-randomly-created-databases! :skip [db-id])
(set ((user->client :rasta) :get 200 "database"))))
;; GET /api/databases (include tables)
(let [db-name (str "A" (random-name))] ; make sure this name comes before "test-data"
(expect-eval-actual-first
(set (concat [(match-$ (Database :name db-name)
{:created_at $
:engine "postgres"
:id $
:updated_at $
:name $
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:tables []
:features (mapv name (driver/features (driver/engine->driver :postgres)))})]
(filter identity (for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(let [database (get-or-create-test-data-db! (driver/engine->driver engine))]
(match-$ database
{:created_at $
:engine (name $engine)
:id $
:updated_at $
:name "test-data"
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:tables (->> (db/select Table, :db_id (:id database))
(mapv table-details)
(sort-by :name))
:features (mapv name (driver/features (driver/engine->driver engine)))})))))))
(do
;; Delete all the randomly created Databases we've made so far
(db/cascade-delete! Database :id [:not-in (set (filter identity
(for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(:id (get-or-create-test-data-db! (driver/engine->driver engine)))))))])
;; Add an extra DB so we have something to fetch besides the Test DB
(create-db db-name)
;; Now hit the endpoint
(set ((user->client :rasta) :get 200 "database" :include_tables true)))))
(expect-with-temp-db-created-via-api [{db-id :id}]
(set (cons (match-$ (Database db-id)
{:created_at $
:engine "postgres"
:id $
:updated_at $
:name $
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:tables []
:features (map name (driver/features (driver/engine->driver :postgres)))})
(filter identity (for [engine datasets/all-valid-engines]
(datasets/when-testing-engine engine
(let [database (get-or-create-test-data-db! (driver/engine->driver engine))]
(match-$ database
{:created_at $
:engine (name $engine)
:id $
:updated_at $
:name "test-data"
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
:tables (sort-by :name (for [table (db/select Table, :db_id (:id database))]
(table-details table)))
:features (map name (driver/features (driver/engine->driver engine)))})))))))
(do
(delete-randomly-created-databases! :skip [db-id])
(set ((user->client :rasta) :get 200 "database" :include_tables true))))
;; ## GET /api/meta/table/:id/query_metadata
;; TODO - add in example with Field :values
......@@ -293,65 +296,65 @@
;; ## GET /api/database/:id/tables
;; These should come back in alphabetical order
(expect
(let [db-id (id)]
[(match-$ (Table (id :categories))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "CATEGORIES"
:rows 75
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Categories"
:raw_table_id $})
(match-$ (Table (id :checkins))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "CHECKINS"
:rows 1000
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Checkins"
:raw_table_id $})
(match-$ (Table (id :users))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "USERS"
:rows 15
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Users"
:raw_table_id $})
(match-$ (Table (id :venues))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "VENUES"
:rows 100
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Venues"
:raw_table_id $})])
(let [db-id (id)]
[(match-$ (Table (id :categories))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "CATEGORIES"
:rows 75
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Categories"
:raw_table_id $})
(match-$ (Table (id :checkins))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "CHECKINS"
:rows 1000
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Checkins"
:raw_table_id $})
(match-$ (Table (id :users))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "USERS"
:rows 15
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Users"
:raw_table_id $})
(match-$ (Table (id :venues))
{:description nil
:entity_type nil
:visibility_type nil
:schema "PUBLIC"
:name "VENUES"
:rows 100
:updated_at $
:entity_name nil
:active true
:id $
:db_id db-id
:created_at $
:display_name "Venues"
:raw_table_id $})])
((user->client :rasta) :get 200 (format "database/%d/tables" (id))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment