Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Nov 14, 2023
  2. Nov 13, 2023
  3. Nov 12, 2023
  4. Nov 11, 2023
  5. Nov 10, 2023
    • Mark Bastian's avatar
      Fixing flaky tests related to `inner-query-name-collisions-test` (#32355) (#35612) · efea8d01
      Mark Bastian authored
      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.
      Unverified
      efea8d01
    • Jerry Huang's avatar
    • Ryan Laurie's avatar
    • Jeff Bruemmer's avatar
      links (#35603) · 65dc7663
      Jeff Bruemmer authored
      Unverified
      65dc7663
    • John Swanson's avatar
      Audit log CRUD permission failures (#35391) · e825ef2c
      John Swanson authored
      * Audit log CRUD permission failures
      
      When we run `metabase.api.common/*-check` functions like `update-check` or `create-check`, publish an event when the
      check fails, along with enough context that, on the other side, the audit log handler can record the relevant event.
      
      Note that we won't fire events for a `read-check` failure. Eventually we will and these events will be handled by the "read log" instead of the audit log.
      
      If we throw an event that isn't derived from `:metabase/event` it will throw an exception. That `derive` no longer belongs in the
      audit log code, and until we have the view log there's not really a logical place to put it. For now, I just commented
      out the `publish-event!` call, with the assumption that the view log is coming very soon. Otherwise we can just delete
      it and add it back in when it's needed.
      
      Some tangentially related required changes here to make this work include:
      
      * allow `audit-log/model-name` to work with RootCollections
      
      `(t2/model root-collection)` returns a Class rather than a keyword - `name` doesn't work here, so we'll use
      `getSimpleName` instead.
      
      * add a default model-details clause for dashboards
      
      If the event-type is not one that we've explicitly delineated, just return an empty map for the model details.
      
      * Migration to fit longer model names in `activity.model`
      
      The `NativeQuerySnippet` or `PermissionsGroupMembership` models, for example, were too long to fit in `VARCHAR(16)`.
      I'm doubling the length of this column to fit the longest model I see, with a little extra room.
      Unverified
      e825ef2c
    • Uladzimir Havenchyk's avatar
    • Nemanja Glumac's avatar
      Further optimize static-viz check for the backend workflow (#35580) · aa2c4ec1
      Nemanja Glumac authored
      Follow up after #34209
      Unverified
      aa2c4ec1
    • Noah Moss's avatar
      Speed up audit DB tests (#35522) · 6845d6a9
      Noah Moss authored
      
      Co-authored-by: default avatarbryan <bryan.maass@gmail.com>
      Unverified
      6845d6a9
    • Nemanja Glumac's avatar
      Refine default CI triggers (#35617) · 77f0b766
      Nemanja Glumac authored
      Unverified
      77f0b766
    • Ryan Laurie's avatar
      Give cljs tests 15 min (#35608) · e66d80d9
      Ryan Laurie authored
      Unverified
      e66d80d9
    • Jesse Devaney's avatar
    • John Swanson's avatar
      Skip `:event/{card,dashboard}-update` on pin/unpin (#35066) · 263800c1
      John Swanson authored
      When a card or dashboard is pinned, the entity itself is not changing, so let's not send the `:event/card-update` or
      `:event/dashboard-update` events in these cases.
      Unverified
      263800c1
Loading