Fix slow back button x-ray flakes (#35387)

* Attempting to fix x-ray back timeout issues with smaller card count

It looks like the flakes on https://www.deploysentinel.com/ci/analysis?tab=flake for "dashboard back navigation should display a back to the dashboard button in table x-ray dashboards" and "dashboard back navigation should display a back to the dashboard button in model x-ray dashboards" frequently fail at the `cy.wait("@dataset")` stage. I'm guessing this is because the dashboard code is so slow. This PR limits the number of cards produce to `MAX_CARDS` (5) in order to speed up card generation and help with the flakes.

* Adding a timeout to slow x-rays for back button. Hopefully, the small number of max cards makes this unnecessary, though.

* Bumped timeout to 15s instead of 5s. Looks like it was already 5. Again, hopefully the change to minimized the number of generated cards will fix this flake and not take as long.

* Fixing the `show` query param for x-rays

`show` in `core` can be either `:all` or an integer. However, the API layer only exposes the `:all` or `nil` options. This PR exposes the integer path as well.

Note that this is incremental to get flakes working faster. I'll still need to add API tests for this.

* Adding API unit tests to `metabase.api.automagic-dashboards-test` for `show` cases.

* Fixed deftest name to be unique.

* Fixing reference to new card in flaky test

e2e/test/scenarios/dashboard/dashboard-back-navigation.cy.spec.js

@@ -33,6 +33,8 @@ import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
 const { ORDERS_ID } = SAMPLE_DATABASE;
 const PG_DB_ID = 2;
 const PERMISSION_ERROR = "Sorry, you don't have permission to see this card.";
+const MAX_CARDS = 5;
+const MAX_XRAY_WAIT_TIMEOUT = 15000;
 
 describe("scenarios > dashboard > dashboard back navigation", () => {
   beforeEach(() => {
@@ -103,9 +105,9 @@ describe("scenarios > dashboard > dashboard back navigation", () => {
     "should display a back to the dashboard button in table x-ray dashboards",
     { tags: "@slow" },
     () => {
-      const cardTitle = "Sales per state";
-      cy.visit(`/auto/dashboard/table/${ORDERS_ID}`);
-      cy.wait("@dataset");
+      const cardTitle = "Total transactions";
+      cy.visit(`/auto/dashboard/table/${ORDERS_ID}?#show=${MAX_CARDS}`);
+      cy.wait("@dataset", { timeout: MAX_XRAY_WAIT_TIMEOUT });
 
       getDashboardCards()
         .filter(`:contains("${cardTitle}")`)
@@ -127,8 +129,10 @@ describe("scenarios > dashboard > dashboard back navigation", () => {
     () => {
       const cardTitle = "Orders by Subtotal";
       cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
-      cy.visit(`/auto/dashboard/model/${ORDERS_QUESTION_ID}`);
-      cy.wait("@dataset");
+      cy.visit(
+        `/auto/dashboard/model/${ORDERS_QUESTION_ID}?#show=${MAX_CARDS}`,
+      );
+      cy.wait("@dataset", { timeout: MAX_XRAY_WAIT_TIMEOUT });
 
       getDashboardCards()
         .filter(`:contains("${cardTitle}")`)

src/metabase/api/automagic_dashboards.clj

@@ -31,7 +31,7 @@
 
 (def ^:private Show
   (mu/with-api-error-message
-    [:maybe [:enum "all"]]
+    [:maybe [:or [:enum "all"] nat-int?]]
     (deferred-tru "invalid show value")))
 
 (def ^:private Prefix
@@ -143,17 +143,22 @@
     [:enum "segment" "adhoc" "table"]
     (deferred-tru "Invalid comparison entity type. Can only be one of \"table\", \"segment\", or \"adhoc\"")))
 
+(defn- coerce-show
+  "Show is either nil, \"all\", or a number. If it's a string it needs to be converted into a keyword."
+  [show]
+  (cond-> show (= "all" show) keyword))
+
 (api/defendpoint GET "/:entity/:entity-id-or-query"
   "Return an automagic dashboard for entity `entity` with id `id`."
   [entity entity-id-or-query show]
-  {show   [:maybe [:= "all"]]
+  {show   [:maybe [:or [:= "all"] nat-int?]]
    entity (mu/with-api-error-message
             (into [:enum] entities)
             (deferred-tru "Invalid entity type"))}
   (if (= entity "transform")
     (transform.dashboard/dashboard (->entity entity entity-id-or-query))
     (-> (->entity entity entity-id-or-query)
-        (automagic-analysis {:show (keyword show)}))))
+        (automagic-analysis {:show (coerce-show show)}))))
 
 (defn linked-entities
   "Identify the pk field of the model with `pk_ref`, and then find any fks that have that pk as a target."
@@ -270,7 +275,7 @@
    prefix Prefix
    dashboard-template   DashboardTemplate}
   (-> (->entity entity entity-id-or-query)
-      (automagic-analysis {:show (keyword show)
+      (automagic-analysis {:show (coerce-show show)
                            :dashboard-template ["table" prefix dashboard-template]})))
 
 (api/defendpoint GET "/:entity/:entity-id-or-query/cell/:cell-query"
@@ -282,7 +287,7 @@
    show       Show
    cell-query Base64EncodedJSON}
   (-> (->entity entity entity-id-or-query)
-      (automagic-analysis {:show       (keyword show)
+      (automagic-analysis {:show       (coerce-show show)
                            :cell-query (decode-base64-json cell-query)})))
 
 (api/defendpoint GET "/:entity/:entity-id-or-query/cell/:cell-query/rule/:prefix/:dashboard-template"
@@ -295,7 +300,7 @@
    dashboard-template       DashboardTemplate
    cell-query Base64EncodedJSON}
   (-> (->entity entity entity-id-or-query)
-      (automagic-analysis {:show       (keyword show)
+      (automagic-analysis {:show       (coerce-show show)
                            :dashboard-template       ["table" prefix dashboard-template]
                            :cell-query (decode-base64-json cell-query)})))
 
@@ -308,7 +313,7 @@
    comparison-entity ComparisonEntity}
   (let [left      (->entity entity entity-id-or-query)
         right     (->entity comparison-entity comparison-entity-id-or-query)
-        dashboard (automagic-analysis left {:show         (keyword show)
+        dashboard (automagic-analysis left {:show         (coerce-show show)
                                             :query-filter nil
                                             :comparison?  true})]
     (comparison-dashboard dashboard left right {})))
@@ -324,7 +329,7 @@
    comparison-entity ComparisonEntity}
   (let [left      (->entity entity entity-id-or-query)
         right     (->entity comparison-entity comparison-entity-id-or-query)
-        dashboard (automagic-analysis left {:show         (keyword show)
+        dashboard (automagic-analysis left {:show         (coerce-show show)
                                             :dashboard-template         ["table" prefix dashboard-template]
                                             :query-filter nil
                                             :comparison?  true})]
@@ -341,7 +346,7 @@
    comparison-entity ComparisonEntity}
   (let [left      (->entity entity entity-id-or-query)
         right     (->entity comparison-entity comparison-entity-id-or-query)
-        dashboard (automagic-analysis left {:show         (keyword show)
+        dashboard (automagic-analysis left {:show         (coerce-show show)
                                             :query-filter nil
                                             :comparison?  true})]
     (comparison-dashboard dashboard left right {:left {:cell-query (decode-base64-json cell-query)}})))
@@ -359,7 +364,7 @@
    comparison-entity ComparisonEntity}
   (let [left      (->entity entity entity-id-or-query)
         right     (->entity comparison-entity comparison-entity-id-or-query)
-        dashboard (automagic-analysis left {:show         (keyword show)
+        dashboard (automagic-analysis left {:show         (coerce-show show)
                                             :dashboard-template         ["table" prefix dashboard-template]
                                             :query-filter nil})]
     (comparison-dashboard dashboard left right {:left {:cell-query (decode-base64-json cell-query)}})))

test/metabase/api/automagic_dashboards_test.clj

@@ -65,7 +65,6 @@
                         (perms/grant-permissions! (perms-group/all-users) (perms/data-perms-path (mt/id))))))
            result))))))
 
-
 ;;; ------------------- X-ray  -------------------
 
 (deftest table-xray-test
@@ -481,3 +480,87 @@
                                                 :model-index       model-index
                                                 :model-index-value model-index-value})]
               (cards-have-filters? (:dashcards dash) pk-filters))))))))
