Test for sample database having sync scheduled #41752
Conversation
| @@ -269,26 +270,6 @@ | |||
|
|
|||
| (def ^:private monthly-schedule {:schedule_type "monthly" :schedule_day "fri" :schedule_frame "last"}) | |||
|
|
|||
| (defn- all-db-sync-triggers-name | |||
There was a problem hiding this comment.
I moved these functions to metabase.task.sync-databases-test because they were being used in a few places. And since they mostly depend on that namespace it felt natural to put them there.
| (let [db (t2/instance :model/Database db)] | ||
| (assert (some? (#'task/scheduler)) "makes sure the scheduler is initialized!") | ||
| (->> (for [task-info @#'task.sync-databases/all-tasks] | ||
| (keep #(when (= (.getName ^TriggerKey (#'task.sync-databases/trigger-key db task-info)) (:key %)) |
There was a problem hiding this comment.
You can lift the left hand side of the comparison out of the loop into a :let in the for expression. I also think map-filter is more elegant than this keep construction, it'll let you just use a set as the predicate and avoid repeating the key getter.
| flatten | ||
| set))) |
There was a problem hiding this comment.
This is nice and clear but I try to avoid flatten given how complex a function it is. If this was application code I would probably use (reduce into #{}) instead.
|
Like this reorganization and the crisp test 🍏 |
|
@crisptrutski your comments are on code that I'm moving, not code that I'm changing. All I did was copy+paste from one namespace to another. Following the principle that code refactoring like this is pretty low-value and we don't need to clean up code that we move, I'm going to ignore them for now. I'll accept the grammar suggestions though, because they're no brainers |
Grammar fixes Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>
|
@calherries Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Follow-up to #40066
This PR adds a test for the sample database getting its sync scheduled as if it were a normal database that was added after the initial metabase setup.
Previously this didn't really need test coverage because the sample database was being inserted using
t2/insert!, and the sync tasks were scheduled in theafter-inserthooks. Now we're inserting the sample database in a migration without usingt2/insert!, we should test this specifically. The jobs are being scheduled because we're usingt2/update!to update the sample database details outside of the migration, but that seems like a pretty shaky guarantee and could be changed in the future, so this PR adds the test coverage.