Clojurians Log v2

https://github.com/metabase/metabase/blob/master/test/metabase/lib/aggregation_test.cljc#L819-L823 I’m not sure i follow the warning

that's the wrong file

metabase/test/metabase/lib/aggregation_test.cljc ?

"test-regression/checkouts/metabase/test/metabase/lib/aggregation_test.cljc",

oh sorry, my bad

let's see

looks like test-regression/checkouts/ code is not the same as in he current master because of SHA

this is the location, I'm still on SHA aa0cdb546d7c9e4ef5c52ad23c656272b7599e23

https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/test/metabase/lib/aggregation_test.cljc#L819-L823

maybe it would be nice to print github links along with the findings with the right SHA so you can click on them :)

@dpsutton the issue there is that is is nested

ah i see. yeah. the nested is will only be evaluated if the outer fails? i wonder what happens here

don't know about evaluation but it's likely just a paredit typo or so

this PR detected several of them in various projects

yeah. they both evaluate. and i bet you are right it’s possibly a parinfer issue?

nah probably paredit. The author didn’t use parinfer. Parinfer was my guess because it can move stuff around more easily

I also found some of these:

(is (= foo 1) (= foo 3))

docstring says that second argument is a message:

(defmacro is
  "Generic assertion macro.  'form' is any predicate test.
  'msg' is an optional message to attach to the assertion.

yep

so it will not be evaluated as a test

user=> (is (= 1 1) (is (= 1 3)))

FAIL in () (NO_SOURCE_FILE:18)
expected: 1
  actual: 3
    diff: - 1
          + 3
true

yes, so as long as the first test succeeds, you will never noticde it

the original one worked in a very weird way but did work

agree if the second arg to is is an assertion without it’s own is wrapper

one such case here: https://github.com/nextjournal/clerk/pull/795

More suspicious ones here @dpsutton https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/test/metabase/analytics/stats_test.clj#L442-L455

(is false? ...)

Thank you. I have a PR for this, I'll add this one to it

There's more. https://github.com/clj-kondo/clj-kondo/pull/2722/files#diff-297302157a95b47ba8139cfe7a67d9d09a9ba2e753d1bbf1f8ecf8770ddfd0b4 Note that this is all based on commit aa0cdb546d7c9e4ef5c52ad23c656272b7599e23 Thanks to @jonurnieta for finding these

https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/test/metabase/app_db/internal_user_test.clj#L17

https://github.com/metabase/metabase/blob/aa0cdb546d7c9e4ef5c52ad23c656272b7599e23/test/metabase/embedding/api/embed_test.clj#L184

(not an exhaustive list, but just sampling through and they look legit)

does @bronsa now also work for metabase? I saw a commit of his

he does

nice

This is by design since sometimes you just want to enable a condition explicitly. So we don't wan on booleans in condition always true.

okey, I suspected that. Thanks!

I'm adding condition-always true to clojure.test/is https://github.com/clj-kondo/clj-kondo/issues/2340

Nice. Actually I encountered (is true (contains? ...)) since your types for clojure.test So warning on that does make sense probably

yes, I was just writting a comment

https://github.com/clj-kondo/clj-kondo/issues/2340#issuecomment-3724444905

but first argument can be warned

I did detect that case already through the type system so that was already good

but warning on a literal in is seems like a thing that can be warned about

unless it's a local reference

might want to think about always true tests that exist to test flow control (like whether an exception was thrown)

example?

(try
  ;; something that I expect to throw an exception
  (is false "oops!")
  (catch Exception e (is true))

yeah, that shouldn't be detected by clj-kondo (and currently isn't)

so maybe we should keep not warning on literal booleans

(is true) and (is false) in particular are often used for flow control testing, but maybe other cases are more likely bugs, don't know

that's the heuristic we used for condition-always-true so far

found this one in metabase

(ns mage.start-db-test
  (:require
   [clojure.test :refer [deftest testing is]]))

(deftest start-db-tests
  (testing "DB starts"
    (is "TODO add tests here")))

wonderful :)

seems like a good test to me :)

hahah

I guess so. The more interesting ones were found here where accidentally tests were added in the msg position of is https://github.com/clj-kondo/clj-kondo/pull/2706/files#diff-297302157a95b47ba8139cfe7a67d9d09a9ba2e753d1bbf1f8ecf8770ddfd0b4

like:

(is (= 1 1) (= 1 2))