Rotor appender: all logs being rotated after max count reached

src/taoensso/timbre/appenders/rotor.clj

@@ -36,21 +36,14 @@
   will be deleted. In future, there will be a suffix fn to customize
   the naming of archived logs."
   [basepath max-count]
-  (let [abs-path (-> basepath io/file (.getAbsolutePath))
+  (let [abs-path                        (-> basepath io/file (.getAbsolutePath))
-        logs (->> basepath
+        logs                            (-> basepath matching-files sort)
-                  matching-files
+        [logs-to-rotate logs-to-delete] (split-at max-count logs)]
-                  (take max-count)
+    (doseq [log-to-delete logs-to-delete]
-                  (map (fn [^File x] (.getAbsolutePath x)))
+      (io/delete-file log-to-delete))
-                  sort
+    (doseq [[^File log-file n]
-                  reverse)
+            (reverse (map vector logs-to-rotate (iterate inc 1)))]
-        num-logs (count logs)
+      (.renameTo log-file (io/file (format "%s.%03d" abs-path n))))))
-        overflow? (> num-logs max-count)]
-    (when overflow?
-      (io/delete-file (first logs)))
-    (loop [[log & more] (if overflow? (rest logs) logs) n num-logs]
-      (when log
-        (.renameTo (io/file log) (io/file (format "%s.%03d" abs-path n)))
-        (recur more (dec n))))))
 
 (defn appender-fn [{:keys [ap-config output]}]
   (let [{:keys [path max-size backlog]

test/taoensso/test/timbre/appenders/rotor.clj

@@ -0,0 +1,40 @@
+(ns taoensso.test.timbre.appenders.rotor
+  (:require [taoensso.timbre.appenders.rotor :as rotor :refer :all]
+            [clojure.test                              :refer :all]
+            [clojure.java.io                           :refer [file]]))
+
+(defn with-temp-dir-containing-log-files
+  "Call f with the temp directory name, that directory having n log
+  files created within it"
+  [n f]
+  (let [tmp-dir (java.io.File/createTempFile "test" "")
+        log-file-basename "log"
+        log-files (into [log-file-basename]
+                        (map #(format "%s.%03d" log-file-basename %) (range 1 n)))]
+    (.delete tmp-dir)
+    (.mkdirs tmp-dir)
+    (doseq [filename log-files] (.createNewFile (file tmp-dir filename)))
+    (try
+      (f (.getAbsolutePath (file tmp-dir (first log-files))))
+      (finally
+        (doseq [filename log-files] (.delete (file tmp-dir filename)))
+        (.delete (file tmp-dir))))))
+
+(deftest test-rotor
+  (testing "rotating logs"
+    (testing "exposing the bug TODO write something better"
+      ;; when we rotate with a full backlog of log files,
+      ;; the last should get deleted
+      (with-temp-dir-containing-log-files 5
+        (fn [basepath]
+          (#'rotor/rotate-logs basepath 2)
+          (is (not (.exists (file (str basepath))))
+              "log should have been rotated to log.001")
+          (is (.exists (file (str basepath ".001")))
+              "log.001 should remain")
+          (is (.exists (file (str basepath ".002")))
+              "log.002 should remain")
+          (is (not (.exists (file (str basepath ".003"))))
+              "log.003 should be deleted because it is past the max-count threshold")
+          (is (not (.exists (file (str basepath ".004"))))
+              "log.004 should be deleted because it is past the max-count threshold"))))))