Kondo linter tests for :metabase/validate-logging (metabase#43104)

* PoC Kondo linter test

* Log linter tests

* Remove fail-test

.clj-kondo/config.edn

@@ -1,5 +1,5 @@
 ;; -*- comment-column: 100; -*-
-{:config-paths ["macros"]
+{:config-paths ["macros" "src"]
  :linters
  {:aliased-namespace-symbol     {:level :warning}
   :invalid-arity                {:skip-args [metabase.lib.util.match/match]} ; TODO (cam): can we fix these?

.clj-kondo/hooks/common.clj → .clj-kondo/src/hooks/common.clj

@@ -376,8 +376,15 @@
     (when (hooks/token-node? node)
       (let [sexpr (hooks/sexpr node)]
         (when (symbol? sexpr)
-          (when-let [resolved (hooks/resolve {:name sexpr})]
-            (symbol (name (:ns resolved)) (name (:name resolved)))))))
+          (let [resolved (hooks/resolve {:name sexpr})]
+            (cond
+              (and resolved (:ns resolved))
+              (symbol (name (:ns resolved)) (name (:name resolved)))
+
+              ;; if it wasn't resolved but is still qualified it's probably using the full namespace name rather than an
+              ;; alias.
+              (qualified-symbol? sexpr)
+              sexpr)))))
     ;; some symbols like `*count/Integer` aren't resolvable.
     (catch Exception _
       nil)))

.clj-kondo/hooks/metabase/util/log.clj → .clj-kondo/src/hooks/metabase/util/log.clj

@@ -124,40 +124,3 @@
       (check-for-i18n-format-specifiers node format-string)
       (check-format-string-arg-count node format-string args)))
   x)
