Fix #836 clj-kondo linter generation when using pred :fn's

[juszczakn]

Trying to fix #836 . When using a predicate fn schema inside of a function schema, it would output a the type as :fn, such that clj-kondo checked that the arg provided is a fn and not of type any.

Ex:

(defn times [x y z] (* x y z))
(m/=> times [:=> [:cat int? [:fn #(int? %)] int?] [:fn #(int? %)]])

(times 1 2 3)
;; clj-kondo warns that `2` should be a function, not an integer.

[juszczakn]

I wanted to have a separate test for this specific issue, so that it could have it's own assertion w/ docstring. While doing so, this test broke because it's testing the entire ns, which seemed unnecessary. I would think as this test grew bigger, the chances for errors/mistakes to sneak through in a larger and larger assertion might grow, particularly with no docstrings explaining these.

So I updated the test to stop it from growing. New tests can be written in their own deftests, which seems better to me? /shrug

[ikitommi]

Thanks for thinking about this. We now have two methods for testing which adds complexity (extra filtering list of symbols to be tested with 1). I would choose just one way to test these:

1. Lean/Coherent: just add the new cases into to the existing assertion map. One could add comments above the expected map entry as extra info. Other malli test namespaces have this, e.g. swagger-tests so it's a common testing pattern. Adding a new case (here, a new defn) means the existing tests would break, but it's a good and automatic indication you have to add a new expectation too. Less code, but also less info/context on errors.
2. Normal: a helper that allows to test each defn separately in a own deftest / testing or is block. athen break down clj-kondo-integration-test.

I'm ok with both, but as a minimalist, would have done 1 myself.

Could you migrate the full test ns to use either one?

[juszczakn]

If other test ns' are already following this pattern, I think consistency is worth going for, so I'll just update it to do (1).

Personally, though, I don't really like using comments for this kinda thing. My problems w/ it are:

• I have a tendency to just copy-paste 'actual' test output back into the test if it matches what I expect, and that ends up erasing the comments. (might just be a 'me' problem :) )
• Like you mention, generally less UX friendly than having the str actually show up in the test failure output, extra legwork to cross-examine and figure things out. Relatively minor issue /shrug.

[ikitommi]

Looks good, one wish for test coherency. I can do that after merge if yiu don't have time to do that.

[ikitommi]

Thanks for thinking about this. We now have two methods for testing which adds complexity (extra filtering list of symbols to be tested with 1). I would choose just one way to test these:

1. Lean/Coherent: just add the new cases into to the existing assertion map. One could add comments above the expected map entry as extra info. Other malli test namespaces have this, e.g. swagger-tests so it's a common testing pattern. Adding a new case (here, a new defn) means the existing tests would break, but it's a good and automatic indication you have to add a new expectation too. Less code, but also less info/context on errors.
2. Normal: a helper that allows to test each defn separately in a own deftest / testing or is block. athen break down clj-kondo-integration-test.

I'm ok with both, but as a minimalist, would have done 1 myself.

Could you migrate the full test ns to use either one?

[juszczakn]

Ok, I updated the PR to follow the IT test standard.

I'm not sure why the workflow is failing for cljs? I don't really understand the error message and I don't have any cljs experience at all, so I'm kinda lost there.

ERROR in (check-test) (Error:NaN:NaN)
Uncaught exception, not in assertion.
expected: nil
  actual: #error {:message ":malli.generator/no-generator", :data {:type :malli.generator/no-generator, :message :malli.generator/no-generator, :data {:options {:malli.core/function-checker #object[malli$generator$function_checker], :malli.generator/original-generator-schema #object[malli.core.t_malli$core24556]}, :schema #object[malli.core.t_malli$core24462]}}}

[ikitommi]

instrumentation tests were testing all registered function schemas using generative testing. I added a namespace-filter that it only tests functions from that namespace. See 2caabf1.

the schema [:=> [:cat int? [:fn #(int? %)] int?] [:fn #(int? %)] can be tested with generative testing as the :fn doesn't generate anything. In real life, if you need :fn that can generate values, you could mark it to use :int generator:

[:=> [:cat int? [:fn {:gen/schema :int} #(int? %)] int?] [:fn {:gen/schema :int} #(int? %)]

[ikitommi]

Thanks!!