hourly task runner runs at the top of the hour

src/metabase/task.clj

@@ -98,19 +98,40 @@
   "Tasks to run nightly at midnight (according to the system calendar).
    Functions will be passed no arguments.")
 
-(def ^:private hourly-task-delay
-  "Number of milliseconds to wait before running hourly tasks a second time around.
-   (Provided here so the unit tests can replace this value so we can actually test the scheduling mechanism)."
-  (* 1000 60 60))
+(defn- hour
+  "Current hour (0 - 23) according to the system calendar."
+  []
+  (.get (Calendar/getInstance) Calendar/HOUR))
+
+(defn- minute
+  "Current minute (0 - 59) according to the system calendar."
+  []
+  (.get (Calendar/getInstance) Calendar/MINUTE))
+
+(defn- minutes-until-next-hour
+  "Number of minutes (1 - 60) until the top of the *next* hour."
+  []
+  (- 60 (minute)))
+
+(defn- hourly-task-delay
+  "Number of milliseconds to wait before running hourly tasks the next time around.
+
+   This is the number of milliseconds until the top of the next hour; e.g., if the test runner is started
+   with `(start-task-runner!)` at 8:23 PM, this function will return the number of milliseconds until 9:00 PM,
+   which will be first time we'd want
+
+   (This is provided here so the unit tests can replace this fn so we can test the scheduling mechanism.)"
+  []
+  (* 1000 60 (minutes-until-next-hour)))
 
 ;; TODO: Does it matter whether we run at the top of each hour or just once per hour?
 (defn- run-hourly-tasks
   "Run the `hourly-tasks-hook` in parallel, then sleep for an hour."
   []
   ;; Sleep first, that way we're not trying to run a ton of tasks as soon as Metabase spins up
-  (Thread/sleep hourly-task-delay)
+  (Thread/sleep (hourly-task-delay))
   (log/info "Running hourly tasks...")
-  (run-hook #'hourly-tasks-hook :parallel (.get (Calendar/getInstance) Calendar/HOUR))
+  (run-hook #'hourly-tasks-hook :parallel (hour))
   (recur))
 
 (defn- run-nightly-tasks

test/metabase/task_test.clj

@@ -6,14 +6,14 @@
 
 (defhook task-test-hook "Hook for test purposes.")
 
-(def task-test-atom
+(def task-test-atom-counter
   (atom 0))
 
-(defn- inc-task-test-atom []
-  (swap! task-test-atom inc))
+(defn- inc-task-test-atom-counter []
+  (swap! task-test-atom-counter inc))
 
-(defn- inc-task-test-atom-twice []
-  (swap! task-test-atom (partial + 2)))
+(defn- inc-task-test-atom-counter-twice []
+  (swap! task-test-atom-counter (partial + 2)))
 
 ;; ## HOOK TESTS
 
@@ -24,29 +24,29 @@
      6  ; (4)
      9] ; (5)
   [;; (1) get initial value
-   (do (reset! task-test-atom 0)   ; reset back to 0
+   (do (reset! task-test-atom-counter 0)   ; reset back to 0
        (run-hook #'task-test-hook)
-       @task-test-atom)
+       @task-test-atom-counter)
 
    ;; (2) now add a hook function. Should increment the counter once
-   (do (add-hook! #'task-test-hook inc-task-test-atom)
+   (do (add-hook! #'task-test-hook inc-task-test-atom-counter)
        (run-hook #'task-test-hook)
-       @task-test-atom)
+       @task-test-atom-counter)
 
    ;; (3) ok, run the hook twice. Should increment counter twice
    (do (run-hook #'task-test-hook)
        (run-hook #'task-test-hook)
-       @task-test-atom)
+       @task-test-atom-counter)
 
    ;; (4) add another hook function that increments counter twice on each call (for a total of + 3)
-   (do (add-hook! #'task-test-hook inc-task-test-atom-twice)
+   (do (add-hook! #'task-test-hook inc-task-test-atom-counter-twice)
        (run-hook #'task-test-hook)
-       @task-test-atom)
+       @task-test-atom-counter)
 
    ;; (5) check that we can't add duplicate hooks - should still be just +3
-   (do (add-hook! #'task-test-hook inc-task-test-atom-twice)
+   (do (add-hook! #'task-test-hook inc-task-test-atom-counter-twice)
        (run-hook #'task-test-hook)
-       @task-test-atom)])
+       @task-test-atom-counter)])
 
 
 ;; ## TASK RUNNER TESTS
@@ -54,39 +54,31 @@
 (defn- system-hour []
   (.get (Calendar/getInstance) Calendar/HOUR))
 
-(defn- inc-task-test-atom-by-system-hour [hour]
-  (swap! task-test-atom (partial + (system-hour))))
-
-;; we'll temporarily swap out the `hourly-tasks-hook` functions
-;; so the tests don't cause some crazy database analysis or w/e to start up
-(def ^:private original-hourly-tasks-hook-functions
-  (atom nil))
-(defn- stash-original-hourly-tasks-hook-functions!   [] (reset! original-hourly-tasks-hook-functions @hourly-tasks-hook))
-(defn- restore-original-hourly-tasks-hook-functions! [] (reset! hourly-tasks-hook @original-hourly-tasks-hook-functions))
-
-(expect [0
-         (system-hour)       ; we can also check that the `hourly-tasks-hook` is passing the correct param to its functions
-         (* 3 (system-hour))
-         :ok]
-  (do
-    ;; stop the task runner and set the internal interval to 30 ms.
-    ;; Add `inc-task-test-atom-by-system-hour` to the `hourly-tasks-hook` and restart
-    (stop-task-runner!)
-    (stash-original-hourly-tasks-hook-functions!)
-    (intern 'metabase.task 'hourly-task-delay 30)
-    (add-hook! #'hourly-tasks-hook inc-task-test-atom-by-system-hour)
-    (reset! task-test-atom 0)
-    (start-task-runner!)
-
-    [@task-test-atom       ; should be 0, since not enough time has elaspsed for the hook to be executed
-     (do (Thread/sleep 45)
-         @task-test-atom)  ; should have been called once (~15ms ago)
-     (do (Thread/sleep 60)
-         @task-test-atom)  ; should have been called two more times
-
-     ;; no put things back how we found them
-     (do (stop-task-runner!)
-         (intern 'metabase.task 'hourly-task-delay (* 1000 60 60))
-         (restore-original-hourly-tasks-hook-functions!)
-         (start-task-runner!)
-         :ok)]))
+(defn- inc-task-test-atom-counter-by-system-hour [hour]
+  (swap! task-test-atom-counter (partial + (system-hour))))
+
+(defhook mock-hourly-tasks-hook
+  "Hook that will replace the actual hourly-tasks-hook in our unit test.")
+
+(expect [[0
+          (system-hour)       ; we can also check that the `hourly-tasks-hook` is passing the correct param to its functions
+          (* 3 (system-hour))
+          :stopped]
+         :restarted]
+  [(do
+     (stop-task-runner!)
+     (with-redefs [metabase.task/hourly-task-delay (constantly 30)
+                   metabase.task/hourly-tasks-hook mock-hourly-tasks-hook]
+       (add-hook! #'hourly-tasks-hook inc-task-test-atom-counter-by-system-hour)
+       (reset! task-test-atom-counter 0)
+       (start-task-runner!)
+
+       [@task-test-atom-counter      ; should be 0, since not enough time has elaspsed for the hook to be executed
+        (do (Thread/sleep 45)
+            @task-test-atom-counter) ; should have been called once (~15ms ago)
+        (do (Thread/sleep 60)
+            @task-test-atom-counter) ; should have been called two more times
+        (do (stop-task-runner!)
+            :stopped)]))
+   (do (start-task-runner!)
+       :restarted)])