Improvement suggestion: Warning against using `is` with non-booleans

[cjohansen]

It is currently quite easy to misspell (is (= "string1" "string2")) as (is "string1" "string2") because of (is form msg) (where form can be any value) - see #64.

Maybe Kaocha could issue a warning when calling is with more than one argument and the first is a string? It's technically valid, but very likely a bug, and the warning could be silenced with (is (boolean form) msg).

[plexus]

I think this is a good idea, using is with any literal in the first position should really be a failure. (so anything except a list or a var name).

[magnars]

Note that literals in first position is normal if you're using the default expected, actual order, while a literal in the second position is normal if you're using actual, expected. So if the kaocha-noyoda-plugin is to continue working, this needs to be somehow configurable, preferably at an extension point available to plugins. :)

[plexus]

You're thinking of (is (= ,,,)), we're talking about a literal inside the (is ) form itself, like (is {:hello :world} (greeting))

[magnars]

Yes, of course. Sorry for the noise. Carry on. 😄

[plexus]

hi @ashimapanjwani, certainly!

We're going to have to replace the clojure.test/assert-any function. This function looks like this:

(defn assert-any
  "Returns generic assertion code for any test, including macros, Java
  method calls, or isolated symbols."
  [msg form]
  `(let [value# ~form]
     (if value#
       (do-report {:type :pass, :message ~msg,
                :expected '~form, :actual value#})
       (do-report {:type :fail, :message ~msg,
                :expected '~form, :actual value#}))
     value#))

It's used inside a macro, which is why it returns code. (Try evaluating (clojure.test/assert-any "hello" :some-value) in a REPL and you'll see what I mean). We don't "own" this function, it's part of Clojure, so we can't just go in and change the source. But we can replace it at runtime which is referred to as monkey patching.

Have a look at the kaocha.monkey-patch namespace. There's actually some earlier code of mine in there commented out that replaces assert-any, this was for a different reason so you can ignore the actual definition, but you can see what the idea is. First we define our own version.

(defn assert-any ...)

And then at the end replace the version from clojure.test with our own.

(alter-var-root #'t/assert-any (constantly assert-any))

Inside assert-any we'll have to check if form is any of the following: string?, vector?, map?, number?, keyword?. If that's the case then we should emit a warning (use kaocha.output/warn), for example (filling in <form> and <msg> with the actual form/message.

WARNING: asserting a data literal will always pass, did you forget an (= ...) in (is <form> <msg>)?

The final tricky thing is that you will need to do the check for this when assert-any is called, so that you can still access the unevaluated form, but I think it's best to only emit the warning when the generated code gets run. So something like this

(defn assert-any [msg form]
  (let [literal? (or (string? form) ...)]
    `(let [value# ~form]
        (when literal?
           (kaocha.output/warn ...))
        ...original code...
        )))

I hope that's clear. Have you come across macros before? Happy to find you some resources. "`" is pronounced "backtick", "backquote" or "syntax quote", in case you want to know how to google for that.

[alysbrooks]

For a real-world example, I think #190 describes a test with a similar problem, but I don't think the suggested fix would catch it, because we're building up the value (a string) using a function call. I think we should still go ahead with the suggested fix, but we might want to consider ways to catch subtler versions of the problem, too.

[plexus]

That's a trickier one, we can't assume that the function called in is will always return a boolean, people will regularly rely on the fact that non-nil non-false values are truthy.

[alysbrooks]

That's true. I suppose we could catch this mistake with some spec'd functions because we could check if we're calling a function that is spec'd to only return truthy values. Otherwise, I'm not sure it's feasible.

[alysbrooks]

This is a useful feature that I don't think we will have time to address in the near future. PRs implementing it are welcome, however.