+
+;; ------------------------------------------------ `show` limit test  -------------------------------------------------
+;; Historically, the used params are `nil` and "all", so this tests the integer case.
+
+(defn- card-count-check
+  "Create a dashboard via API twice, once with a limit and once without, and return the results."
+  [limit template args]
+  (mt/with-test-user :rasta
+    (with-dashboard-cleanup
+      (let [api-endpoint  (apply format (str "automagic-dashboards/" template) args)
+            resp          (mt/user-http-request :rasta :get 200 api-endpoint)
+            slimmed       (mt/user-http-request :rasta :get 200 api-endpoint :show limit)
+            card-count-fn (fn [dashboard] (count (keep :card (:dashcards dashboard))))]
+        {:base-count (card-count-fn resp)
+         :show-count (card-count-fn slimmed)}))))
+
+(deftest table-show-param-test
+  (testing "x-ray of a table with show set reduces the number of returned cards"
+    (let [show-limit 1
+          {:keys [base-count show-count]} (card-count-check show-limit "table/%s" [(mt/id :venues)])]
+      (testing "The non-slimmed dashboard isn't already at \"limit\" cards"
+        (is (< show-count base-count)))
+      (testing "Only \"limit\" cards are produced"
+        (is (= show-limit show-count))))))
+
+(deftest metric-xray-show-param-test
+  (testing "x-ray of a metric with show set reduces the number of returned cards"
+    (t2.with-temp/with-temp [Metric {metric-id :id} {:table_id   (mt/id :venues)
+                                                     :definition {:query {:aggregation ["count"]}}}]
+      (let [show-limit 1
+            {:keys [base-count show-count]} (card-count-check show-limit "metric/%s" [metric-id])]
+        (testing "The non-slimmed dashboard isn't already at \"limit\" cards"
+          (is (< show-count base-count)))
+        (testing "Only \"limit\" cards are produced"
+          (is (= show-limit show-count)))))))
+
+(deftest segment-xray-show-param-test
+  (testing "x-ray of a segment with show set reduces the number of returned cards"
+    (t2.with-temp/with-temp [Segment {segment-id :id} {:table_id   (mt/id :venues)
+                                                       :definition {:filter [:> [:field (mt/id :venues :price) nil] 10]}}]
+      (let [show-limit 1
+            {:keys [base-count show-count]} (card-count-check show-limit "segment/%s" [segment-id])]
+        (testing "The non-slimmed dashboard isn't already at \"limit\" cards"
+          (is (< show-count base-count)))
+        (testing "Only \"limit\" cards are produced"
+          (is (= show-limit show-count)))))))
+
+(deftest field-xray-show-param-test
+  (testing "x-ray of a field with show set reduces the number of returned cards"
+    (let [show-limit 1
+          {:keys [base-count show-count]} (card-count-check show-limit "field/%s" [(mt/id :venues :price)])]
+      (testing "The non-slimmed dashboard isn't already at \"limit\" cards"
+        (is (< show-count base-count)))
+      (testing "Only \"limit\" cards are produced"
+        (is (= show-limit show-count))))))
+
+(deftest cell-query-xray-show-param-test
+  (testing "x-ray of a cell-query with show set reduces the number of returned cards"
+    (t2.with-temp/with-temp [Card {card-id :id} {:table_id      (mt/id :venues)
+                                                 :dataset_query (mt/mbql-query venues
+                                                                  {:filter [:> $price 10]})}]
+      (let [cell-query (magic.util/encode-base64-json [:> [:field (mt/id :venues :price) nil] 5])
+            show-limit 2
+            {:keys [base-count show-count]} (card-count-check show-limit "question/%s/cell/%s" [card-id cell-query])]
+        (testing "The non-slimmed dashboard isn't already at \"limit\" cards"
+          (is (< show-count base-count)))
+        (testing "Only \"limit\" cards are produced"
+          (is (= show-limit show-count)))))))
+
+(deftest comparison-xray-show-param-test
+  (testing "x-ray of a comparison with show set reduces the number of returned cards"
+    (t2.with-temp/with-temp [Segment {segment-id :id} @segment]
+      (let [show-limit 1
+            {:keys [base-count show-count]} (card-count-check show-limit
+                                                              "adhoc/%s/cell/%s/compare/segment/%s"
+                                                              [(->> (mt/mbql-query venues
+                                                                      {:filter [:> $price 10]})
+                                                                    (magic.util/encode-base64-json))
+                                                               (->> [:= [:field (mt/id :venues :price) nil] 15]
+                                                                    (magic.util/encode-base64-json))
+                                                               segment-id])]
+        (testing "The slimmed dashboard produces less than the base dashboard"
+          ;;NOTE - Comparisons produce multiple dashboards and merge the results, so you don't get exactly `show-limit` cards
+          (is (< show-count base-count)))))))