Test fixes & extra tests [ci drivers]

src/metabase/query_processor/middleware/binning.clj

@@ -64,14 +64,14 @@
   (u/round-to-decimals 5 (/ (- max-value min-value)
                             num-bins)))
 
-(s/defn ^:private calculate-num-bins :- s/Num
+(s/defn ^:private calculate-num-bins :- su/IntGreaterThanZero
   "Calculate number of bins of width `bin-width` required to cover interval [`min-value`, `max-value`]."
   [min-value :- s/Num, max-value :- s/Num, bin-width :- (s/constrained s/Num (complement neg?) "number >= 0")]
   (long (Math/ceil (/ (- max-value min-value)
                       bin-width))))
 
 (s/defn ^:private resolve-default-strategy :- [(s/one (s/enum :bin-width :num-bins) "strategy")
-                                               (s/one {:bin-width s/Num, :num-bins s/Num} "opts")]
+                                               (s/one {:bin-width s/Num, :num-bins su/IntGreaterThanZero} "opts")]
   "Determine the approprate strategy & options to use when `:default` strategy was specified."
   [metadata :- {:special_type su/FieldType, s/Any s/Any}, min-value :- s/Num, max-value :- s/Num]
   (if (isa? (:special_type metadata) :type/Coordinate)
@@ -147,6 +147,21 @@
 
 ;;; -------------------------------------------- Adding resolved options ---------------------------------------------
 
+(defn- resolve-options [strategy strategy-param metadata min-value max-value]
+  (case strategy
+    :num-bins
+    [:num-bins
+     {:num-bins  strategy-param
+      :bin-width (calculate-bin-width min-value max-value strategy-param)}]
+
+    :bin-width
+    [:bin-width
+     {:bin-width strategy-param
+      :num-bins  (calculate-num-bins min-value max-value strategy-param)}]
+
+    :default
+    (resolve-default-strategy metadata min-value max-value)))
+
 (s/defn ^:private update-binned-field :- mbql.s/binning-strategy
   "Given a `binning-strategy` clause, resolve the binning strategy (either provided or found if default is specified)
   and calculate the number of bins and bin width for this field. `field-id->filters` contains related criteria that
@@ -163,22 +178,10 @@
          :as   min-max}                 (extract-bounds (when (integer? field-id-or-name) field-id-or-name)
                                                         (:fingerprint metadata)
                                                         field-id->filters)
-        [new-strategy resolved-options] (case strategy
-                                          :num-bins
-                                          [:num-bins
-                                           {:num-bins  strategy-param
-                                            :bin-width (calculate-bin-width min-value max-value strategy-param)}]
-
-                                          :bin-width
-                                          [:bin-width
-                                           {:bin-width strategy-param
-                                            :num-bins  (calculate-num-bins min-value max-value strategy-param)}]
-
-                                          :default
-                                          (resolve-default-strategy metadata min-value max-value))
+        [new-strategy resolved-options] (resolve-options strategy strategy-param metadata min-value max-value)
         resolved-options (merge min-max resolved-options)]
     ;; Bail out and use unmodifed version if we can't converge on a nice version.
-    [:binning-strategy field-clause new-strategy strategy-param (or (nicer-breakout strategy resolved-options)
+    [:binning-strategy field-clause new-strategy strategy-param (or (nicer-breakout new-strategy resolved-options)
                                                                     resolved-options)]))
 
 

test/metabase/query_processor/middleware/binning_test.clj

 (ns metabase.query-processor.middleware.binning-test
   (:require [expectations :refer [expect]]
-            [metabase.models.field :as field]
-            [metabase.query-processor.middleware.binning :as binning]))
+            [metabase.models.field :as field :refer [Field]]
+            [metabase.query-processor.middleware.binning :as binning]
+            [metabase.query-processor.store :as qp.store]
+            [metabase.test.data :as data]
+            [metabase.util :as u]
+            [toucan.util.test :as tt]))
 
 (expect
   {}
@@ -84,3 +88,44 @@
                          [:num-bins  {:min-value 9,   :max-value 1002, :num-bins 8, :bin-width 0}]
                          [:bin-width {:min-value 9,   :max-value 1002, :num-bins 1, :bin-width 15.0}]]]
     (#'binning/nicer-breakout strategy opts)))
+
+;; does `resolve-default-strategy` work the way we'd expect?
+(expect
+  [:num-bins {:num-bins 8, :bin-width 28.28321}]
+  (#'binning/resolve-default-strategy {:special_type :type/Income} 12.061602936923117 238.32732001721533))
+
+;; does `nicer-breakout` make things NICER?
+(expect
+  {:min-value 0.0, :max-value 240.0, :num-bins 8, :bin-width 30}
+  (#'binning/nicer-breakout :num-bins {:min-value 12.061602936923117
+                                       :max-value 238.32732001721533
+                                       :bin-width 28.28321
+                                       :num-bins  8}))
+
+;; Try an end-to-end test of the middleware
+(tt/expect-with-temp [Field [field (field/map->FieldInstance
+                                    {:database_type "DOUBLE"
+                                     :table_id      (data/id :checkins)
+                                     :special_type  :type/Income
+                                     :name          "TOTAL"
+                                     :display_name  "Total"
+                                     :fingerprint   {:global {:distinct-count 10000}
+                                                     :type   {:type/Number {:min 12.061602936923117
+                                                                            :max 238.32732001721533
+                                                                            :avg 82.96014815230829}}}
+                                     :base_type     :type/Float})]]
+  {:query    {:source-table (data/id :checkins)
+              :breakout     [[:binning-strategy
+                              [:field-id (u/get-id field)]
+                              :num-bins
+                              nil
+                              {:min-value 0.0, :max-value 240.0, :num-bins 8, :bin-width 30}]]}
+   :type     :query
+   :database (data/id)}
+  (qp.store/with-store
+    (qp.store/store-field! field)
+    ((binning/update-binning-strategy identity)
+     {:query    {:source-table (data/id :checkins)
+                 :breakout     [[:binning-strategy [:field-id (u/get-id field)] :default]]}
+      :type     :query
+      :database (data/id)})))