-
-(comment
-  ;; should fail, missing a format arg
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf "Migration lock was acquired after %d retries.")))})
-
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf e "Migration lock was acquired after %d retries.")))})
-
-  ;; should fail, too many format args
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf "Migration lock was acquired after %d retries." 1 2)))})
-
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf e "Migration lock was acquired after %d retries." 1 2)))})
-
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf "Migration lock was acquired after {0} retries." 1)))})
-
-  ;; should fail, has format args but is warn rather than warnf
-  (info
-   {:node (api/parse-string
-           (str '(log/warn e "Migration lock was acquired after %d retries." 1 2)))})
-
-  ;; should fail -- should not use i18n/tru or i18n/trs
-  (infof
-   {:node (api/parse-string
-           (str '(log/warnf (metabase.util.i18n/tru "Migration lock was acquired after {0} retries." 1))))})
-
-  (info
-   {:node (api/parse-string
-           (str '(log/warn (metabase.util.i18n/trs "Migration lock was acquired after {0} retries." 1))))}))

.clj-kondo/test/hooks/common_test.clj

@@ -0,0 +1,19 @@
+(ns ^:mb/once hooks.common-test
+  (:require
+   [clj-kondo.hooks-api :as api]
+   [clj-kondo.impl.utils]
+   [clojure.test :refer :all]
+   [hooks.common]))
+
+(deftest ^:parallel node->qualified-symbol-test
+  (binding [clj-kondo.impl.utils/*ctx* {:namespaces (atom nil)}]
+    (is (= 'metabase.util.i18n/tru
+           (hooks.common/node->qualified-symbol (api/parse-string "metabase.util.i18n/tru"))))))
+
+(deftest ^:parallel node->qualified-symbol-test-2
+  (binding [clj-kondo.impl.utils/*ctx* {:namespaces (atom {:clj {:clj '{hooks.common-test {:qualify-ns {i18n metabase.util.i18n}}}}})
+                                        :lang       :clj
+                                        :base-lang  :clj
+                                        :ns         {:name 'hooks.common-test}}]
+    (is (= 'metabase.util.i18n/tru
+           (hooks.common/node->qualified-symbol (api/parse-string "i18n/tru"))))))

.clj-kondo/test/hooks/metabase/util/log_test.clj

@@ -0,0 +1,77 @@
+(ns ^:mb/once hooks.metabase.util.log-test
+  (:require
+   [clj-kondo.hooks-api :as api]
+   [clj-kondo.impl.utils]
+   [clojure.string :as str]
+   [clojure.test :refer :all]
+   [hooks.metabase.util.log]))
+
+(defn- warnings
+  [form]
+  (let [f (if (str/ends-with? (name (first form)) "f")
+            hooks.metabase.util.log/infof
+            hooks.metabase.util.log/info)]
+    (binding [clj-kondo.impl.utils/*ctx* {:config     {:linters {:metabase/validate-logging {:level :warning}}}
+                                          :ignores    (atom nil)
+                                          :findings   (atom [])
+                                          :namespaces (atom {})}]
+      (f {:node (api/parse-string (pr-str form))})
+      (mapv :message @(:findings clj-kondo.impl.utils/*ctx*)))))
+
+(deftest ^:parallel warn-on-missing-format-args-test
+  (testing "should fail, missing a format arg"
+    (are [form] (= ["log format string expects 1 arguments instead of 0."]
+                   (warnings (quote form)))
+      (metabase.util.log/warnf "Migration lock was acquired after %d retries.")
+      (metabase.util.log/warnf e "Migration lock was acquired after %d retries."))))
+
+(deftest ^:parallel warn-on-too-many-format-args-test-1
+  (testing "should fail, too many format args"
+    (are [form] (= ["log format string expects 1 arguments instead of 2."]
+                   (warnings (quote form)))
+      (metabase.util.log/warnf "Migration lock was acquired after %d retries." 1 2)
+      (metabase.util.log/warnf e "Migration lock was acquired after %d retries." 1 2))))
+
+(deftest ^:parallel warn-on-too-many-format-args-test-2
+  (testing "should fail, too many format args"
+    (are [form] (= ["this looks like an i18n format string. Don't use identifiers like {0} in logging."
+                    "Don't use metabase.util.log/warnf with no format string arguments, use metabase.util.log/warn instead."
+                    "log format string expects 0 arguments instead of 1."]
+                   (warnings (quote form)))
+      (metabase.util.log/warnf "Migration lock was acquired after {0} retries." 1)
+      (metabase.util.log/warnf e "Migration lock was acquired after {0} retries." 1))))
+
+(deftest ^:parallel warn-on-format-args-without-logf-test
+  (testing "should fail, has format args but is warn rather than warnf"
+    (are [form] (= ["metabase.util.log/warn used with a format string, use metabase.util.log/warnf instead."]
+                   (warnings (quote form)))
+      (metabase.util.log/warn "Migration lock was acquired after %d retries." 1)
+      (metabase.util.log/warn e "Migration lock was acquired after %d retries." 1))))
+
+(deftest ^:parallel warn-on-format-without-logf-test
+  (testing "should fail, has format args but is warn rather than warnf"
+    (are [form] (= ["Use metabase.util.log/warnf instead of metabase.util.log/warn + format"]
+                   (warnings (quote form)))
+      (metabase.util.log/warn (format "Migration lock was acquired after %d retries." 1))
+      (metabase.util.log/warn e (format "Migration lock was acquired after %d retries." 1)))))
+
+(deftest ^:parallel warn-on-logf-with-no-format-args-test
+  (testing "should fail, has format args but is warn rather than warnf"
+    (are [form] (= ["Don't use metabase.util.log/warnf with no format string arguments, use metabase.util.log/warn instead."]
+                   (warnings (quote form)))
+      (metabase.util.log/warnf "Migration lock cleared.")
+      (metabase.util.log/warnf e "Migration lock cleared."))))
+
+(deftest ^:parallel warn-on-i18n-test-1
+  (testing "should fail -- should not use i18n/tru or i18n/trs"
+    (are [form] (= ["do not i18n the logs!"]
+                   (warnings (quote form)))
+      (metabase.util.log/warnf (metabase.util.i18n/tru "Migration lock was acquired after {0} retries." 1))
+      (metabase.util.log/warnf e (metabase.util.i18n/tru "Migration lock was acquired after {0} retries." 1)))))
+
+(deftest ^:parallel warn-on-i18n-test-2
+  (testing "should fail -- should not use i18n/tru or i18n/trs"
+    (are [form] (= ["do not i18n the logs!"]
+                   (warnings (quote form)))
+      (metabase.util.log/warn (metabase.util.i18n/trs "Migration lock was acquired after {0} retries." 1))
+      (metabase.util.log/warn e (metabase.util.i18n/trs "Migration lock was acquired after {0} retries." 1)))))

.gitignore

@@ -96,7 +96,8 @@ modules/drivers/*/.lsp/*
 .clj-kondo/*
 !.clj-kondo/README.md
 !.clj-kondo/config.edn
-!.clj-kondo/hooks/
+!.clj-kondo/src/
+!.clj-kondo/test/
 !.clj-kondo/macros/
 
 # Editor- or environment-specific local files

deps.edn

@@ -254,7 +254,7 @@
     ring/ring-mock               {:mvn/version "0.4.0"}                     ; creating Ring request maps for testing purposes
     talltale/talltale            {:mvn/version "0.5.14"}}                   ; generates fake data, useful for prototyping or load testing
 
-   :extra-paths ["dev/src" "local/src" "test" "test_resources"]
+   :extra-paths ["dev/src" "local/src" "test" "test_resources" ".clj-kondo/src" ".clj-kondo/test"]
    :jvm-opts    ["-Dmb.run.mode=dev"
                  "-Dmb.field.filter.operators.enabled=true"
                  "-Dmb.test.env.setting=ABCDEFG"
@@ -619,9 +619,9 @@
    :extra-deps
    {org.clojure/data.json {:mvn/version "2.5.0"}}}
 
-  ;;; Run tests for the build scripts:
-  ;;;
-  ;;; clj -X:dev:drivers:build:build-dev:build-test
+  ;; Run tests for the build scripts:
+  ;;
+  ;; clj -X:dev:drivers:build:build-dev:build-test
   :build-test
   {:exec-fn   mb.hawk.core/find-and-run-tests-cli
    :exec-args {:only ["bin/build/test"]}}
@@ -703,6 +703,12 @@
   {:exec-args {:only [metabase.api.search-test
                       "test/metabase/search"]}}
 
+  ;; test custom clj-kondo linters and hooks.
+  ;;
+  ;;    clj -X:dev:test:test/kondo
+  :test/kondo
+  {:exec-args {:only [".clj-kondo/test"]}}
+
   ;; watch and reload clojure namespaces
   :watch {:extra-deps
           {com.nextjournal/beholder     {:mvn/version "1.0.2"} ;; watcher

src/metabase/util/jvm.clj

@@ -315,5 +315,5 @@
             (if (>= elapsed-time timeout-ms)
               nil ; timeout reached
               (do
-                (Thread/sleep interval-ms)
+                (Thread/sleep (long interval-ms))
                 (recur)))))))))

test/metabase/test_runner.clj

@@ -82,7 +82,7 @@
    "test_resources"])
 
 (defn- default-options []
-  {:namespace-pattern   #"^metabase.*"
+  {:namespace-pattern   #"^(?:(?:metabase.*)|(?:hooks\..*))" ; anything starting with `metabase*` (including `metabase-enterprise`) or `hooks.*`
    :exclude-directories excluded-directories
    :test-warn-time      3000})