Fork me on GitHub
#kaocha
<
2020-04-01
>
rickmoynihan16:04:39

Has anyone seen this error from kaocha/expound before?

Exception: clojure.lang.ExceptionInfo: Call to expound.alpha/printer did not conform to spec:
alpha.clj:258

-- Spec failed --------------------

Function arguments

  (nil)
   ^^^

should satisfy

  map?

-------------------------
Detected 1 error

{:clojure.spec.alpha/problems [{:path [:args :explain-data], :pred clojure.core/map?, :val nil, :via [], :in [0]}], :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__2509 0x1b78c761 "[email protected]"], :clojure.spec.alpha/value (nil), :clojure.spec.alpha/args (nil), :clojure.spec.alpha/failure :instrument, :orchestra.spec.test/caller {:file "alpha.clj", :line 258, :var-scope clojure.spec.alpha/explain-out}, :orchestra.spec/var #'expound.alpha/printer}
 at orchestra.spec.test$spec_checking_fn$conform_BANG___3298.invoke (test.cljc:115)
    ...

rickmoynihan17:04:45

I think this is being caused by a test of mine calling (s/explain-str :a.spec.of/ours data)

bbrinck17:04:39

Hm, for some reason it looks like the printer is being called with nil for explain-data.

bbrinck17:04:06

Is it possible that somehow explain-str is being called even though the data actually matches the spec?

bbrinck17:04:27

(s/explain-data int? 1) ;; nil

rickmoynihan17:04:38

I am explicitly calling s/explain-str in my test

bbrinck17:04:56

what does (s/valid? :a.spec.of/ours data) return

rickmoynihan17:04:54

s/explain-data returns nil

rickmoynihan17:04:03

as you’d expect

rickmoynihan17:04:35

It looks like expound in kaocha is interfering with spec usage in my test. At a repl it works; but via ./bin/kaocha it fails with the expound error

bbrinck17:04:31

Hm, I’d be curious to see how expound is set up in your codebase and/or kaocha. e.g. presumably there is a (set! s/*explain-out* expound/printer) somewhere

rickmoynihan17:04:13

I can’t find any reference to expound in my app anywhere

rickmoynihan17:04:18

that was my first thought too

rickmoynihan17:04:06

but kaocha includes expound & orchestra now

bbrinck17:04:10

Ok I’ll see if I can build a local repro this evening

bbrinck19:04:43

If you can run same test with another test runner, that’d be useful as well.

bbrinck19:04:58

I realize you tried the REPL but a REPL potentially has other state, so it’d be useful to try “lein test” or an equivalent

bbrinck02:04:12

I can reproduce here. I’ll dig into it more tomorrow.

bbrinck02:04:37

The workaround is to redefine the fdef spec (or disable instrumentation for the functions in expound):

(s/fdef expound/printer
        :args (s/cat :explain-data (s/nilable map?))
        :ret nil?)

rickmoynihan10:04:15

@U08EKSQMS Thanks for looking into this for me. What is it that is currently defining that fdef?

rickmoynihan10:04:32

I can’t seem to find it in expound; or kaocha

rickmoynihan11:04:30

So I guess the reason this happens and hasn’t been discovered before is because expounds printer assumes it never needs to print success messages? i.e. the binding approach doesn’t currently assume that users are going to call s/explain-str themselves within that binding form?

bbrinck13:04:33

@U06HHF230 I suspect the reason this hasn’t been reported earlier is that you need 3 things to be true. 1. You must use explain-str (not expound-str) 2. You must have a case where you are calling explain-str for data that matches the spec (a common use case is to only call this if s/valid? returns false) 3. You must have instrumented expound/printer I suspect that’s quite rare

👍 4
bbrinck13:04:32

;; Using expound directly works
  (expound/expound-str :foobar/name "")
  ;; => "Success!\n"  

  ;; A common pattern of using valid? + explain-str works
  (binding [s/*explain-out* expound/printer]
    (if-not (s/valid? :foobar/name "")
      (s/explain-str :foobar/name ""))
    )
  ;; => nil

  ;; If instrumentation is off, it works
  (binding [s/*explain-out* expound/printer]
    (s/explain-str :foobar/name "")
    )
  ;; => "Success!\n"  )

  ;; But if instrumentation is turned on, it fails!
  (st/instrument)
  (binding [s/*explain-out* expound/printer]
    (s/explain-str :foobar/name "")
    )
  ;; error

  ;; Even w/ instrumentation, expound-str works
  (expound/expound-str :foobar/name "")
  ;; => "Success!\n"

  ;; ... as does pattern of using valid? + explain-str

  (binding [s/*explain-out* expound/printer]
    (if-not (s/valid? :foobar/name "")
      (s/explain-str :foobar/name ""))
    )
  ;; => nil

rickmoynihan15:04:45

Yeah I suspect it is quite rare… The code in question here was actually a clojure.test assertion that some test data conformed to the clojure spec…

(= "Success!\n" (s/explain-str :mut.download/job-instance-schema test-job))
It’s pretty hacky, however it was written that way because it gives a better clojure.test error when the spec fails.

rickmoynihan15:04:07

an s/valid? call just gives a true/false error etc.

rickmoynihan15:04:30

Instrumenting is a better approach generally to this kind of thing too… but there are some problems with instrumentation in this code base. Mainly that we use integrant and load-namespaces quite extensively; so not all specs in use are loaded upfront; because the code loading is dynamic.

bbrinck15:04:24

I think the following would work

(is (s/valid? :foobar/name 1)
        (s/explain-str :foobar/name 1)
        ))
The above is repetitive, but I suspect some macro magic could clean it up 🙂

bbrinck15:04:13

(basically test for valid but then use explain-str for the is failure message)

rickmoynihan15:04:49

I guess that’ll still cause the expound problem though 🙂

bbrinck15:04:03

It doesn’t seem to

bbrinck15:04:49

I haven’t dug into the clojure.test code, but my suspicion is that the message is not evaluated unless is check fails

rickmoynihan15:04:29

yeah I guess the messages are evaluated later outside the binding

bbrinck15:04:46

Oh, nevermind. I still had the “workaround” spec declared. The above won’t work 😞

bbrinck15:04:06

I suppose you could use a macro to delay eval of the message but at that point it’s getting pretty complicated

rickmoynihan15:04:15

can we not just correct the printer spec in expound; to allow nil? Or does that imply having the printer print success messages?

rickmoynihan15:04:10

i.e. your workaround; I’m curious why that’s not a fix.

bbrinck15:04:17

It is the fix

bbrinck15:04:34

I just haven’t had time to apply it and cut a release 🙂

rickmoynihan15:04:48

ok, well I can wait. It’s not a big deal.

rickmoynihan15:04:08

I’d already commented out the assertion for now anyway.

👍 4
bbrinck15:04:13

FWIW, I haven’t used it, but there are also fancier tools to assert that some data matches a spec. YMMV https://github.com/nedap/utils.spec/blob/3c9c0f32d3bb436aa8cc6b975b3d843ca95968ab/src/nedap/utils/spec/api.cljc#L9-L15

rickmoynihan15:04:08

ahh thanks good to know — this code is pretty old. I had written similar things in the past, but never extracted it into something to reuse as it was always a bit hacky.

rickmoynihan15:04:36

and made do with the string check in this instance

bbrinck15:04:05

Yeah, it makes sense. The string check is a good solution - if there wasn’t this bug 😆 . In any case, thanks for reporting it, I’ll try to get it fixed soon.

rickmoynihan15:04:38

Well thanks a million anyway, it was insightful 😁