Skip to content
Snippets Groups Projects
Unverified Commit efea8d01 authored by Mark Bastian's avatar Mark Bastian Committed by GitHub
Browse files

Fixing flaky tests related to `inner-query-name-collisions-test` (#32355) (#35612)

It appears the code fails when run in parallel due to the `metabot-settings/enum-cardinality-threshold` value.

To demonstrate, I ran:

```clojure
(frequencies
  (pmap
  (fn [_]
    (clojure.test/run-test inner-query-name-collisions-test))
  (range 1000)))
```

and got:

```
{{:test 1, :pass 12, :fail 0, :error 0, :type :summary} 414, {:test 1, :pass 11, :fail 1, :error 0, :type :summary} 586}
```

Running with a simple sequential map produced no errors.

Wrapping the second test in `tu/with-temporary-setting-values [metabot-settings/enum-cardinality-threshold 10]`
reduced the error count dramatically (in the teens for 1000 samples), but still wasn't bulletproof.
I'm guessing the two `testing` directives in the test are run in parallel and the setting is not thread safe.

By adding the explicit threshold to the test and breaking it out into a standalone test the
error count drops dramatically.

```
(frequencies
  (pmap
    (fn [_]
      (clojure.test/run-test inner-query-name-collisions-with-joins-test))
    (range 1000)))
;=> {{:test 1, :pass 6, :fail 0, :error 0, :type :summary} 998, {:test 1, :pass 5, :fail 1, :error 0, :type :summary} 2}
```

I'm still not sure why this fails (only twice in 1000 runs), but apparently something about
this operation is not thread safe. However, given the additional isolation added by this PR
we go from a 60ish percent failure rate down to a 0.2% failure rate locally.

Hopefully this is adequate enough to prevent any substantial level of future flakes.
parent f2823e4c
No related branches found
No related tags found
No related merge requests found
......@@ -369,33 +369,36 @@
{:keys [column_aliases create_table_ddl]} (metabot-util/denormalize-model orders-model)]
(is (= 9 (count (re-seq #"ABC(?:_\d+)?" column_aliases))))
;; Ensure that the same aliases are used in the create table ddl
(is (= 9 (count (re-seq #"ABC" create_table_ddl)))))))))
(is (= 9 (count (re-seq #"ABC" create_table_ddl))))))))))
(deftest inner-query-name-collisions-with-joins-test
(testing "Models with name collisions across joins are also correctly disambiguated"
(mt/dataset sample-dataset
(t2.with-temp/with-temp
[Card model {:dataset true
:database_id (mt/id)
:query_type :query
:dataset_query
(mt/mbql-query orders
{:fields [$total &products.products.category &self.products.category]
:joins [{:source-table $$products
:condition [:= $product_id &products.products.id]
:strategy :left-join
:alias "products"}
{:source-table $$products
:condition [:= $id &self.products.id]
:strategy :left-join
:alias "self"}]})}]
(let [model (update model :result_metadata
(fn [v]
(map #(assoc % :display_name "FOO") v)))
{:keys [column_aliases create_table_ddl]} (metabot-util/denormalize-model model)]
(is (= "\"TOTAL\" AS FOO, \"products__CATEGORY\" AS FOO_2, \"self__CATEGORY\" AS FOO_3"
column_aliases))
;; Ensure that the same aliases are used in the create table ddl
;; 7 = 3 for the column names + 2 for the type creation + 2 for the type references
(is (= 7 (count (re-seq #"FOO" create_table_ddl)))))))))
(tu/with-temporary-setting-values [metabot-settings/enum-cardinality-threshold 10]
(t2.with-temp/with-temp
[Card model {:dataset true
:database_id (mt/id)
:query_type :query
:dataset_query
(mt/mbql-query orders
{:fields [$total &products.products.category &self.products.category]
:joins [{:source-table $$products
:condition [:= $product_id &products.products.id]
:strategy :left-join
:alias "products"}
{:source-table $$products
:condition [:= $id &self.products.id]
:strategy :left-join
:alias "self"}]})}]
(let [model (update model :result_metadata
(fn [v]
(map #(assoc % :display_name "FOO") v)))
{:keys [column_aliases create_table_ddl]} (metabot-util/denormalize-model model)]
(is (= "\"TOTAL\" AS FOO, \"products__CATEGORY\" AS FOO_2, \"self__CATEGORY\" AS FOO_3"
column_aliases))
;; Ensure that the same aliases are used in the create table ddl
;; 7 = 3 for the column names + 2 for the type creation + 2 for the type references
(is (= 7 (count (re-seq #"FOO" create_table_ddl))))))))))
(deftest ^:parallel deconflicting-aliases-test
(testing "Test sql_name generation deconfliction:
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment